Updated: Comment #88

Problem/Motivation

The class loader is quite difficult to work with. There are lots of loop holes that are taken that make it quite difficult to ensure its functionality.

Proposed resolution

Use Composer's autoload.php to handle the registration of third-party projects and Drupal's namespaces. It's future-proof as Composer is ever-evolving, and will handle any additional functionality we aim to provide. Instead of making the class loader swappable, allow the use of ClassLoader wrappers to keep the APC support.

Remaining tasks

  1. RTBC and commit
  2. #2060425: Improve ClassLoader decorator support
  3. #1818628: Use Composer's optimized ClassLoader for Core/Component classes

User interface changes

None.

API changes

Original report by Rob Loach

The class loader interface is a mess, and just needs to be simpler and cleaner.

  1. Load vendor/autoload.php in index.php
  2. Ensure modules can still register additional namespaces
  3. Ensure Plugins is okay
Files: 
CommentFileSizeAuthor
#106 real-fs-are-case-sensitive-2038135-106.patch729 bytesBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,387 pass(es). View
#96 2038135-96.patch13.95 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 58,064 pass(es). View
#96 interdiff.txt1.01 KBcatch
#88 2038135-88.patch13.76 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 58,438 pass(es). View
#88 2038135-88.interdiff.txt6.82 KBRobLoach
#87 2038135-87.patch18.6 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 58,393 pass(es). View
#82 2038135-82.patch18.66 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2038135-82.patch. Unable to apply patch. See the log in the details link for more information. View
#82 2038135-82.interdiff.txt6.04 KBRobLoach
#73 2038135_6.patch18.62 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 57,846 pass(es). View
#71 51-62-interdiff.diff818 bytesRobLoach
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 51-62-interdiff.diff. Unable to apply patch. See the log in the details link for more information. View
#62 2038135.patch18.62 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 57,757 pass(es). View
#51 2038135.patch18.65 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 57,629 pass(es). View
#50 D8-composer-loader-piecemeal-2038135-44.patch18.55 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 58,059 pass(es). View
#44 D8-composer-loader-piecemeal-2038135-44.patch18.58 KBdonquixote
PASSED: [[SimpleTest]]: [MySQL] 57,795 pass(es). View
#44 D8-composer-loader-decorator-2038135-44.interdiff.txt5.25 KBdonquixote
#44 D8-composer-loader-complete-2038135-44.patch21.5 KBdonquixote
PASSED: [[SimpleTest]]: [MySQL] 57,869 pass(es). View
#38 D8-composer-loader-piecemeal-2038135-38.patch18.28 KBdonquixote
PASSED: [[SimpleTest]]: [MySQL] 57,965 pass(es). View
#38 D8-loader-decorator-2038135-26-vs-38.interdiff.txt5.22 KBdonquixote
#37 D8-composer-loader-piecemeal-2038135-37.patch17.9 KBdonquixote
PASSED: [[SimpleTest]]: [MySQL] 57,749 pass(es). View
#37 D8-composer-loader-decorator-2038135-37.interdiff.txt5.27 KBdonquixote
#34 2038135-nopatch-apc.png95.1 KBRobLoach
#34 2038135-nopatch-noapc.png75.5 KBRobLoach
#34 2038135-withpatch-apc.png92.45 KBRobLoach
#34 2038135-withpatch-noapc.png100.79 KBRobLoach
#34 2038135-withpatch-apc-optimized.png118.8 KBRobLoach
#34 2038135-withpatch-noapc-optimized.png119.24 KBRobLoach
#26 2038135.patch21.46 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 57,551 pass(es). View
#23 2038135.patch94.55 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 57,542 pass(es). View
#17 2038135_2.patch92.34 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 57,160 pass(es). View
#15 2038135.patch93.56 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 57,167 pass(es). View
#10 2038135.patch93.7 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] 57,129 pass(es), 16 fail(s), and 0 exception(s). View
#6 2038135.patch79.91 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#5 2038135-composer-autoload.patch74.87 KBCrell
FAILED: [[SimpleTest]]: [MySQL] 56,113 pass(es), 15 fail(s), and 11 exception(s). View
#3 2038135-composer-autoload.patch72.42 KBCrell
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh --clean. View
#1 2038135.patch13.73 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2038135.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

RobLoach’s picture

Status: Active » Needs work
FileSize
13.73 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2038135.patch. Unable to apply patch. See the log in the details link for more information. View
Crell’s picture

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

