- Issue created by @catch
- π¬π§United Kingdom catch
Pushed up a proof of concept.
Still @todo:
1. Need to define the array structure for when we want to issue a deprecation and then handle trigger_error() in the classloader.
2. Need to put the moved classes into a container parameter so that we don't have to hard-code them in DrupalKernel and can add them from modules etc.
- π¬π§United Kingdom catch
I think I figured out the bits in #4, see the MR.
- π¬π§United Kingdom catch
We should do the container parameter fiddling in a compiler pass, then pass one array into the bc class loader, but this was enough to figure out the test coverage and the YAML format.
- π¬π§United Kingdom catch
There was an issue to make the CR link optional somewhere, which I can't find now. But even if we do that, it will take a while to decide, so probably need to add CR support here. I got annoyed between having to add a fake CR in the test vs. phpcs failures. Can look at that when sorting out the container parameter stuff.
- π¬π§United Kingdom catch
Not keen on the fact we can't remove container parameters, maybe we can set them to empty or something, but for now merging without removing anything.
- π¬π§United Kingdom catch
Main current use case for this apart from the existing class_alias in core is for π [meta] Lots of non-plugin PHP classes in plugin directories Active where we can potentially save parsing 100 classes every cache clear, but can only do so when those classes are actually moved (as opposed to deprecating the class in-place) - if we add this support, then we can git mv the files, add them to moved_classes - no need to leave the old class around on disk.
- πΊπΈUnited States nicxvan
This is great!
I think the answer is no, but is there anyway this can allow the copied classes to add return types too?
- π¬π§United Kingdom catch
I think adding return types would be the same as if we added return types without moving the class.
- π¬π§United Kingdom alexpott πͺπΊπ
Not keen on the fact we can't remove container parameters,
Doesn't
$containerBuilder->getParameterBag()->remove()
allow you do to that? - π¬π§United Kingdom catch
$containerBuilder->getParameterBag()->remove()
It's not on the interface and I think an earlier iteration of the MR I was getting complaints from the DebugClassLoader for calling it (or something like that). I do think we can probably set them to empty arrays instead, which would save most of the space in the container.
Another option is that Symfony allows you to pass parameters, or the entire ParameterBag, to services, and we could have the classloader as a service, inject that way, and then register that as a classloader, but that just feels horrible.
The Needs Review Queue Bot β tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide β to find step-by-step guides for working with issues.
- π¬π§United Kingdom catch
This is now blocking π Add a fallback classloader that can handle missing traits Active which is in turn currently blocking π Triggering deprecations for plugins using annotations when core plugin type has been converted to attributes Needs work so bumping priority and tagging.
One comment since it looks like the
change_url
is required in the deprecation message.- πΊπΈUnited States nicxvan
Took a pretty good look at this, there are two comments to address still.
I think this needs a change record to let contrib know they can use this too I would think.
Feels pretty close otherwise.
I see the change record, so removing tag. I have a couple more suggestions after looking again.
- π¬π§United Kingdom catch
Applied the two suggestions, both of which made sense to me.
- πΊπΈUnited States nicxvan
Read through the CR, it looks clear to me too!
- π¬π§United Kingdom alexpott πͺπΊπ
I asked myself "what could possibly go wrong?" and I wondered if we could create a loop so I added
core.moved_classes: 'Drupal\Core\Foo': class: 'Drupal\Core\Bar' 'Drupal\Core\Bar': class: 'Drupal\Core\Foo'
And did
$ vendor/bin/drush ev "dump(class_exists('Drupal\Core\Foo'));" [warning] Class "Drupal\Core\Foo" not found BackwardsCompatibilityClassLoader.php:24 [warning] Class "Drupal\Core\Bar" not found BackwardsCompatibilityClassLoader.php:24 ^ false
So this doesn't cause a problem which is good.
One thing I wonder is if we we should do some check in \Drupal\Core\DependencyInjection\Compiler\BackwardsCompatibilityClassLoaderPass::process() to ensure that all the keys in the class are Drupal\$module - because I'm not sure we should allow a module to set something on behalf of another module. I've tried to think through if that'd ever be okay and I don't really think it is. Putting back to needs review to get others thoughts.
- π¬π§United Kingdom catch
because I'm not sure we should allow a module to set something on behalf of another module. I've tried to think through if that'd ever be okay and I don't really think it is.
I think it would be very inadvisable, but I don't think we should prevent it for a couple of reasons:
1. Nothing stops someone registering an early request event listener and calling class_alias()
2. There could be a legitimate use case for moving a class from one module to another - say a sub-module that provides a trait or base class, and then it's is needed in the main module after all. You could argue the declaration should happen in the sub-module that the class is being moved from, but it seems plausible enough I don't think it's worth adding the validation to prevent people using it for something like that.
- π«π·France andypost
curious how this will work without server restart, opcache clean-up at least
- π¬π§United Kingdom alexpott πͺπΊπ
@catch thanks for the answer. Setting back to RTBC as my question is answered.
@andypost good point. We can address this we need to change how we set the classloader apcu prefix. At the moment we do
// Add the APCu prefix to use to cache found/not-found classes. if (Settings::get('class_loader_auto_detect', TRUE) && method_exists($this->classLoader, 'setApcuPrefix')) { // Vary the APCu key by which modules are installed to allow // class_exists() checks to determine functionality. $id = 'class_loader:' . crc32(implode(':', array_keys($this->container->getParameter('container.modules')))); $prefix = Settings::getApcuPrefix($id, $this->root); $this->classLoader->setApcuPrefix($prefix); }
This should change to:
$moved_classes = $this->container->getParameter('moved_classes') ?? []; // Add the APCu prefix to use to cache found/not-found classes. if (Settings::get('class_loader_auto_detect', TRUE) && method_exists($this->classLoader, 'setApcuPrefix')) { // Vary the APCu key by which modules are installed to allow // class_exists() checks to determine functionality. $id = 'class_loader:' . hash('xxh3', implode(':', array_merge( array_keys($this->container->getParameter('container.modules')), array_keys($moved_classes) ))); $prefix = Settings::getApcuPrefix($id, $this->root); $this->classLoader->setApcuPrefix($prefix); } if (!empty($moved_classes)) { $bc_class_loader = new BackwardsCompatibilityClassLoader($this->container->getParameter('moved_classes')); spl_autoload_register([$bc_class_loader, 'loadClass'], TRUE, FALSE); }
- π¬π§United Kingdom catch
I don't think #29 is right. This doesn't affect how the current classloader works at all, it only adds a fallback classloader with no caching at all. There is no way to override classes that actually exist on the filesystem using this, and this is by design (otherwise it would have to run first which would add at least some overhead).
- π¬π§United Kingdom alexpott πͺπΊπ
@catch if we don't alter the acpu prefix key then we'll have problems if you have an apcu optimised classloader enabled. That caches missing classes.
Consider the following flow...
- Move a class.
- Load code on webserver that tries to use the original class - breaks because original class is missing
- Add parameter to module to use the alias and rebuild the container
- Try again on server... won't work because the missing class is cached in APCu.
- π¬π§United Kingdom catch
This classloader only runs when all other classloaders return false. If the false is cached in apcu, what is the difference?
- π¬π§United Kingdom catch
Did the following test.
Renamed DynamicPageCacheSubscriber to DynamicPageCacheSubscriber2
Refreshed the page:
Error: Class "Drupal\dynamic_page_cache\EventSubscriber\DynamicPageCacheSubscriber" not found in Drupal\Component\DependencyInjection\Container->createService() (line 259 of /var/www/html/ 57 02/Mar 09:12 php Warning Warning: include(): Failed opening '/var/www/html/core/modules/dynamic_page_cache/src/EventSubscriber/DynamicPageCacheSubscriber.php' for inclusion (include_path='/var/www/html/vendor/pear 56 02/Mar 09:12 php Warning Warning: include(/var/www/html/core/modules/dynamic_page_cache/src/EventSubscriber/DynamicPageCacheSubscriber.php): Failed to open stream: No such file or directory in include() (line 576
Added this:
diff --git a/core/core.services.yml b/core/core.services.yml index bb73e29d505..8966901b49f 100644 --- a/core/core.services.yml +++ b/core/core.services.yml @@ -12,6 +12,8 @@ parameters: core.moved_classes: 'Drupal\Core\StringTranslation\TranslationWrapper': class: 'Drupal\Core\StringTranslation\TranslatableMarkup' + 'Drupal\dynamic_page_cache\EventSubscriber\DynamicPageCacheSubscriber': + class: 'Drupal\dynamic_page_cache\EventSubscriber\DynamicPageCacheSubscriber2'
Rebuilt the container (so that the moved_classes change was picked up), refreshed the page. No errors. The service definition in dynamic_page_cache_subscriber references the old classname still.
- π¬π§United Kingdom alexpott πͺπΊπ
@catch did you do
composer dump-autoload --apcu
? And have apcu enabled?If you that... then add
new \Drupal\Core\Foooo();
to index.php - I did this before the call to send the response. Then fetch the front page. This caches the missing class. Then add'Drupal\Core\Fooo': class: 'Drupal\Core\StringTranslation\TranslatableMarkup'
to core.moved_classes.
No amount of drush cr or webserver restarting can fix this afaics because of the way that the composer class loader caches stuff in APCu. The only fix is to prepend the class loader. Which is a shame but there's no way around that.
You're right though that my hopes to fix this with changing the apcu cache key are futile... that's just not going to work.
- π¬π§United Kingdom alexpott πͺπΊπ
Interesting aside: I wonder if we can speed up tests by adding
--apcu-autoloader
to the composer install. new \Drupal\Core\Foooo();
'Drupal\Core\Fooo': class: 'Drupal\Core\StringTranslation\TranslatableMarkup'
The class names don't match here, 4 o's instead of 3. Is this exactly how it was tested?
- π¬π§United Kingdom alexpott πͺπΊπ
@godotislate is was tested badly! Great spot. This works with the APCu optimised classloader great. I tested using the correct classnames and it still calls into the BackwardsCompatibilityClassLoader even when the missing class is cached in APCu which is great.
-
alexpott β
committed 9244acff on 11.x
Issue #3502882 by catch, alexpott, godotislate: Add a classloader that...
-
alexpott β
committed 9244acff on 11.x
@godotislate is was tested badly! Great spot. This works with the APCu optimised classloader great.
Nice!
- π«π·France andypost
speed up tests by adding --apcu-autoloader to the composer install.
could use follow-up as core bet on composer's autoloader #3020296: Remove Symfony's classloader as it does not exist in Symfony 4 β
- π¬π§United Kingdom catch
Thank you! Next one up in the chain is π Add a fallback classloader that can handle missing traits Active .
Automatically closed - issue fixed for 2 weeks with no activity.
- π³πΏNew Zealand quietone
Corresponding changes are needed on the How to deprecate β page.