Explore Krautoload as a solution for PSR-4 class loading

mention tiny overhead of multiple loaders

mention benchmarks

mention the alternative that was suggested.

Hi!

We don't need the PEAR implementations at all in Drupal core. Is it necessary or desirable to keep that functionality?

I think for an "external" library that lives in vendor/, it is ok to have some stuff that won't be used in core.
The essential thing is that we don't load/include stuff from that library that we don't need.

Overall, this seems highly abstracted, perhaps more than necessary. That's probably just fine if the end-result is a faster class loader, but I get the feeling that this is more flexibility than we require for Drupal core. If we can optimize even further by removing abstraction, I'm all for that.

I am already killing one level of abstraction for regular loadClass() operations :)
I am going to comment on that when I have a new patch..

Let's use {@inheritdoc} in all the places where documentation is copy/pasted from the interfaces.

Yes!
Documentation needs to be improved throughout..

See also #2033611: Port AnnotatedClassDiscovery to PSR-4, and make it agnostic of directory information.

A big and bad-ass patch.
What does it do?

1. It introduces the krautoload library (or a very modified version, that will soon go upstream).
   core/vendor/krautoload
   This brings autoloading for PSR-0 and PSR-4, and class discovery for both.
2. It uses Krautoload for autoloading.
   For each module, we register lib/ as PSR-0, and src/ as PSR-4.
   (we may change this)
3. It uses Krautoload for AnnotatedClassDiscovery.
4. It uses Krautoload for simpletest class discovery.
5. It replaces the container.namespaces ArrayObject service with a "SearchableNamespaces" object.
6. (EDIT:) It replaces the class_loader service with a Krautoload\\RegistrationHub, which is a wrapper around the class loader and provides a bunch of namespace registration methods. (this is to not bloat the classloader itself with those methods)
7. (EDIT:) It introduces a new test that would break the existing core classloader, but does not break krautoload. (these are edge cases, so you might not care)

EDIT: Caveat: Comments are horrible.

