Remove drupal_classloader()

Added a related issue, which uses a different approach.

We don't support 5.3 for drupal 8, fwiw

1. +++ b/core/includes/bootstrap.inc
   @@ -1399,50 +1400,6 @@ function _current_path($path = NULL) {
   -function drupal_classloader($class_loader = NULL) {
   ...
   -  static $loader;
   

   Removing this static is welcome.

   At the same time, this construct did ensure that the class loader is only constructed once and not reconstructed from scratch more than once in a single process/request.

   However, given that this patch touches all instances, it looks like that situation did only occur in the setUp() of test base classes.

   Theoretically, constructing a new class loader for each test (method) is appropriate — however, a class loader is a native global state construct of PHP core.

   We need to ensure that (1) we're not stacking a dozen of class loaders on top of each other, and (2) due to how Simpletest works, we theoretically need to restore the original class loader (of the parent site/test runner) after tearing down a test when restoring the original environment, since the test might have manipulated the class loader.

2. +++ b/core/includes/install.core.inc
   @@ -358,7 +358,8 @@ function install_begin_request(&$install_state) {
   +  $classloader = require __DIR__ . '/../vendor/autoload.php';
   

   This appears to be the only instance outside of tests that reconstructs a new classloader in the same process.

   We need to assign and inject the $loader from /core/install.php into install_drupal() and pass it forward into install_begin_request().

   Don't worry, we just need to change the signature of these two functions. I'd even suggest to inject it as the first argument, shifting the $settings / $install_state arguments by one position.

3. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -410,6 +405,21 @@ public function getContainer() {
   +  public function getClassloader() {
   ...
   +  public function setClassloader($classloader) {
   

   These two methods are not necessary and should be removed.

   The class loader is exposed as synthetic service 'class_loader' on the service container; see DrupalKernel::attachSynthetic().

4. +++ b/index.php
   @@ -18,6 +19,20 @@
   +  // $apc_loader = new ApcClassLoader('drupal.' . Settings::getHashSalt(), $autoloader);
   

   There's not really a reason for why we cannot do this from within DrupalKernel, instead of having to hack index.php.

   Ideally as a separate method, so that alternative kernels are able to override it.

   Alternatively, we could change the workflow and move this code directly into settings.php (commented out by default). To do so, we just need to forward the $classloader from DrupalKernel into Settings::initialize(). Just passing the argument is sufficient; the variable will be available in the local scope of settings.php.

   I'd be in favor of the latter approach, because it removes the indirection of the current settings variables, doesn't require to hack any front controllers, and allows you to use a different classloader/decorator.

re: 1) Interesting. Sorry, in that case, 2) is moot. And 1) can be ignored, since the behavior is effectively the same; the object is just retrieved from a different static. The additional requires are fine and not measurable (only a conditional file inclusion of a possibly non-existing file would present an issue).

+++ b/index.php
@@ -18,6 +19,20 @@
+  // By default, use the ClassLoader which is best for development, as it does
+  // not break when code is moved on the file system. However, as it is slow,
+  // the APC class loader can be used in production. To use the APC class loader
+  // simply uncomment the lines below. Note that ApcClassLoader decorates
+  // ClassLoader and only provides the findFile() method, but none of the others.
+  // The actual registry is still in ClassLoader.
+
+  // require_once __DIR__ . '/core/vendor/symfony/class-loader/Symfony/Component/ClassLoader/ApcClassLoader.php';
+  // $apc_loader = new ApcClassLoader('drupal.' . Settings::getHashSalt(), $autoloader);
+  // $autoloader->unregister();
+  // $apc_loader->register();
+  // $kernel->setClassloader($apc_loader);

Unlike Symfony, Drupal generally views index.php as its own file, not a particular app instance's file. That is, this is technically telling people to hack core to enable to APC class loader.

That said, I'm not adverse to it. I think in the circumstances it's probably for the best, but we should be conscious of what we're doing here.

I haven't dug into the rest of the implementation details so I won't comment on them.

leaving DrupalKernel::setClassLoader() in place

+++ b/core/lib/Drupal/Core/DrupalKernel.php
+    Settings::initialize($site_path, $class_loader);
+    $kernel->setClassLoader($class_loader);

Ah. Oh. Interesting challenge. ;)

That said, your patch seemingly implements the possible solution of making Settings::initialize() accept &$class_loader by reference, so the custom code in settings.php is able to replace the referenced variable itself (instead of the object reference) already.

That looks fully correct to me. What else needs to be refactored to fully propagate the variable? Can't be much, so I think we should.

what [do] you think about the settings.php solution. It's a little bit more work than the $class_loader = 'apc' we had before, but it's a lot more flexible, and I don't think it's that bad.

That was exactly my train of thought as well. The architectural concept of a string identifier indirection is artificially limited to a hard-coded set of string identifiers understood by Drupal core. By moving the code directly into settings.php, people can do whatever they want.

However, can we move the example code from within the PHPDoc comment into "more actual" PHP code that is commented out via # or /* ... */, please?

re: install.core.inc changes: Let's revert those for now, as they are not necessary.

Indeed, the initial installer invocation + front controller is pretty much one of the last major puzzle pieces of #1530756: [meta] Use a proper kernel for the installer

Using settings seems to be good enough approach. Still actual sites could still better control their front controller, but including this one
exception for logic inside settings.php feels okay.

1. +++ b/core/modules/system/src/Tests/DrupalKernel/DrupalKernelTest.php
   @@ -59,7 +52,8 @@ protected function setUp() {
   +    $class_loader = require __DIR__ . '/../../../../../vendor/autoload.php';
   +    $kernel = DrupalKernel::createFromRequest($request, $class_loader, 'testing');
   
   +++ b/core/modules/system/src/Tests/System/IgnoreReplicaSubscriberTest.php
   @@ -32,7 +32,8 @@ function testSystemInitIgnoresSecondaries() {
   +    $class_loader = require __DIR__ . '/../../../../../vendor/autoload.php';
   

   Did we considered to just use DRUPAL_ROOT instead?

2. +++ b/sites/default/default.settings.php
   @@ -377,11 +377,26 @@
   +
   +// Do not do anything if no hash salt is set.
   +if ($settings['hash_salt']) {
   +  // Wrap $class_loader with Symfony's APC class loader.
   +  $apc_loader = new ApcClassLoader('drupal.' . $settings['hash_salt'], $class_loader);
   +  $class_loader->unregister();
   +  $apc_loader->register();
   +  // Replace the $class_loader variable.
   +  $class_loader = $apc_loader;
   +}
   

   Just ensure that we don't accidentally enable APC support by default.

   Now that the hash_salt could be manually set here, we could get rid of having it as part of settings but rather be usual configuration.
   Instead use something like:

   // Change this to a site specific value, in case you have multiple sites running on the webserver.
   $apc_hash = 'apc_hash';
   $apc_loader = ...
   

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -220,20 +217,38 @@ public static function createFromRequest(Request $request, ClassLoader $class_lo
+  public function initializeSettings(Request $request) {

Is there any reason to make this methdo public? It seems rather pointless given that you would need createFromRequest anyway. At least there is no other external call to this function.

any thoughts on testing this?

The initial bootstrap can only be tested in a PHPUnit test that runs in a separate and cleanly isolated process, and which uses the virtual site environment infrastructure for booting a kernel from #2304461: KernelTestBaseTNG™ — otherwise, all affected classes are loaded already and the class loader isn't triggered.

1. +  public function initializeSettings(Request $request) {
   +    // This should not be called multiple times.
   +    if (isset($this->sitePath)) {
   +      throw new \BadMethodCallException('Do not initialize settings multiple times.');
   +    }
   

   I agree with @dawehner, let's make this method protected.

   Also, the new exception is a behavior change, doesn't appear to be relevant or related to this issue.

2. +++ b/core/modules/system/src/Tests/DrupalKernel/DrupalKernelTest.php
   +    $class_loader = require __DIR__ . '/../../../../../vendor/autoload.php';
   

   In the affected test classes, we can use DRUPAL_ROOT. In all other non-test code that isn't deeply nested, I'd prefer to keep the relative include as in the patch.

Happy to help out.

I'd recommend to move the 'hash_salt' (APCu prefix) discussion into a separate issue, since that is an unrelated change in behavior, and needs more discussion, because we still need a unique value, so as to not clash with other Drupal installations that may run on the same machine/process/memory.

+++ b/core/includes/bootstrap.inc
@@ -1451,7 +1408,7 @@ function drupal_classloader($class_loader = NULL) {
 function drupal_classloader_register($name, $path) {
-  $loader = drupal_classloader();
+  $loader = \Drupal::service('class_loader');
   $loader->addPsr4('Drupal\\' . $name . '\\', DRUPAL_ROOT . '/' . $path . '/src');
 }

drupal_classloader_register is indeed just used in system_list() which requires a properly booted classloader already.

I read the issue + the patch, but despite the thorough comments this is totally going to be a "close my eyes and commit it" type patch. ;) Since tests are passing I'm assuming either a) things aren't horribly broken, or b) since this touches the very guts of Drupal if anything IS broken it'll get found out pretty quick. ;)

I do have one concern before doing that, though, which is that catch was the creator of #2023325: Add interface for classloader so it can be cleanly swapped and since these two issues overlap (and since he's had a lot of very strong opinions about class loading in general), ideally he would be the one to review this. The problem with that is he's not back until next week, which would mean this sits here for a few days.

Tobias, since this is primarily blocking your "composer all the things" work (which dawehner showed a preview of in IRC and it looks awesome!), how urgent do you need this in this week? It's only a normal, so can it wait a few days for catch?

Ok, great. Thanks so much for your understanding. Assigning to catch for a look-see. Sounds like a cool project though!

OK read through this and discussed briefly with tstoeckler . This removes the bug part of #2023325: Add interface for classloader so it can be cleanly swapped (so I've retitled it and changed it to a task) - which will allow for experimentation with more performant class loaders.

Patch itself looks fine, I'm not clear how useful the "hack index.php to change the location of autoload.php" use case will be if we stop checking in composer stuff to the repo, but it's not going to make that worse anyway.

Committed/pushed to 8.0.x, thanks!

The changes to DrupalKernel need to be replicated in TestKernel: #2367557: Update TestKernel to match class loader changes in Drupal Kernel