Crell’s picture

Title: Put drupal_classloader() on a diet » Use the Composer autoloader to make everything simpler
Status: Needs work » Needs review
FileSize
72.42 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh --clean. View

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?

Status: Needs review » Needs work

The last submitted patch, 2038135-composer-autoload.patch, failed testing.

Crell’s picture

Status: Needs work » Needs review
FileSize
74.87 KB
FAILED: [[SimpleTest]]: [MySQL] 56,113 pass(es), 15 fail(s), and 11 exception(s). View

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.

RobLoach’s picture

FileSize
79.91 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
  • Cleaned up some docs
  • Fixed settings.php
  • Removed $settingsObject change
  • statistics.php

Status: Needs review » Needs work
Issue tags: -composer

The last submitted patch, 2038135.patch, failed testing.

RobLoach’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work
Issue tags: +composer

The last submitted patch, 2038135.patch, failed testing.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
93.7 KB
FAILED: [[SimpleTest]]: [MySQL] 57,129 pass(es), 16 fail(s), and 0 exception(s). View

Moving drupal_handle_request() so that bootstrap.inc doesn't have to be included for unit tests.

ParisLiakos’s picture

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

Status: Needs review » Needs work

The last submitted patch, 2038135.patch, failed testing.

Crell’s picture

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

ParisLiakos’s picture

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

RobLoach’s picture

Status: Needs work » Needs review
FileSize
93.56 KB
PASSED: [[SimpleTest]]: [MySQL] 57,167 pass(es). View
  • define('REQUEST_TIME', (int) $_SERVER['REQUEST_TIME']);
RobLoach’s picture

Issue summary: View changes

Updated issue summary.

RobLoach’s picture

Issue summary: View changes

a

Crell’s picture

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

RobLoach’s picture

FileSize
92.34 KB
PASSED: [[SimpleTest]]: [MySQL] 57,160 pass(es). View

Removed the above change Crell mentioned.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Assigned: Unassigned » catch

Assigning to @catch since he some performance concerns.

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs profiling

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

catch’s picture

catch’s picture

Status: Needs work » Postponed
RobLoach’s picture

Status: Postponed » Needs review
FileSize
94.55 KB
PASSED: [[SimpleTest]]: [MySQL] 57,542 pass(es). View

A few things this patch does:

  1. Does not remove the ability to use APC
  2. Documents how to use APC/Xcache/etc from settings.php
  3. Introduces a drupal_classloader_initialize() function that facilitates such activities

Would probably like to clean up the drupal_classloader_initialize() function a bit more, but this shows that we don't have to suffer regression in the switch. Making the class loader swappable is kind of silly, instead you'd want to provide a wrapper around the class loader to gain benefit of the caching systems.

Having all of this in a class map is completely unrelated to this issue. This issue is simply about using Composer's ClassLoader, and not suffering regression in the process.

RobLoach’s picture

Issue summary: View changes

j

Crell’s picture

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.

RobLoach’s picture

Status: Needs review » Needs work

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.

By making the wrapper swappable, I mean:

$settings['class_loader'] = 'Symfony\Component\ClassLoader\ApcClassLoader';

Then you can use whatever cached-ClassLoader you want. They all use the same API. What I think should change in this patch is in drupal_classloader_initialize(). We could improve this a plenty, some ideas:

  1. Return the existing ClassLoader wrapper object
  2. Rename it to drupal_classloader_wrapper()?
  3. Do something awesomenesssauces
RobLoach’s picture

Issue summary: View changes

a

RobLoach’s picture

Issue summary: View changes

Updated issue summary.

RobLoach’s picture

FileSize
21.46 KB
PASSED: [[SimpleTest]]: [MySQL] 57,551 pass(es). View
  1. Cleaned up drupal_classloader_wrapper()
  2. Removed unneeded extra stuff from drush composer dump-autoload
RobLoach’s picture

Issue summary: View changes

Updated issue summary.

Crell’s picture

Status: Needs work » Needs review

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

donquixote’s picture

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.

RobLoach’s picture

Composer does not support PSR-4 (yet).

Off topic from this, but it will be part of Composer in beta1 when that happens. Get involved in the next FIG vote so we could make that happen quicker. Class loading is simply class loading. There's also #1971198: [policy] Drupal and PSR-0/PSR-4 Class Loading if you want to talk about PSR-4 for Drupal-stuffs.

