Use the Composer autoloader to make everything simpler

+++ b/core/includes/bootstrap.inc
@@ -2921,56 +2915,26 @@ function arg($index = NULL, $path = NULL) {
+    // Retrieve the Composer ClassLoader for loading classes.
+    $loader = include DRUPAL_ROOT . '/core/vendor/autoload.php';

A step farther: This line can go at the top of index.php/update.php/install.php.

Fixed the bugs in the previous patch.

What this does: Switch to the composer autoloader, because there's no legit reason to not use it.

1) It can be activated before anything other code runs, eliminating many race conditions.
2) We can't rely on Symfony ClassLoader's APC support, because with PHP 5.5 its future is uncertain.
3) With one additional toggle, we can generate a classmap for ALL core and vendor classes. That, with an opcode cache, is going to be the fastest option available. Maybe we do that by default, maybe we leave it to site admins who know what they're doing, I don't care.

Bot?

Fixing run-tests.sh and simpletest.

Note that this patch is big, but most of it is jitter in composer's generated files. Ignore those. We'll rip them out eventually.

related.. #2003800: Move drupal_check_memory_limit() and parse_size() functionality to components introduces a Bootstrap class.maybe the handleRequest method should go there

No, because Bootstrap is in Component, and handleRequest() is not Component-compatible. It's too Drupalish.

yeah, true that, i missed that one, nvm me

+++ b/core/modules/system/lib/Drupal/system/Tests/Module/ClassLoaderTest.php
@@ -39,8 +39,8 @@ function testClassLoading() {
     // Check twice to test an unprimed and primed system_list() cache.
     for ($i=0; $i<2; $i++) {
       $this->drupalGet('module-test/class-loading');
-      $this->assertText($this->expected, 'Autoloader loads classes from an enabled module.');
     }
+    $this->assertText($this->expected, 'Autoloader loads classes from an enabled module.');

Are you sure about this change? This means we only check the second time through, doesn't it?

Note: This DOES remove support for the APC flag on the Symfony classloader. I think that's OK. If we find that we should in some cases enable an APC class loader in addition to or instead of the classmap option in Composer, we should submit that as an option upstream for Composer rather than trying to solve it ourselves.

I think this should move forward. We can then compare benchmarks of classloader vs. not, and experiment with APC as well.

Assigning to @catch since he some performance concerns.

This isn't remotely RTBC.

On the classmap, I'd like to see the following answered:

How many classes will be in it that we never, ever use? I'm expecting thousands - we already have 353 in the autoload_classmap.php that ships with core currently for PHPUnit alone. Then there's entire libraries that are in Composer that we don't use, like Assetic (which might be removed if we're still not using it by release, but still). And libraries we only use one or two classes from like Doctrine Common. Drupal modules also by design ship with a tonne of classes that never get used as well - tests, test modules, plugins like field formatters or views arguments that might or might not get configured.

It's impossible for Composer to know any of this when it generates the classmap, and it's a much bigger problem for Drupal than most users of Composer.

Seldaek mentioned on https://github.com/composer/composer/pull/811 that he did benchmarks with a couple thousand classes, there's already 5347 in core (more than half outside vendor), that's with most router conversions not done yet and no contrib modules so could easily get over 10,000. Any class_exists() checks for non-existent classes (which we used to do for bundles) aren't covered by a classmap either.

More fundamentally Composer doesn't know about Drupal classes in lib, or modules, so none of those classes would be in the classmap as it stands now. We could add a custom classmap generator (which would have to operate within a Drupal bootstrap to get the enabled modules list to set up the $dirs parameter correctly). Profiling would have found this.

You'd also need to be able to regenerate the classmap when modules are disabled or enabled to avoid falling back to file system checks (and on multiple web servers), that's not been discussed anywhere afaik. Some might be able to get that into a deployment step, but it's a lot more maintenance than being able to rely on something that's cached rather than statically generated.

