Remove Symfony's classloader as it does not exist in Symfony 4

@mglaman create an issue recently about how our classloader is really unperformant and just switching to the default one provided by composer is an improvement - I'll try to dig that out.

I think he just mentioned that in slack, not sure if there's an issue, but I posted my thoughts in #1818628: Use Composer's optimized ClassLoader for Core/Component classes related that, I've also disabled the symfony apcu classloader and instead I'm now using the optimized composer including enabled apcu support (for the module-provided classes).

There are two patches for using Composer's APCu caching in #2937534: Bring the Symfony\Component\ClassLoader\ApcClassLoader into core and undeprecate

Updated title and issue summary heavily based on #3 from @catch. Hope that sounds right?

Also reparenting since this does not block making Drupal Symfony 4 compatible.

Fixing the parent one more time :)

Should belong in 9.0.x.

If I understand this correctly, there are two main items that need addressed for D9:

1. A replacement for the deprecated APCu class loader removed in Symfony 4. It appears to me the more straightforward/low friction/best reviewed of the two right now is #2704571: Add an APCu classloader with a single entry. I think this issue has a slight edge as well in so far as it requires the least adjustment to packaging and autoloading; it's basically a drop-in replacement.
2. For non-APCu conditions: The Composer classloader is used. You can optionally optimize it at build time if you are not simply installing from the tarball.
3. I believe we also need to address/remove the deprecated WinCacheClassLoader and XCacheClassLoader, both of which have deprecation notices suppressed right now. See #2938369: The Symfony\Component\ClassLoader\WinCacheClassLoader class is deprecated. As best I can tell, APCu is the recommended replacement for both, but we do need to remove these references from D9. This should, I believe, remove the last dependencies on symfony/class-loader if we use the APCu replacement component linked above.

After the fun found in #3104015-65: Replace ZendFramework/* dependencies with their Laminas equivalents I think we should consider completely removing our custom autoloading replacement code and allow people to use the composer optimised autoloader. The way in which we swap out the autoloader is very surprising for a project built using composer. Now that composer works on the tarball we could detect that you're not using an optimised autoloader and have APCu having available on the system status report page and then recommend that you rebuild your autoloader using composer. We could also build the tarball with the optimised autoloader and see what happens when APCu is not available. If it works then we could consider building it that way by default.

We've already switched all our projects to the default optimized apcu composer classloader and explicitly disable the logic we have in core and that results in performance improvements compared to HEAD.

So I'm not sure we really need to have a single-entry replacement for that that we maintain ourself. Like @alexpott said, what we do is quite weird.

> We could also build the tarball with the optimised autoloader and see what happens when APCu is not available. If it works then we could consider building it that way by default.

Last I checked that works fine, there's still a runtime check to see if it's actually enabled. So +1 to that.

What I'm wondering is what will happen with sites that have the classloader explicitly in settings.php, because if we remove this, they will just fail hard on that.

Very cool and thank you both for the insightful notes regarding whether this is much of a to-do at all. Given the ability of Composer to provide an optimized autoloader we do not have to maintain in-tree, I think this means we can remove the need/ability to override the class loader in settings.php? Per Berdir's question, I think if the autoloader-switcheroo code is removed, the answer to what will happen if a class loader is specified is "nothing." But that also means we can remove that parameter from the settings initialization static method and all callers, since it's no longer used. I *think* we can just remove this without a deprecation notice and make it a Change Record, but I'm not quite sure since this seems to be an exception to the rule regarding deprecations and changes in 9.0.0?

If this is the direction to go then the only other to-do would be to add a status report item to nudge site owners to optimize the autoloader when APCu is detected.

Attaching a patch to see what breaks...