Its lookup algorithm is not the fastest we could get for Drupal (esp. for modules).

Premature optimization, and if performance is a concern, then use a caching facility like the ApcClassLoader, or any of the other decorators.

Composer-generated class maps do not contain module-provided classes, because Composer does not know which of them are enabled.

We're not using any class maps for this, but if you want to get involved in that discussion though, join over at #1818628: Use Composer's optimized ClassLoader for Core/Component classes.

It does not support class discovery (in the way we need, e.g. for AnnotatedClassDiscovery).

Again, as you mentioned, this is rather off-topic from this discussion. Go over to #2033611: Port AnnotatedClassDiscovery to PSR-4, and make it agnostic of directory information.. I'm sure we could support it with a decorator if we wanted, any any other method. Implementation detail.

There is nothing in autoload.php we could not do with a custom or 3rd party loader.

That's precisely the point. autoload.php is there, and it's working, let's use it. Your pull request looks great and we should go ahead with it. Composer is always evolving and getting better, let's use it and gain that benefit from it.

We have to write less code ourselves, and don't rely on additional 3rd party code.

Yup! Proudly Found Elsewhere. We save a lot of the bloat of drupal_classloader() with this, while keeping the flexibility.

donquixote’s picture

#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?

Crell’s picture

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.

donquixote’s picture

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

donquixote’s picture

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

RobLoach’s picture

donquixote’s picture

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.

donquixote’s picture

Issue summary: View changes

Updated issue summary.

RobLoach’s picture

Issue summary: View changes

a

donquixote’s picture

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.

donquixote’s picture

FileSize
5.27 KB
17.9 KB
PASSED: [[SimpleTest]]: [MySQL] 57,749 pass(es). View
donquixote’s picture

FileSize
5.22 KB
18.28 KB
PASSED: [[SimpleTest]]: [MySQL] 57,965 pass(es). View

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

Status: Needs review » Needs work
Issue tags: -Proudly Found Elsewhere, -Needs profiling, -composer

The last submitted patch, D8-composer-loader-piecemeal-2038135-38.patch, failed testing.

donquixote’s picture

Status: Needs work » Needs review
Issue tags: +Proudly Found Elsewhere, +Needs profiling, +composer
RobLoach’s picture

Review

  • 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
  • I can understand that we are technically doing two things here, but I strongly recommend the approach taken in #26. Gives us the flexibility we want, it works, and uses tools that are already available to us. Jamming both the decorator initialization and retrieval of the loader into drupal_classloader() feels like bloat since they do different things.
  • Although there are better ways to do benchmarking, it shows there are improvements... We have to push forwards

Recommendation

  • Re-roll #26 and upload that? Either that or fix #26's classloader_wrapper function to use "apc" instead of "Symfony\ThingyThingy" decorator
RobLoach’s picture

Status: Needs review » Needs work
donquixote’s picture

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.

donquixote’s picture

Status: Needs work » Needs review
FileSize
21.5 KB
PASSED: [[SimpleTest]]: [MySQL] 57,869 pass(es). View
5.25 KB
18.58 KB
PASSED: [[SimpleTest]]: [MySQL] 57,795 pass(es). View

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.

donquixote’s picture

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();
RobLoach’s picture

I'd recommend D8-composer-loader-piecemeal-2038135-44.patch and maybe move it into \Drupal afterwards. That patch looks pretty good to me as it is. Swiches the class loader, cleans up a lot of its use, keeps APC as an option, I think it's the best of both worlds.

What follow ups should we put together?

donquixote’s picture

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.

donquixote’s picture

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.

donquixote’s picture

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

RobLoach’s picture

Issue summary: View changes

Updated issue summary.

RobLoach’s picture

Issue tags: -Needs profiling
FileSize
18.55 KB
PASSED: [[SimpleTest]]: [MySQL] 58,059 pass(es). View

Between donquixote's benchmarks and my profile runs, I'd consider the speed benefits are clear and will only get better.

Attaching D8-composer-loader-piecemeal-2038135-44.patch from above with fixed 644 permission on core/update.php. I'll update the issue summary with the new scope change, and update any follow ups in the OP. Just need a good review! Thanks.

RobLoach’s picture

FileSize
18.65 KB
PASSED: [[SimpleTest]]: [MySQL] 57,629 pass(es). View
  • Fixed an issue where the loader would register twice inadvertently in drupal_classloader(). Composer registers the loader for us.