The APC classloader handles this and already works. This patch is a regression until there's at least comparable performance to APC as well as the deployment issues understood and documented.

I also posted two possible approaches on #1241190: Possible approaches to bootstrap/front loading of classes which would be somewhere in between the APC classloader and statically generated classmap (i.e. build the classmap on runtime).

Postponing on #2023325: Add interface for classloader so it can be cleanly swapped.

If I understand this correctly, the net result is "use composer, but we can wrap it in a caching object provided by Symfony". Which seems like a perfectly good architecture to me, and best of both worlds.

I don't know why we'd bother making the wrapper swappable, though. It looks like we introduce more bootstrap-level functions to do so, which I'm not a big fan of.

Can we get some benchmarks on this? Opcode cache/no-cache, APC-caching on/off, classmap-autoloader/default-autoloader. (Skipping no-APC/APC-caching, since that's a nonsensical combination.)

There are different reasons why I doubt that Composer's autoload.php will get us very far.
However, I can imagine that it could be a useful intermediate step.

Issues:

1. Composer does not support PSR-4 (yet).
   This means, we have to wait, or we have to revert this stuff when going to PSR-4.
2. Its lookup algorithm is not the fastest we could get for Drupal (esp. for modules).
   Composer's loader has a smart predictor technique to avoid long loops: It groups registered namespaces by their first character.
   Unfortunately, in Drupal, most of these first characters are "D". So there are still those long loops which slow down the class loader.
   https://github.com/donquixote/autoload-bench/issues/1 reveals how we could improve this for Drupal.
   Sure, APC decorator and classmap help a lot. But we cannot always rely on that, esp. during development, and for modules.
3. Composer-generated class maps do not contain module-provided classes, because Composer does not know which of them are enabled.
4. It does not support class discovery (in the way we need, e.g. for AnnotatedClassDiscovery).
   This is quite a new thing - none of our previous class loaders did that. But it would be pretty convenient, and make the transition to PSR-4 for class discovery much easier.
5. There is nothing in autoload.php we could not do with a custom or 3rd party loader.
   The last missing piece is being discussed here: https://github.com/composer/composer/pull/2105
   When this is in, every custom class loader can grab the autoload information in vendor/composer/*, without actually requiring Composer's autoload.php.

The remaining argument in favor of Composer's autoload.php is that we have to write less code ourselves, and don't rely on additional 3rd party code. I do not deny this argument. We should always leave the door open to switch back to Composer's autoload.php, and we should stay as close as possible to Composer.

But, atm it could be preferable to go with a custom or 3rd party solution.

I am also not strictly advocating for Krautoload. Especially not in its current shape.
If we are getting creative, this stuff can either live in Core or in a 3rd party library. And some day it might even go back into Composer, so we can switch back to autoload.php.

Alternatively, if we really want to benefit from Composer's class loader, we should consider dumping dynamic autoload stuff into the code generation directory, based on which modules are enabled.

EDIT: This being said, it could be ok to go with Composer for now and discuss other solutions in a follow-up.

#29
Ok, I think you are right. We should use autoload.php for the time being.

Composer does not support PSR-4

We are discussing this on the Composer issue queue, here:
https://github.com/composer/composer/issues/1884 (you probably noticed)
-> let's wait for an experimental release, or for the vote to finish.

lookup algorithm is not the fastest we could get for Drupal

Discussion in Composer (probably wontfix): https://github.com/composer/composer/issues/2112
-> can be discussed in a new issue, after this one has finished.

Composer-generated class maps do not contain module-provided classes

-> Can be discussed in a future issue.

It does not support class discovery

I finally get the idea that it would be a mistake to let ClassLoader and NamespaceInspector (or whatever discovery abstraction we use) be the same object. It only complicates everything. So, point invalid.

So, let's go ahead!

HOWEVER:
This issue tries to solve two problems which are rather separate.
First, switching to autoload.php. Second, changing the way that decorators are configured.
Should we not do this piecemeal?

We tried to do it piecemeal in #17 and catch rejected it because of the loss of APC support. This is the MVP. We just need to get it benchmarked.

#17 killed APC, and I don't think that was necessary for a piecemeal patch.

> We just need to get it benchmarked.

Just saying..
With APC or ClassMap (if modules were part of it), one loader is usually as good as another.
And if not, this can be fixed in Composer.

Without APC or ClassMap, there are some subtle differences, but basically they all suck badly for Drupal with many modules enabled.
Either we live with that and tell people to use APC or Classmap, OR we convince Composer to do something smart (that might not pay off for projects other than Drupal) (see #30), OR we use our own custom loader.

Hi,
I'm a bit confused by the numbers.
Comparing "withpatch-noapc-optimized" and "withpatch-noapc".

In one you have a number format of "8.30", 25.00" etc, in the other you have numbers like "1043". I assume it is all milliseconds nevertheless.
I would expect that findFile() "Total Self Cost" is less in the optimized version. But why is php::PDOStatement->execute 20x slower in the non-optimized version? This should only count the "self" cost, so.. class loading should not be a part of it?

findFile() appears radically faster in the optimized version, but this surprises me as well.
In my book, --optimize will only generate a class map for core and vendor, not for modules.
If core namespaces are registered before modules (which they are), then the foreach() loop over all prefixes that begin with "D" should typically find them quite fast, without iterating over all module namespaces (in the no-classmap version).
On the other hand, module classes, which are not in the classmap, should still have the long foreach() loops with a lot of strpos() (in the classmap version).

EDIT:
The expanded findFile() revealing the number of strpos() is quite revealing. If you choose to do the benchmark again, it would be interesting to see that in all versions.

Updated issue summary.

I split the patch from #26 in two steps, to illustrate the "piecemeal" I was talking about in #30.
The two applied one after the other are exactly like #26.
I name the second one ".interdiff" only to avoid tests.
As you can see (re: #31), the first one does not kill APC.

Imo, about the modified decorator settings:
- They are not really related or interdependent with the switch to autoload.php. The same would work with the Symfony loader, or any other.
- They are not in the way of anything else atm - so, no rush.
- They might benefit from having a separate issue and discussion, separate change notice, etc.

I am personally undecided if I agree with the decorator stuff as it is proposed atm. I am not heavily opposing it, but I do have some thoughts about it, and I don't want to derail this issue for it.

Changing the return type spec in the docblock on drupal_classloader() to Composer\Autoload\ClassLoader.

#38: D8-composer-loader-piecemeal-2038135-38.patch queued for re-testing.

Need to unregister the ClassLoader and register the ApcClassLoader if you want to use APC
Turning on APC in the settings doesn't actually use APC

Duh.. ok. But probably easy enough to do.

Jamming both the decorator initialization and retrieval of the loader into drupal_classloader() feels like bloat since they do different things.

I agree. But this is what is already happening, so it is not necessarily our job to fix it in this issue.

or fix #26's classloader_wrapper function to use "apc" instead of "Symfony\ThingyThingy" decorator

Need to think about it.
I wanted to avoid starting to bikeshed the exact format of decorator configuration in this issue, and keep the git history nice. This was the motivation for the split-up.

We have to push forwards

Yeah. We should get this in soon, so we have a clear starting point for future work on the classloader.
If files like install.php and friends can include autoload.php instead of bootstrap.php + calling drupal_classloader(), this is a clear improvement. Any future changes will have to be evaluated against that.

I think I am going to upload different patches, including what you are proposing, so we can choose one and move forward.

I am adding the fixed piecemeal patch, the interdiff (that I would consider as a followup), and the "complete" one, to not be the killjoy.

However,

Either that or fix #26's classloader_wrapper function to use "apc" instead of "Symfony\ThingyThingy" decorator

I started that, and ran into so much to legitimately discuss about, that I really really think we should do this in another issue, and go "piecemeal" for this one.

Stuff we would have to discuss:

• Should we continue to allow settings like 'apc' (and add 'xcache' and 'wincache', and whatever Symfony has to offer) ?
  The idea here is that a user wants to specify the type of cache being used, but does not really care about the class. E.g. if Composer would introduce their own APC wrapper, then we could decide to use that instead of Symfony's, if the user wants "apc".
  We could still allow custom classes.
• Should the setting be renamed to "class_loader_wrapper_name" ? Or "class_loader_wrapper_class" ?
• Should we say "decorator" instead of "wrapper" ? This is what Symfony does.
• Should we catch the exception that is thrown by Symfony if e.g. APC is not available?
• Should we throw an exception if the wrapper class does not exist? Or if the wrapper class does not have the constructor format that we expect?
• Do we need a dedicated exception class?

Or, if we introduce the drupal_classloader_wrapper() function but leave 'apc' as the only option, then should we also keep the stupid argument name "$class_loader", or should we invent a new argument name "$wrapper_name", only to then switch it again to "$wrapper_class" ?

Imo, this issue is the wrong place for these questions, and the additional alternative-piecemeal patch would not help us at all.

to #44:
I also wonder if our function-with-static-variable and get/set duality is still a desirable pattern.
(and if not, then again, I would suggest to do this in a follow-up, and go piecemeal in this one)

Would it not be better to have something like

\Drupal::getClassLoader();
\Drupal::decorateClassLoader($decorator_class);
\Drupal::getClassLoaderDecorator();

Or, we could have a dedicated class for this.

\Drupal\Core\Autoload::setClassLoader($loader);
\Drupal\Core\Autoload::setDecoratorClass($decorator_class);
\Drupal\Core\Autoload::getDecorator();

Cool!

Follow-ups:

1. Get a PSR-4 loader for modules.
   Will Composer have this soon enough? Or do we need to do something ourselves?
2. Improve the way to configure class loader decorators.
   Possible changes to discuss there:
   - Refactor drupal_classloader(), move decorator stuff into \Drupal::something().
   - Support xcache and wincache next to apc.
   - Support custom wrapper classes.
   - Change settings schema
3. Enable class map generation for core + vendor.
   #1818628: Use Composer's optimized ClassLoader for Core/Component classes
4. Class map generation for enabled modules in e.g. sites/default/vendor/composer.
5. Explore Drupal-specific performance improvements to regular PSR-4 / PSR-0 lookups, as in
   https://github.com/donquixote/autoload-bench/issues/1
   This may result in a custom autoload.php for Drupal, to be used instead of the one generated by Composer.

I think the first one is really the most important atm. PSR-4 is a big change for the current stage of D8 development, and we only have this on the table because Dries supports it. We want to get this over with asap.

The other issues do not interfere with the API that much, so they are not as urgent.

I am trying something over here:
#2058867: Consider introducing a native ClassLoader for performance and PSR-4

I still think we should first commit the piecemeal patch from here, even if we might switch to a custom autoload.php in a follow-up.

Wooow.. the Symfony loader turns out really slow here!
So, switching to Composer is clearly justified (but is not necessarily the last word spoken).

Ouch ouch ouch! Thanks for fixing!

+++ b/composer.jsonundefined
@@ -27,7 +27,10 @@
+    "files": [
+      "core/lib/Drupal.php"

Should the assetic stuff be in here too? You added this to the autoload_real.php. I've not used this specific notation before, so I thought I'd ask.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -150,7 +150,7 @@ class DrupalKernel implements DrupalKernelInterface, TerminableInterface {
+   * @param \Composer\Autoload\ClassLoader $class_loader
    *   (optional) The classloader is only used if $storage is not given or
    *   the load from storage fails and a container rebuild is required. In
    *   this case, the loaded modules will be registered with this loader in

Docs say this parameter is optional, but the __construct() signature doesn't seem to agree.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -521,7 +521,7 @@ protected function buildContainer() {
+    $container->register('class_loader', 'Composer\Autoload\ClassLoader')->setSynthetic(TRUE);

Curious as to why we're putting the class name in here instead of just new Definition(). Not necessarily "wrong" but I'd love an explanation.

This is a huge improvement.

Eclipse

1.) Ok
2.) I'd not want to complicate this patch any further if that's not specifically an error you made (and based upon the patch, it isn't). We should probably file a follow up for it though.
3.) Ok, so just following the old convention. All the docs I've read on synthetic services just do:

$container->setDefinition('some.name', new Definition())->setSynthetic(TRUE);

That seems the sort of minor clean up we could knock out while we're in here. Beyond that this patch seems pretty ready to go. Are you cool with that change?

Eclipse

Are we, eventually, going to add everything that is Drupal inside the Drupal class? That class already smells and this is utterly wrong.

I believe chx is referring to Drupal::handleRequest().

Perhaps DrupalKernal is a good place for it

So far DrupalKernel does not contain any static methods.
Maybe Drupal:: is a suitable place for stuff that we have not figured out (yet) how to make it not static?

I think it is a good practice to separate classes with static methods from those that are meant to be instantiated.

+1 for moving it to DrupalKernel. From a division of functionality standpoint, it just makes sense.

Do we follow this pattern of separation of static/nonstatic methods anywhere else? That seems pretty silly to me.

We should not add more of composer until #1852512: Directory structure is fail, part 1: Composer is resolved as I predict that using more composer will lead to more breakage like that. And, that issue needs resolving, anyways.

We're not really "using" any more or less of composer here. Yes we're beginning to make use of their autoloader instead of Symfony's, I would assume we have a technical reason for doing that (maybe one less thing for composer to manager? or performance, or something). The composer change you're asking for is absolutely an upstream PR thing (unless I misunderstand badly), and the resulting code in question isn't even Drupal code and is standardized across all composer managed projects everywhere. I am not sure I see why we should block this issue for a change that would be so much more global in scope. Obviously, we could re-organize the code in question locally for Drupal and manage it in composer the same way we do Drupal\Core|Component code, but I don't think that's what that issue is proposing, nor would it make for any real benefit to us and from a code maintenance perspective, we would lose much.

It is completely possible that I have totally misread the intent of the other issue. Please feel free to set me straight if so, but if not, I fail to see how blocking this issue on that issue is beneficial to us.

Eclipse

@RobLoach

Can I get an interdiff?

Eclipse

#1852512: Directory structure is fail, part 1: Composer is totally and utterly irrelevant to this issue. The categorization of that as "breakage" is also, to be blunt, trolling. Please keep this issue focused on the task at hand.

Btw,
over at #2058867: Consider introducing a native ClassLoader for performance and PSR-4,
I am proposing to introduce a custom core/autoload.php that we use instead of core/vendor/autoload.php
(I think that is the first or second patch in the issue)
At the minimum, this will simply be a one-liner:

/**
 * @file core/autoload.php
 */
return require __DIR__ . '/vendor/autoload.php';

The reason this can be useful is that it would allow us to change the autoloader in the future, without changing all the places where autoload.php is included.

I used to think of this as a follow-up, but I may instead just propose it here instead..

It's a follow-up. Anything more than a straight swap of the Symfony autoloader to the Composer autoloader sans-regressions is out of scope for this issue.

I'm pretty happy with that.

Eclipse

Drupal::handleRequest() makes no sense to me and I can't see any justification for it in the issue. It's especially weird that it requires bootstrap.inc

Can we not have core/autoload.php which requires vendor/autoload.php but also hard codes loading a couple of files like this? Then all that pre-autoloader nasty is in one place, instead of right inside the Drupal class which is supposed to be something else entirely.

Also not that keen on registering the composer classloader, using to load exactly one class, then unregistering it again, once more we could use a custom autoload.php and skip that.

Sorry it took me a while to get back to this.

APC is gone. Long live Zend Opcache. The inclusion of Zend Opcache into php 5.5 seals APC's future. Saying APC future is uncertain is mild...IMHO focus should be on Zend Opcache not on APC.

APC vs Zend Opcache for Symfony2:
http://www.ricardclau.com/2013/03/apc-vs-zend-optimizer-benchmarks-with-...

@giorgio79 APC vs. OpCache benchmarks have absolutely nothing to do with using the APC user cache, which has been forked into APCu. Whether APCu ends up used much is a different question but there is no direct relevance from 'Opcache vs. APC' to this issue at all. OpCache just does not provide this feature, and you can use APC's user cache alongside OpCache.

@RobLoach - Drupal contains all one-line methods, adding logic in there goes against the point of the class. The require_once is the worst though yeah.

If necessary a one-method class for drupal_handle_request() would work for me.

i say \Drupal\Core\Bootstrap::handleRequest()

#80 works for me.

Thanks, Rob.

s

rerolled

If we explicitly include bootstrap.inc, then why don't we require_once autoload.php directly from the top of bootstrap.inc?

Because that's a side-effect, and code files should either declare symbols OR have side-effects but not both. (As specified in PSR-1, which we're mostly following for OO code.)

