-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
fix(autoloader): clear apcu with app management #40191
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
Conversation
Signed-off-by: Arthur Schiwon <[email protected]>
|
Related to #38797 (comment) This would however only work when enabling the app through the web interface i think. As far as I remember apcu is not shared between web workers (mod-php/php-fpm) and cli, so using occ would still have this problem. |
Yes, good point. There we would need to set a flag to clear it next time in web context. |
|
Alternatively we create and store a new hash every time an update happens or an app is enabled or disabled. And now I wonder what happens, if an app is enabled only for a specific groups of users, and its classes are requested by someone else and not being cached. |
For the record, that prefix there is irrelevant for the autoloader, it is this one: Line 616 in cf2e3bc
|
|
Discussing this with @juliushaertl we think entirely removing using apcu for autoloading is most beneficial. Then we do not need to deal with all those side effects, and also the benefit should be fairly small. Relevant bits like the class maps should be sitting in opcache already. We'd follow with this instead, unless there are objections? |
| )); | ||
| $this->clearAppsCache(); | ||
|
|
||
| if (function_exists('apcu_delete') && class_exists(\APCUIterator::class)) { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
follow up PR in #40349 |
Summary
Composer uses apcu to store its results, if available. This includes misses: if a looked up class does not exist, it will be stored as such.
Now some apps may have – although highly discouraged – (optiona) inter-app dependencies. So if a class of an app is requested, but not found, it may lead to autoloading problems once the app is actually enabled – as described in nextcloud/groupfolders#2478 (comment)
This will probably also work vice versa: when disabling and app, its classes my still appear available.
In this draft I only add logic to delete autoloading values when enabling an app. This succeeds as a poc to solve mentioned issue. I think it it also reasonable to reset the opcache in enabling/disabling scenarios, but also in for updates. Perhaps it can be done more cleverer to treat bulk operations just once, but it might not be necessary as it does not happen often, does not take long, and would make it unnecessary complex (even if not that much in total).
Any other thoughts about this? @icewind1991 @juliushaertl @ChristophWurst