donquixote’s picture

Ouch ouch ouch! Thanks for fixing!

Status: Needs review » Needs work

The last submitted patch, 2038135.patch, failed testing.

RobLoach’s picture

Status: Needs work » Needs review
RobLoach’s picture

#51: 2038135.patch queued for re-testing. Unrelated.

EclipseGc’s picture

+++ 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

RobLoach’s picture

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.

Assetic defines its own registration methods in its own composer.json file. We just have to load autoload.php, and Composer handles Assetic's namespace registration for us.

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

Interesting... This is simply just changing to Composer ClassLoader, not changing anything else. Do you believe that fix should be part of this patch?

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.

Again, this is just switching over. We could change the Container's class_loader definition, but I'd rather just keep this change as simple as possible.

Thanks!

EclipseGc’s picture

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

chx’s picture

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

RobLoach’s picture

3.) Ok, so just following the old convention. All the docs I've read on synthetic services just do:

Did some investigation, and it looks like the definition is overridden by the class loader instance anyway as it's passed in. This just changes what class type it expects, the instance itself is saved in initializeContainer():

  $this->container->set('class_loader', $this->classLoader);
Are we, eventually, going to add everything that is Drupal inside the Drupal class? That class already smells and this is utterly wrong.

This patch does not add anything to the Container or the DrupalKernal or Drupal classes. It actually takes steps that will allow us to remove the ClassLoader instance from the Container and DrupalKernal class entirely, since its initialization is much earlier than before. We should put together a follow up to remove it.

Crell’s picture

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

RobLoach’s picture

FileSize
18.62 KB
PASSED: [[SimpleTest]]: [MySQL] 57,757 pass(es). View

This patch removes the class definition of class_loader. EclipseGC mentioned that the definition argument wasn't really needed in IRC.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -521,7 +521,7 @@ protected function buildContainer() {
     // Register synthetic services.
-    $container->register('class_loader', 'Symfony\Component\ClassLoader\ClassLoader')->setSynthetic(TRUE);
+    $container->register('class_loader')->setSynthetic(TRUE);
     $container->register('kernel', 'Symfony\Component\HttpKernel\KernelInterface')->setSynthetic(TRUE);
I believe chx is referring to Drupal::handleRequest().

Ah, I understand, and I completely agree with him. Perhaps DrupalKernal is a good place for it, but it does need to live somewhere. Drupal seems like a good interim solution. We could move it somewhere else after this.

donquixote’s picture

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.

cweagans’s picture

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

chx’s picture

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.

EclipseGc’s picture

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

EclipseGc’s picture

@RobLoach

Can I get an interdiff?

Eclipse

Crell’s picture

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

donquixote’s picture

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

Crell’s picture

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.

RobLoach’s picture

FileSize
818 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 51-62-interdiff.diff. Unable to apply patch. See the log in the details link for more information. View

Can I get an interdiff?

Status: Needs review » Needs work

The last submitted patch, 51-62-interdiff.diff, failed testing.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
18.62 KB
PASSED: [[SimpleTest]]: [MySQL] 57,846 pass(es). View

Whoops, forgot to append -ignore in the interdiff. Here's the patch from #62.

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

I'm pretty happy with that.

Eclipse

catch’s picture

Assigned: catch » Unassigned
Status: Reviewed & tested by the community » Needs work

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.

RobLoach’s picture

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.

It only unregisters the autoloader if you are switching to the APC loader, which is how the ApcClassLoader is designed. And it actually doesn't really use the autoloader to find where Drupal.php is, since Composer includes it straight from vendor/autoload.php.

Can we not have core/autoload.php which requires vendor/autoload.php but also hard codes loading a couple of files like this?

Turn drupal_handle_request() into core/autoload.php? Where else do you suggest it exists? DrupalKernel was recommended in the past. Is that one require_once() on bootstrap.inc, from drupal_handle_request() the nastiness you were referring to? We tried to have Composer load it for us back at #6, but that caused issues with the unit tests.

giorgio79’s picture

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

catch’s picture

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

RobLoach’s picture

Still haven't gotten a firm answer or direction on where the contents of drupal_handle_request() should live. Choose one, or suggest one:

  1. Drupal\Core\DrupalRequest::handle()
  2. Drupal\Core\DrupalKernel::handleRequest()
  3. \Drupal::handleRequest()
  4. Drupal\Core\HttpKernel::handleRequest()
  5. core/includes/handle_request.php
  6. core/autoload.php

