Add a classloader that can handle class moves

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.

I think I figured out the bits in #4, see the MR.

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.

One comment about the deprecation message format.

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.

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.

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.

This is great!

I think the answer is no, but is there anyway this can allow the copied classes to add return types too?

I think adding return types would be the same as if we added return types without moving the class.

Not keen on the fact we can't remove container parameters,

Doesn't $containerBuilder->getParameterBag()->remove() allow you do to that?

If we do #14 I would make the compiler pass \Symfony\Component\DependencyInjection\Compiler\PassConfig::TYPE_BEFORE_OPTIMIZATION so that if someone uses that namespaced parameter in a service it would error (I think).

$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.

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.

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.

Applied the two suggestions, both of which made sense to me.

lgtm

Read through the CR, it looks clear to me too!

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.

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.

curious how this will work without server restart, opcache clean-up at least

@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);
    }

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).

@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...

1. Move a class.
2. Load code on webserver that tries to use the original class - breaks because original class is missing
3. Add parameter to module to use the alias and rebuild the container
4. Try again on server... won't work because the missing class is cached in APCu.

This classloader only runs when all other classloaders return false. If the false is cached in apcu, what is the difference?

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.

@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.

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?

@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.

Committed 9244acf and pushed to 11.x. Thanks!

@godotislate is was tested badly! Great spot. This works with the APCu optimised classloader great.

Nice!

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 →

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.

Corresponding changes are needed on the How to deprecate → page.

How to deprecated updated:
https://www.drupal.org/about/core/policies/core-change-policies/how-to-d... →

Not really sure why the anchor has a leading "s-".