Add a classloader that can handle class moves

Created on 28 January 2025, 2 months ago

Problem/Motivation

Sometimes we want to move a class from one place to a different place. An example is in πŸ“Œ Remove non-formatter plugins out of the file field formatter directory Active .

There are two solutions we've used in the past:

1. Leave the old class in place, usually extending the new class, add @deprecated and trigger a deprecation.

2. Add a class_alias - this has to be manually specified in boostrap.inc and can never be removed.

I think we might be able to handle both deprecation and aliasing in a classloader.

Steps to reproduce

Proposed resolution

Something like:

1. Create a new classloader

2. This classloader allows a $movedClasses array to be populated via a setter.

3. The moved classes array is keyed by the moved class, the value is an array of the new classname, the deprecated from version, the removed version (latter two could be optional).

4. in DrupalKernel, register this as an extra classloader, it should run after the composer classloader so it has zero runtime overhead.

5. When the composer classloader can't find a class, PHP falls back to our classloader. Our classloader checks the $movedClasses property, if the class is in there, it calls class_alias($old_class, $new_class, TRUE);

If this works, we could then look at populating the moved classes array, potentially from a container parameter so that it's possible for core and contrib modules to add to it.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

πŸ“Œ Task
Status

Active

Version

11.0 πŸ”₯

Component

base system

Created by

πŸ‡¬πŸ‡§United Kingdom catch

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @catch
  • πŸ‡¬πŸ‡§United Kingdom catch
  • πŸ‡¬πŸ‡§United Kingdom 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.

  • Merge request !11037Resolve #3502882 "Bc classloader" β†’ (Open) created by catch
  • πŸ‡¬πŸ‡§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.

  • One comment about the deprecation message 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.

  • Pipeline finished with Failed
    2 months ago
    Total: 42584s
    #409743
  • πŸ‡¬πŸ‡§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 alexpott πŸ‡ͺπŸ‡ΊπŸŒ

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

  • πŸ‡¬πŸ‡§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.

  • πŸ‡¬πŸ‡§United Kingdom catch
  • 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...

    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.
  • πŸ‡¬πŸ‡§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.

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

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

    • alexpott β†’ committed 9244acff on 11.x
      Issue #3502882 by catch, alexpott, godotislate: Add a classloader that...
  • @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.

Production build 0.71.5 2024