Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix listeners declaration in case of occ usage #30889

Merged
merged 1 commit into from
Feb 28, 2022
Merged

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Jan 27, 2022

OC\Core\Application does not get constructed when an occ command is
called so some listeners where not correctly plugged, and for instance
calling occ user:delete would not clear user files.

So I moved these listener declarations to Server class instead where there were already some of them. It does fix occ user:delete but I am unsure about other effect.

I replace $container->query(Connection::class) in some listener to $this->get(Connection::class), not 100% sure about this either.
Moving the registerNotifierService calls would crash so I let them in OC\Core\Application instead. Maybe it’s fine that they are not called from occ?

An alternative would be to instantiate OC\Core\Application from occ command, but not sure if it makes sense.

Signed-off-by: Côme Chilliet come.chilliet@nextcloud.com

@come-nc come-nc self-assigned this Jan 27, 2022
@come-nc come-nc added 3. to review Waiting for reviews bug labels Jan 27, 2022
@come-nc come-nc added this to the Nextcloud 24 milestone Jan 27, 2022
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐘 👍 if it works

@icewind1991
Copy link
Member

I would prefer not stuffing this into the already bloated server.php, always initiating OC\Core\Application on occ commands seems sensible to me.

I replace $container->query(Connection::class) in some listener to $this->get(Connection::class), not 100% sure about this either.

That is fine.

@come-nc
Copy link
Contributor Author

come-nc commented Feb 3, 2022

I would prefer not stuffing this into the already bloated server.php, always initiating OC\Core\Application on occ commands seems sensible to me.

That works too, as long as occ commands get the same context as web ui.

@icewind1991 Do you know where the OC\Core\Application gets constructed for web UI? Where/how should it be constructed for occ commands?

@come-nc
Copy link
Contributor Author

come-nc commented Feb 3, 2022

@icewind1991 Do you know where the OC\Core\Application gets constructed for web UI? Where/how should it be constructed for occ commands?

In the web UI OC\Core\Application gets built in the routes.php file of core.

@icewind1991 Would you prefer this as a solution then:

diff --git a/lib/private/Console/Application.php b/lib/private/Console/Application.php
index 12d54b48fa..11a92dc698 100644
--- a/lib/private/Console/Application.php
+++ b/lib/private/Console/Application.php
@@ -30,6 +30,7 @@
  */
 namespace OC\Console;
 
+use OC\Core\Application as CoreApplication;
 use OC\MemoryInfo;
 use OC\NeedsUpdateException;
 use OC_App;
@@ -69,6 +70,8 @@ class Application {
                $this->request = $request;
                $this->logger = $logger;
                $this->memoryInfo = $memoryInfo;
+               /* Build core application to make sure that listeners are registered */
+               \OC::$server->query(CoreApplication::class);
        }
 
        /**

@icewind1991
Copy link
Member

That should be fine yes

@come-nc
Copy link
Contributor Author

come-nc commented Feb 3, 2022

Are we sure there are no other initialization paths than those 2? (either core/routes.php is loaded or OC\Console\Application gets built?)

I know that deleting users through ocs api does not have the problem so I guess both web UI and ocs api go through core/routes.php, is there any other way to run Nextcloud that I am missing? What about dav access, does that init core/routes.php as well?

@come-nc
Copy link
Contributor Author

come-nc commented Feb 3, 2022

This new fix does not work, it breaks installation somehow, I get this locally:

./autotest.sh sqlite
Using PHP executable /usr/bin/php
Using database oc_autotest
Setup environment for sqlite testing on local storage ...
0 chemin mis à jour depuis l'index
Installing ....
The username is already being used

The PHP unit in the CI fails with an other error, not sure what is happening here…

@icewind1991
Copy link
Member

Probably something related to trying to setup to much on an uninstalled setup

You can wrap it in a if ($config->getSystemValueBool('installed')) { to stop it from affecting instalation

@come-nc come-nc force-pushed the fix/fix-occ-listeners branch 2 times, most recently from be0aca8 to 9127292 Compare February 7, 2022 16:30
@come-nc
Copy link
Contributor Author

come-nc commented Feb 7, 2022

Probably something related to trying to setup to much on an uninstalled setup

You can wrap it in a if ($config->getSystemValueBool('installed')) { to stop it from affecting instalation

It works.

Are we sure this was the only init path where OC\Console\Application was not built? Are web ui, ocs api and dav all building it?

@come-nc
Copy link
Contributor Author

come-nc commented Feb 22, 2022

Maybe we can move this to base.php to have it in only one place?

@come-nc
Copy link
Contributor Author

come-nc commented Feb 24, 2022

(At this point PR should be squashed before merge)

…ers correctly

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc merged commit 5c4cba1 into master Feb 28, 2022
@come-nc come-nc deleted the fix/fix-occ-listeners branch February 28, 2022 10:52
@come-nc
Copy link
Contributor Author

come-nc commented Mar 1, 2022

/backport to stable23

@come-nc
Copy link
Contributor Author

come-nc commented Mar 1, 2022

/backport to stable22

@come-nc
Copy link
Contributor Author

come-nc commented Mar 1, 2022

/backport to stable21

@backportbot-nextcloud
Copy link

The backport to stable21 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable22 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable23 failed. Please do this backport manually.

@CarlSchwan
Copy link
Member

Backports?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

local storage is no longer getting cleaned up when user is deleted with occ user:delete
6 participants