This is our ugly code we're moving around, it needs to live somewhere. Just pick a place, anywhere other than bootstrap.inc. Not going to re-roll until we have a firm answer on where. This has fallen far behind the rest of the world, and needs to move forwards.

ParisLiakos’s picture

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

catch’s picture

#80 works for me.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
6.04 KB
18.66 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2038135-82.patch. Unable to apply patch. See the log in the details link for more information. View
Crell’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, Rob.

Crell’s picture

Issue summary: View changes

s

RobLoach’s picture

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

Status: Reviewed & tested by the community » Needs work
Issue tags: +Proudly Found Elsewhere, +composer

The last submitted patch, 2038135-82.patch, failed testing.

RobLoach’s picture

Issue tags: +Needs reroll
ParisLiakos’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
18.6 KB
PASSED: [[SimpleTest]]: [MySQL] 58,393 pass(es). View

rerolled

RobLoach’s picture

FileSize
6.82 KB
13.76 KB
PASSED: [[SimpleTest]]: [MySQL] 58,438 pass(es). View

Found a few things after talking with EclipseGc about this:

  1. We could actually keep drupal_handle_request() where it is just by including bootstrap.inc after autoload.php
  2. Since we're using Composer's ClassLoader now, we don't need to manually register the PHPUnit prefixes since they're handled for us now

The result is a much smaller patch with more deletions than insertions.

donquixote’s picture

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

Crell’s picture

Status: Needs review » Reviewed & tested by the community

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

RobLoach’s picture

Status: Reviewed & tested by the community » Needs review

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

Maybe having it inside drupal_handle_request()? Not sure about including a file directly when including bootstrap.inc. We could, I'm just not sure about the design implications of it. Would there be any?

Stick it right underneath the following?

/**
 * @file
 * Functions that need to be loaded on every Drupal request.
 */
require_once(dirname(__DIR__) . '/vendor/autoload.php');

EDIT: Ah, ignore all that I said. Read #90.

RobLoach’s picture

Issue summary: View changes

Updated

donquixote’s picture

#90: nice :)

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

++

Eclipse

EclipseGc’s picture

Issue summary: View changes

updated

RobLoach’s picture

Going to be using a fork to keep this change going:
http://github.com/RobLoach/drupal/compare/drupal:8.x...2038135

Feel free to fork from there if you're going to be changing anything. Here's the patch and diff:
http://github.com/RobLoach/drupal/compare/drupal:8.x...2038135.patch
http://github.com/RobLoach/drupal/compare/drupal:8.x...2038135.diff

catch’s picture

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

catch’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.01 KB
13.95 KB
PASSED: [[SimpleTest]]: [MySQL] 58,064 pass(es). View

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.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

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

RobLoach’s picture

+1

Except it looks like Rob Loach misunderstood what I was complaining about

Thought you were referring to the Bootstrap class, which doesn't really exist anymore. This solution works too! Thanks.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

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?

EclipseGc’s picture

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

Crell’s picture

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

chx’s picture

Priority: Normal » Critical
Status: Fixed » Active
Issue tags: +Needs change record

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

jibran’s picture

Title: Use the Composer autoloader to make everything simpler » Change notice: Use the Composer autoloader to make everything simpler
donquixote’s picture

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)
RobLoach’s picture

Status: Active » Needs review
Issue tags: -Proudly Found Elsewhere

Use Composer's ClassLoader to handle class loading... If you have any additions to this, please be bold.

[EDIT] Also, thanks to everyone for all the help on this.

Berdir’s picture

FileSize
729 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,387 pass(es). View

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

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

Status: Needs review » Needs work
Issue tags: -Needs change record, -composer

The last submitted patch, real-fs-are-case-sensitive-2038135-106.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: +Needs change record, +composer
RobLoach’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, Berdir. I'm blaming catch, and myself for not, well, catching it :-) .

RobLoach’s picture

Title: Change notice: Use the Composer autoloader to make everything simpler » Fix to load the ApcClassLoader correctly
Issue tags: -Needs change record

The change notice is here:
http://drupal.org/node/2080891

alexpott’s picture

Priority: Critical » Normal
Status: Reviewed & tested by the community » Fixed

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

amateescu’s picture

Title: Fix to load the ApcClassLoader correctly » Use the Composer autoloader to make everything simpler

Restoring title.

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.