#90: nice :)

++

Eclipse

updated

#88: 2038135-88.patch queued for re-testing.

Patch looks good but I found two file_exists() from Composer's classloader as a result of using it to load classes before the ApcClassloader is swapped in (including the APC classloader itself) when profiling.

This is what I meant by:

Also not that keen on registering the composer classloader, using to load exactly one class, then unregistering it again, once more we could use a custom autoload.php and skip that.

Except it looks like Rob Loach misunderstood what I was complaining about, and I got the number of classes wrong and which classes they were...

Uploading a new patch and interdiff.

Unfortunately #96 means catch can't commit this now, but fortunately I think that means he has no further objections so anyone else can. :-)

(Bot will throw it back it if wants to.)

If it's good enough for catch, it's good enough for me! It's also a requirement on the road to PSR-4, so yay.

Committed and pushed to 8.x. Thanks!

I'm not sure if this needs a change notice? Seems like class loaders are not something most module developers interact with?

This shouldn't affect development for modules at all from a "what do I have to do?" stand point. I don't see the need for a CN.

Eclipse

By design, anyone who is impacted by this issue is already following it. :-) Module developers won't care. Thanks, Angie!

So now every script needs to do

require_once __DIR__ . '/core/vendor/autoload.php';

That needs a change notice and possibly even doxygen update . Related: #1436272: Document how to bootstrap Drupal from other scripts

Yes to chx in #102.

And now it is time for follow-ups!
#2058867: Consider introducing a native ClassLoader for performance and PSR-4
This is an optional thing that will

• allow us to have PSR-4 before Composer, or at least
• allow us to prepare the PSR-4 switch without waiting for Composer.
• allow us to have Drupal-specific performance improvements, if we want to. (benchmarks already there)
• Still have full support for "composer install", composer optimize classmap, etc.
  The proposed custom loaders all behave exactly like composer, and use the information generated in core/vendor/composer/autoload_namespaces.php, autoload_classmap.php, etc.
• switch back to Composer when we want to.
• permanently have core/autoload.php instead of core/vendor/autoload.php, which as a minimum will do nothing but include the core/vendor/autoload.php.
  (This would need yet another change notice)

This killed the APC classloader, at least on every FS that is case sensitive.

Typo in the require_once, Classloader vs. ClassLoader...

#106: real-fs-are-case-sensitive-2038135-106.patch queued for re-testing.

Committed 42ca640 and pushed to 8.x. Thanks!

Restoring title.