If you don't feel like studying the patch, here is a snapshot of the class discovery in action in simpletest.

      $namespaces = array();
      foreach ($all_data as $name => $data) {
        $namespaces[] = "Drupal\\$name\\Tests";
      }

      // Build a searchable namespace collection.
      $namespaces = drupal_classloader()->buildSearchableNamespaces($namespaces);

      // Check that each class has a getInfo() method and store the information
      // in an array keyed with the group specified in the test information.
      $groups = array();
      foreach ($namespaces->discoverExistingClasses(TRUE) as $class) {
        // Test classes need to implement getInfo() to be valid.
        if (method_exists($class, 'getInfo')) {
          [..]

The goal is to encapsulate the details of the filesystem mappings with PSR-0 or PSR-4, so that the developer can focus on something else.

I had to change how the searchable namespaces are registered as a service.
They need to be marked as persistent, just like the ArrayObject.
Then on rebuild we need to update both the namespace names, AND the internal finder object.

Tests adapted for API changes.

Alternative should be explored in separate issue!

Green, yeah!

Left to do:
- Fine-tune class and method names, and signatures.
- Fix comments. (they are horrible atm)
- Flesh out cache layers, and enable them in Drupal.
- (EDIT:) Write tests in Krautoload.
- Evaluate, benchmark.

And finally:
- Move all class files to the new location according to PSR-4 (src or lib).

We have this a lot in plugin managers:
(example: FieldTypePluginManager)

-  public function __construct(\Traversable $namespaces, CacheBackendInterface $cache_backend, LanguageManager $language_manager, ModuleHandlerInterface $module_handler) {
+  public function __construct(SearchableNamespacesInterface $namespaces, CacheBackendInterface $cache_backend, LanguageManager $language_manager, ModuleHandlerInterface $module_handler) {
     $annotation_namespaces = array(
-      'Drupal\Core\Entity\Annotation' => DRUPAL_ROOT . '/core/lib',
+      'Drupal\Core\Entity\Annotation' => TRUE,
     );
     parent::__construct('field/field_type', $namespaces, $annotation_namespaces, 'Drupal\Core\Entity\Annotation\FieldType');

Maybe this would be nicer for annotation namespaces:

  public function __construct(SearchableNamespacesInterface $namespaces, CacheBackendInterface $cache_backend, LanguageManager $language_manager, ModuleHandlerInterface $module_handler) {
    parent::__construct('field/field_type', $namespaces, 'Drupal\Core\Entity\Annotation\FieldType');
    $this->addAnnotationNamespace('Drupal\Core\Entity\Annotation');

Since this is being added in core/vendor, we'd likely want to have Composer handle the third-party dependency. I put together an issue for that in the krautoload issue queue.

I agree with this idea. The question is the timing.
I think it has been useful so far that Krautoload is not a finalized library yet, and we can develop it alongside Drupal.

So, the timing should be:
1. Bikeshed the Krautoload API (class names, method names, signatures)
2. Push upstream to github, and add a composer.json
3. Get it into Drupal in the regular composer fashion, and commit the patch.

Do you know if we have an issue to move from lib to src? I remember it being a long-winded discussion, and it really doesn't matter which one we go with, but it would be good to track that discussion.

#1400748-30: Proposal for unified namespace organization and following posts.

I think it is fair to say that many of the arguments in favour of lib apply to PSR-0 and PSR-4 likewise.
What may have changed is how other projects use either lib or src, so we could do some statistics.

Either way, I would absolutely suggest to keep this discussion out of this issue.
The meta issue might not benefit from this discussion either, but it is probably still a better place than here.
#1971198: [policy] Drupal and PSR-0/PSR-4 Class Loading

And again, it's a timing thing:
1. Make it work with src/, or whatever is easier to work with during development.
(we might not even commit this)
2. Switch back to lib/ (if decided somewhere), and move the module classes.

Btw, as a temporary thing we could also map the module namespace to (module dir)/lib/Drupal/(module) with PSR-4, and totally forget about src/. Then the decision is postponed until we actually move the modules.

Having both lib/ and src/ can make the transition phase more smooth.
But we could even work with an overlapping mapping. Krautoload has plugins for PSR-0 that are safe to use with mapping ambiguity. (and it will be easy to add the same thing for PSR-4)

#14 (RobLoach):

Looks like you're defining hard-bindings here? I understand that AnnotatedClassDiscovery is a bit hairy at the moment. Do you know if we have an issue around to help clean it up a bit? This is part of the test suite though, so many we don't need to clean it up.

What do you mean with "hard bindings" in this context?

I understand that AnnotatedClassDiscovery is a bit hairy at the moment. Do you know if we have an issue around to help clean it up a bit?

So far I only know of #2033611: Port AnnotatedClassDiscovery to PSR-4, and make it agnostic of directory information..

There is a connection with PSR-4:
In the original AnnotatedClassDiscovery and plugin managers, we are passing around arrays where the keys are namespaces, and the values are filesystem locations associated with those namespaces.
With PSR-4, those arrays would get a different meaning. And, creating respective arrays for sub-namespaces would be different, because we would also have to change the mapped directories to the respective subfolders.
It became even uglier when I wanted to support both PSR-0 and PSR-4 in parallel.

I think encapsulating those into SearchableNamespaces (or name it SearchableNamespaceCollection) is a good idea.
This way, AnnotatedClassDiscovery no longer needs to care about directories, only about namespaces.

SearchableNamespaces needs
- one NamespaceVisitor (or name it NamespaceFinder -> bikeshed!), which is a more powerful version of the ClassLoader, and can handle the filesystem mappings during discovery.
- a simple array of namespace names, without any filesystem information.

During a test, for the NamespaceVisitor, we can either use the one from the container, which should already have all module namespace mappings. Or we create our own, and register only the namespace mappings we need. This is what the patch does in AnnotatedClassDiscoveryTest.

#19:
Krautoload on github is on the way, with composer.json and PHPUnit tests.

Meanwhile, I am trying to refit the plugin manager constructors.

EDIT: Forget this patch!! It is agains the wrong version..

Better..

New stuff:

1. Krautoload has evolved. Some of the changes:
   • Refactoring, improved comments, bug fixes, PHPUnit tests added, etc.
   • Full support for class maps generated by Composer:
     ->composerVendorDir(DRUPAL_ROOT . '/core/vendor') simply picks up whatever it finds in the vendor/composer directory.
     This means, you can run composer dump-autoload --optimize and it will have the same effect as if you had the original Composer class loader in place.
   • Cache layers for APC, XCache, WinCache, equivalent to those in Symfony (in fact the logic is mostly copied, but the architecture is different).
   • Bootstrap helper Krautoload::start() now receives an $options array to activate the cache (among other things).
2. Krautoload is now downloaded via composer, to core/vendor/donquixote/krautoload.
   (See Project name: "krautoload" vs "donquixote/krautoload" vs "krautoload/krautoload". on github)
3. APC cache activated depending on a setting.
4. Use php core/scripts/switch-psr4.sh to automatically move all module class files to their new location. (currently that is src/)
   It turns out you can run this without even having to flush caches! Isn't that awesome :)

Important stuff from previous patches:

• Class loading with Krautoload instead of Symfony or Composer.
• AnnotatedClassDiscovery: Discovery with Krautoload instead of Drupal-native.
  See #2033611: Port AnnotatedClassDiscovery to PSR-4, and make it agnostic of directory information.
• simpletest: Discovery with Krautoload instead of Drupal-native.
• class_loader service is now an instance of Krautoload\RegistrationHub.
  This means it no longer is a real class loader, but a facade object to do registration.
  See https://github.com/donquixote/krautoload/issues/14
• container.namespaces service is no longer an ArrayObject, but a Krautoload\SearchableNamespaces_Interface object, encapsulating the directory information.
• Signature of plugin managers changes, because they no longer receive an ArrayObject.
  See #2033611-13: Port AnnotatedClassDiscovery to PSR-4, and make it agnostic of directory information.

About the patch:
composer update / composer install did result in a huge changeset, with stuff unrelated to this issue.
This is because some libraries have a new version.
For this patch, I did revert all of those changes except for core/vendor/donquixote/krautoload, to make it easier to review.

It seems for every valid patch I have to upload at least 1x garbage..

We could have a tool that generates a class map for everything: Core and enabled extensions (modules/themes).
This class map would live somewhere in the generated code directory (e.g. sites/default/files/php).

By definition, this file cannot be generated by composer, because composer does not know about enabled modules.
It will need a Drupal bootstrap to know about modules, so it can't be a simple standalone script.
It has to consider all namespaces that are registered in every request, but it has to ignore namespaces that are just temporarily registered - e.g. for simpletest.

Since Krautoload already has some discovery capabilities, it wouldn't be too hard to add one for generating class maps.
The big question is, where to call this within Drupal.

Mention that it can work with a class map compiled by composer.

I updated the issue summary and the "arguments" section, I hope this is not too opinionated.

Alternatives

It was suggested to use a hand-crafted PSR-4 loader, and do all the rest with Composer's class loader.
The composer loader would have a compiled class map for core classes.

However,
- Krautoload can handle compiled class maps just as good as Composer, and it plays nicely with the composer vendor directory.
- Krautoload can have a cache layer on top of everything, instead of "just" for modules.
- By having two class loaders on the stack, we totally eat up any gain from micro-optimization. Having two loaders means that for one half (or whatever percent) of classes we fire a loader that does nothing.

E.g. if the composer loader is first in the chain, only for core and vendor, and autoload fires for a module-provided class, then the following will happen:
- Composer will look in the class map and find nothing.
- Composer will look into registered prefixes, which is exactly what we want to avoid. And again, finds nothing.
- Krautoload will find the class (e.g. in APC cache).

On the other hand, if Krautoload is first, and autoload fires with a core- or vendor-provided class, then
- Krautoload will check the APC cache, and find nothing.
- Krautoload will check the registered prefixes, which is exactly what we want to avoid. And again, finds nothing.
- Composer will do the class loading (e.g. via class map)

Besides, Composer does not help us with class discovery.

If this alternative is still being considered, it should be explored in a separate issue!

Updated issue summary: Krautoload has evolved. Removed "performance" section, this is sufficiently covered elsewhere. Add more thoughts to the "alternative".

EDIT: Yeah, pass!
For patch notes, see #22.

Left to do:

1. Reduce hard references to the Krautoload library, and replace them with Drupal-native classes in most places.
   This makes us less vulnerable to changes in Krautoload (it is still "alpha").
2. (Decide on the signature for plugin managers (see #22).)

The plan:

1. Drupal-native SearchableNamespaces(+Interface) extending the one from Krautoload, so plugin managers don't need to hard-reference Krautoload.
2. Drupal-native ClassLoaderAdapter(+Interface) and NamespaceInspectorAdapter(+Interface) extending the RegistrationHub from Krautoload, and adding some Drupal-specific methods.
   Please give some feedback here on github: Find a better name for RegistrationHub, and add an interface.
   Btw, for my taste this thing may also implement the class-loader-adapter thingie by RobLoach.

I played around locally, and got this to work:

// Adapter for a simple class loader.
$adapter = \Drupal\Core\ClassLoader\ClassLoaderAdapter::start();
// Adapter for a namespace inspector.
$adapter->addDrupalExtension('system', 'core/modules/system');
// Register Drupal-specific stuff.
$adapter->addDrupalExtensionsByFileName($extension_filenames);
$adapter->addDrupalExtensionTests('system', 'core/modules/system');
$adapter->addDrupalCore();
$adapter->addDrupalCoreTests();
// Same as Krautoload, but returns an instance of \Drupal\Core\ClassLoader\SearchableNamespacesInterface.
$searchable_namespaces = $adapter->buildSearchableNamespaces(array('Drupal\system\Plugin\xyz'));

The cool thing: There will be only one class to change, to switch from PSR-0 to PSR-4 for modules.

Updated issue summary.

@Pancho: I need some feedback over at
#2033611: Port AnnotatedClassDiscovery to PSR-4, and make it agnostic of directory information.
This is the part that is the most intrusive to existing code, and I can't do these decisions alone.

And of course, I would be happy if people could finally settle on the src vs lib.
Maybe organize a vote over in the meta issue?

More stuff!

• Classes in tests folders move to PSR-4:
  tests/Drupal/$extension/Tests -> tests/src
• Update Krautoload to 0.0.1-alpha7.
• Introduce a Drupal-native ClassLoaderAdapterInterface (and NamespaceInspectorAdapter+Interface) extending their respective Krautoload counterparts, adding Drupal-specific registration methods.
• Introduce a Drupal-native SearchableNamespacesInterface, to reduce hard references to Krautoload all over the place.
• Only one file is left that decides whether module namespaces are registered as PSR-0 and PSR-4. So the aftermath will be super easy.

I was going to upload a second patch which actually has all the class files being moved around.
Unfortunately, the size of that is 18.6 MB, so beyond the upload limit. I might find a different solution, e.g. a pull request?

@Pancho: Now is the time for reviewing and feedback!

@Pancho, others:
Anyone who wants to help testing this, please try
php core/scripts/switch-psr4.sh
This should go even without a cache refresh.
(I'm not sure about simpletest, maybe a flush will be useful)

Here we have two branches to explore:
PSR-4 enabled:
https://github.com/donquixote/drupal/tree/8.x-krautoload/core/modules/sy...
Files moved to new location:
https://github.com/donquixote/drupal/tree/8.x-krautoload-moved/core/modu...

Links go directly to system module, which is a nice example.
Have a look at src/ and tests/src/.

EDIT: Attention, the git history is messy! We don't want to pull these branches with all their history.

Describe how Krautoload will be embedded in Drupal.

There are two problems with this:

• The algorithm and plugin system in Krautoload is not the best we can get, performance-wise. It is faster than Composer if you have many modules, but we can do much better, if we are already writing our own thing.
• ClassLoader and NamespaceInspector being the same object may be convenient, but this does make it more difficult to replace the loader with something custom via settings.php, because now if you want to replace the loader you also need to replace the NamespaceInspector.

I am proposing something over at
#2023325-27: Add interface for classloader so it can be cleanly swapped
that should solve these problems. (but so far does nothing about plugin discovery)

The loader might live in Drupal core, or it might be outsourced to github (e.g. as Krautoload v2). The latter would have the benefit that it is easier for others to reuse.

All this being said: I would like to keep a lot of the architectural stuff - e.g. the concept of SearchableNamespaces, NamespaceInspector, etc. So, reviews are still appreciated, especially if they focus on this stuff.

Yes, this can be closed.