1. +++ b/core/includes/install.core.inc
   +++ b/core/includes/install.core.inc
   @@ -335,7 +335,7 @@ function install_begin_request($class_loader, &$install_state) {
   
   @@ -335,7 +335,7 @@ function install_begin_request($class_loader, &$install_state) {
      }
    
      $site_path = empty($install_state['site_path']) ? DrupalKernel::findSitePath($request, FALSE) : $install_state['site_path'];
   -  Settings::initialize(dirname(dirname(__DIR__)), $site_path, $class_loader);
   +  Settings::initialize(dirname(dirname(__DIR__)), $site_path);
    
      // Ensure that procedural dependencies are loaded as early as possible,
   

   not sure if we should remove the ability to set a different classloader. If someone for some weird reason still *does* want to use ApcClassLoader then they can still require that or another package and put code in settings.php.

2. +++ b/sites/default/default.settings.php
   @@ -421,38 +421,6 @@
   -/*
   -if ($settings['hash_salt']) {
   -  $prefix = 'drupal.' . hash('sha256', 'drupal.' . $settings['hash_salt']);
   -  $apc_loader = new \Symfony\Component\ClassLoader\ApcClassLoader($prefix, $class_loader);
   -  unset($prefix);
   -  $class_loader->unregister();
   -  $apc_loader->register();
   -  $class_loader = $apc_loader;
   -}
   -*/
   

   What I meant is that if someone uncommented this piece then they will get a fatal error on 9.0, before we can check/handle that in any way really.

   We could *maybe* do some class_alias trickery, but we'd need to always do it, as we can't detect if sites are going to do that or not.

As Berdir mentioned in #18, the composer classloader has a runtime check that prevents any apcu functions being called if the extension is not available and enabled:

    public function setApcuPrefix($apcuPrefix)
    {
        $this->apcuPrefix = function_exists('apcu_fetch') && filter_var(ini_get('apc.enabled'), FILTER_VALIDATE_BOOLEAN) ? $apcuPrefix : null;
    }

A problem with packaging the tarball with apcu caching enabled though, is that if multiple sites are on the same server they will share the same cache prefix and would likely start loading each other's files.

One of the patches in #2937534: Bring the Symfony\Component\ClassLoader\ApcClassLoader into core and undeprecate just enables composer's APCu caching with Drupal's prefix that includes the site's root directory in a hash:

       if (function_exists('apcu_fetch')) {
-        $loader = new ApcClassLoader($prefix, $this->classLoader);
+        // Prefer Composer's APCu caching introduced in 1.3.0.
+        if (method_exists($this->classLoader, 'setApcuPrefix')) {
+          $this->classLoader->setApcuPrefix($prefix);
+        }
+        else {
+          $loader = new ApcClassLoader($prefix, $this->classLoader);
+        }
       }

(it's probably safe to assume by now that everyone is using at least Composer 1.3 though)

Berdir - Point taken on #20.1, I am not feeling strongly any which way about the ability to override the class loader at runtime and if we keep that feature in, that does also have the benefit of reducing the entropy of the patch...

For #20.2, the error would be something like, class ApcClassLoader not found, which yes would throw early but A) anyone overriding their class loader is likely an advanced user already and B) this is not unlike other errors a user upgrading from 8.x to 9.0 who missed deprecated->removed functions would get if they did not analyze their code as part of the upgrade?

Re: #21, excellent point regarding the cache prefix; I don't think it's out of the question to reduce the current classloader-swapping logic to just the section that conditionally updates the prefix?

I'll try to roll an updated patch per the above later today.

Here's a patch that completely removes our autoloader in favour of adding composer's APC optimisation by default which is very similar to Symfony 3's Symfony\Component\ClassLoader\ApcClassLoader.

I've left the ability to change the classloader in Settings.php - I think we could deprecate that in Drupal 10. I'm not sure that custom autoloaders are worth supporting.

I've tested this patch with both APCu enabled and disabled and it works fine.

If we choose to go this way I think we need a decent CR and release note about adding the config to existing sites. We could try to detect where APCu optimisations are enabled in autoloader and put something on the system status report.

Most of #19 didn't apply anymore so interdiff was a bit hard - plus this approach is slightly smaller.

Ah looking at the way \Composer\Autoload\ClassLoader works the moment we set the prefix the classloader will start using APCu (if it is enabled). Therefore we don't actually need to add the autoloader configuration to composer.json.

Also we're not currently recommending APCu on the system status report. So imo that could be done in a follow-up because you'd currently need APCu enabled to take advantage of the code in D8 anyway. So setting the prefix gives us the same advantages as D8 but with much less complexity.

One thing this brings up is that potentially people want to disable our optimisation so we should leave that in.

Patch improves the tests of $settings['class_loader_auto_detect'] = FALSE;

So we're running into a problem that Composer's classloader caches misses because it doesn't expect the codebase to be changing at runtime. But we do that on module install and every request in Drupal so we need to overcome that. I have an idea.

So we can move the prefix settings to when we have a module list and use that to ensure that when modules change we change the classlist apcu cache key because at that point missing classes might no longer be missing.

I've also stepped through the early boot environment to ensure we're using an authorative classmap for all the classes that are involved before we set the prefix - thereby avoiding apcu lookup and file existence checks.

+++ b/composer.lock
@@ -660,11 +660,29 @@
+                    "lib/Drupal/Core/Database/Driver/mysql/Connection.php",
+                    "lib/Drupal/Core/Database/Driver/pgsql/Connection.php",
+                    "lib/Drupal/Core/Database/Driver/sqlite/Connection.php",

How are alternative DB drivers affected by this?

This fixes the test of DrupalKernel. Not sure what's going on with \Drupal\Tests\help_topics\Functional\HelpTopicSearchTest::testUninstall() yet. It does not fail locally :(

@amateescu they are not "affected" we look in the db to load the container - we can't set the APCu prefix until the container is loaded because we need the module list. Adding them here adds them to the authoritative class map so we don't do any unnecessary searching of the file system for the classes. We'd find them but adding them here means that we skip APCu and file existence checking on them at runtime. Their existence is checked when composer builds the classmap.

Interesting. So there's an undeclared dependency in the hep_topics_test module on the help_topics module. This patch uncovers that because we no longer are able to fetch the class from APCu because the module is uninstalled. This is a good thing!

Updated the issue summary.

I'm not sure what to do about #1818628: Use Composer's optimized ClassLoader for Core/Component classes and #2704571: Add an APCu classloader with a single entry - both of them are affected by this change and become somewhat less important but they might have avenues worth exploring. That said I think we should try and implement things like #2704571: Add an APCu classloader with a single entry upstream in composer land.

The tests time for #34 took 54 mins and the last run on HEAD took 1hr and 1 min. So there's some evidence that the latest patch is quicker. But it is probably within the natural fluctuation of DrupalCI. The good thing is that APCu on the testbots has not exploded.

This looks great to me.

I think the only thing left here is to remove "symfony/class-loader": "~3.4.0", from core/composer.json and \Drupal\Core\Composer\Composer::$packageToCleanup? Or would we do that elsewhere?

I manually verified that it works with that removed, I also check that it works with apcu enabled and not,

I also did some profiling with blackfire (xdebug disabled), not sure how much to trust these numbers. Comparing time spent on spl_autoload_call(), with composer install --no-dev -o, lots of modules, disabled render caches, uid 1.

HEAD with apcu: 739 calls, 11.6ms
Patch with apcu: 738 calls, 11.5ms.

HEAD without apcu: 734 calls, 14.7ms
Patch without apcu: 734 calls, 14.9ms

Fewer calls are probably because we don't load the apcu cache stuff, time difference is small enough to be ignored I think.

Looks great clean-up!

Removed the dependency on and references to symfony/class-loader

Added a change record for WinCache - https://www.drupal.org/node/3116297

This looks great!

Removing the Needs change record tag.

Oops. Looks like I accidentally changed the status.

+++ b/composer.lock
@@ -487,7 +487,6 @@
                 "symfony-cmf/routing": "^2.1",

+++ b/sites/default/default.settings.php
@@ -424,35 +424,13 @@
  * Class Loader.
  *
- * If the APC extension is detected, the Symfony APC class loader is used for
- * performance reasons. Detection can be prevented by setting
- * class_loader_auto_detect to false, as in the example below.
+ * If the APCu extension is detected, the classloader will be optimised to use
+ * it. Set to FALSE to disable this.
+ *
+ * @see https://getcomposer.org/doc/articles/autoloader-optimization.md
  */
 # $settings['class_loader_auto_detect'] = FALSE;
 

We're kind of changing the behaviour of this setting.

On our sites, we have it set to FALSE and rely on running composer install -o --apcu-autoloader.

But the new logic means that we no longer need to disable this and instead can just rely on the new default behavior.

So maybe we should do a change record for this. We could possibly also actually rename this setting to a more explicit class_loader_use_apcu or so?

@Berdir / #45 I left it as the same to have minimal BC impact. Your sites are still going to behave as they do now. Preserving the behaviour of that setting meaning that we don’t change the classloader seems important to me. Because it means if you disabled the APCu classloader by setting $settings['class_loader_auto_detect'] to false and making no other changes then the result is the same. Say for example you want to use APCu for other caches but not classloading (which imo might be reasonable if you have a super fast filesystem).

@Berdir but I agree a change record to announce that we're always using the Composer class loader and never the Symfony one is a good idea and that should detail that if you were setting this in the past to use only the Composer class loader then this is no longer necessary.

Add another CR - https://www.drupal.org/node/3116384

I think this is ready and I think we have a nice solution now with less complexity, fewer dependencies and comparable performance as far as I could profile (#39).

Note: I know that @catch will want to have a look at this before it gets committed, but this is an issue for him anyway.

@catch there is a comment already...

    // Check for our packages, and then optimize them if they're present.

This is adding to the list of optimisations that exist already. It's not the best comment though.

This is breaking HEAD tests on PHP 7.4 because of swapped arguments of implode in DrupalKernel

Thanks @mondrake - hotfixed.

My understanding after reading this issue and the associated change record (https://www.drupal.org/node/3116384) is that if you want to use the APCu autoloader in D9, you must still pass the --apcu-autoloader flag to Composer. Is that correct?

The wording of the change record is ambiguous: it's not clear if it's just $settings['class_loader_auto_detect'] = FALSE; that's obsolete, or both the setting and the Composer flag.