Add a classloader that can handle class moves

One comment about the deprecation message format.

This is great!

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

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

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.

One comment since it looks like the change_record 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.

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.

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

@catch thanks for the answer.

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

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

@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

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

Seems like a good documentation

New docs look good - thanks

@godotislate, thanks for updating the docs. I trimmed it down to be just a 'how to' and corrected the change_record to not point to an issue.