Quick summary:
Explore Krautoload as a solution for PSR-4 class loading.
About PSR-4 / PSR-X
The PHP-FIG group is discussing a new autoload standard, that will be an alternative to PSR-0 with
- more shallow directory structure.
- no more special handling of the underscore in the class name, and thus, no more backwards support for PEAR.
The working title is "PSR-X", the official title of the spec will probably be "PSR-4".
https://github.com/php-fig/fig-standards/blob/master/proposed/autoloader.md
It was suggested to adopt this for Drupal, even before the official PSR is released.
The main argument was a more DX-friendly directory structure, avoiding subfolder levels that are perceived as redundant.
#1971198: [policy] Drupal and PSR-0/PSR-4 Class Loading
Popular class loading solutions out there (Symfony, Composer) do not yet support the upcoming standard.
One obvious reason is that the standard is not released yet, and it is not entirely clear which name it will have.
The Krautoload library
Krautoload (github.com/donquixote/krautoload) has been suggested by donquixote as a possible solution for PSR-4 class loading in Drupal.
Some facts about the Krautoload library:
- It is a spin-off of the xautoload module, which currently has > 1000 happy users in D7, and rarely any issues.
- It copies or mimicks a lot of stuff from Symfony and Composer. E.g. it can be wired up with a class map and with PSR-0 prefixes, and it has cache decorators for APC, XCache, WinCache. This stuff is equivalent with Symfony/Composer, so we can expect that performance will be mostly identical.
- It has a unique lookup algorithm, with plugins registered for specific namespaces/directories, to distinguish between different patterns (PSR-4, PSR-0, PEAR, other). The lookup is O(1), no matter how many namespaces are registered, and how many different patterns are in use.
- It supports PSR-4.
- It is designed to compete in performance with other popular class loaders. Especially if you have many namespaces.
- It has micro-optimizations for different variations of PSR-0. E.g. if you already know you are not going to use underscores in the class name, Krautoload can use this information to optimize the lookup.
- It is fully aware of all the subtleties of PSR-0, that other loaders silently ignore. It is the user's choice whether to fully support those subtleties, or to take the same (tiny) risk as other loaders.
- It provides class discovery with different filesystem patterns (PSR-0, PSR-4).
- There are convenience methods to do typical composer stuff.
The only "caveat" would be that this is a brand-new thing which is still evolving, and has no significant known user base.
I personally don't see this as a big problem, but it means we apply do some precautions:
We should attempt to reduce the number of places with hard references to Krautoload. Esp, avoid that modules and contrib have to deal directly with Krautoload stuff.
E.g. by subclassing some of the classes and interfaces, so that modules can deal with Drupal-native equivalents.
Integration in Drupal
Changes to module directory structure
New PSR-4 file structure:
class: Drupal\$extension_name\Foo\Bar
old (PSR-0): $extension_dir/lib/Drupal/$extension_name/Foo/Bar.php
new (PSR-4): $extension_dir/src/Foo/Bar.php
(src vs lib is still discussed)
Handling of underscores (not happening in Drupal, just for completeness)
class: Drupal\$extension_name\Foo\Bar_Baz
old (PSR-0): $extension_dir/lib/Drupal/$extension_name/Foo/Bar/Baz.php
new (PSR-4): $extension_dir/src/Foo/Bar_Baz.php
(src vs lib is still being discussed)
Classes in "tests" subfolder:
class: Drupal\$extension_name\Tests\FooTest
old (PSR-0): $extension_dir/tests/Drupal/$extension_name/Tests/FooTest.php
new (PSR-4): $extension_dir/tests/src/FooTest.php
(src vs lib is still being discussed)
Changes to namespace registration
Krautoload provides a rich adapter interface to register "stuff" to the class loader.
Drupal extends this, to add methods that are specific to Drupal.
// Bootstrap:
$adapter = Krautoload::start($options);
// Register composer classmap and namespaces found in the vendor/composer directory.
// (this includes Drupal core)
$adapter->composerVendorDir(DRUPAL_ROOT . '/core/vendor');
// Register a Drupal extension (currently PSR-0 *and* PSR-4).
$adapter->addDrupalExtension('system', 'core/modules/system');
// Register a Drupal extension 'tests' folder.
$adapter->addDrupalExtensionTests('system', 'core/modules/system');
Changes to AnnotatedClassDiscovery and plugin managers
See #2033611: Port AnnotatedClassDiscovery to PSR-4, and make it agnostic of directory information.
This is important to review!!!
Changes to simpletest discovery
simpletest_classloader_register();
[..]
$namespaces = array('Drupal\system\Tests', 'Drupal\views\Tests', ...);
$searchable_namespaces = drupal_classloader()->buildSearchableNamespaces($namespaces);
foreach ($searchable_namespaces->discoverExistingClasses(TRUE) as $class) {
..
}
Plan of execution
We will go smoothly in different steps:
- Implement support PSR-4 alongside PSR-0, without actually moving any classes.
Have a script that can move all classes around to their new location, so people can give it a try.
This is already working with current patches! (e.g. #32) - Move module classes around to their new location:
This is as easy as executing that script.
Give D8 contrib a chance to do the same. - Remove remaining PSR-0 stuff, clean up.
This should not be too hard either.
Status
Both class loading and plugin discovery have been proven to work fine.
The library on github has mostly stabilized, and semver will protect us from breaking API changes.
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.
Besides, Composer does not help us with class discovery.
If this alternative is still being considered, it should be explored in a separate issue!
Performance impact of multiple class loaders on the spl stack
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.
- Composer will do the class loading (e.g. via class map)
Comment | File | Size | Author |
---|---|---|---|
#32 | D8-krautoload-2033501-32.patch | 290.46 KB | donquixote |
#28 | D8-krautoload-psrx-2033501-28.patch | 266.32 KB | donquixote |
#24 | D8-krautoload-psrx-2033501-24.patch | 265.68 KB | donquixote |
#23 | D8-krautoload-psrx-2033501-23.patch | 265.58 KB | donquixote |
#22 | D8-krautoload-psrx-2033501-22.patch | 265.58 KB | donquixote |
Comments
Comment #0.0
donquixote CreditAttribution: donquixote commentedmention tiny overhead of multiple loaders
Comment #0.1
donquixote CreditAttribution: donquixote commentedmention benchmarks
Comment #0.2
donquixote CreditAttribution: donquixote commentedmention the alternative that was suggested.
Comment #1
quicksketchThanks @donquixote for making this new issue. My first look over the code, some things appeared to me:
- We don't need the PEAR implementations at all in Drupal core. Is it necessary or desirable to keep that functionality?
- 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.
- Let's use
{@inheritdoc}
in all the places where documentation is copy/pasted from the interfaces.This sort of thing isn't really an area I'm familiar with, so I'm glad to see you taking it on.
Comment #2
donquixote CreditAttribution: donquixote commentedHi!
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.
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..
Yes!
Documentation needs to be improved throughout..
Comment #3
donquixote CreditAttribution: donquixote commentedSee also #2033611: Port AnnotatedClassDiscovery to PSR-4, and make it agnostic of directory information.
Comment #4
donquixote CreditAttribution: donquixote commentedA big and bad-ass patch.
What does it do?
core/vendor/krautoload
This brings autoloading for PSR-0 and PSR-4, and class discovery for both.
For each module, we register lib/ as PSR-0, and src/ as PSR-4.
(we may change this)
EDIT: Caveat: Comments are horrible.
Comment #5
donquixote CreditAttribution: donquixote commentedIf you don't feel like studying the patch, here is a snapshot of the class discovery in action in simpletest.
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.
Comment #6
donquixote CreditAttribution: donquixote commentedComment #8
donquixote CreditAttribution: donquixote commentedI 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.
Comment #9
donquixote CreditAttribution: donquixote commentedComment #11
donquixote CreditAttribution: donquixote commentedTests adapted for API changes.
Comment #11.0
donquixote CreditAttribution: donquixote commentedAlternative should be explored in separate issue!
Comment #12
donquixote CreditAttribution: donquixote commentedGreen, 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).
Comment #13
donquixote CreditAttribution: donquixote commentedWe have this a lot in plugin managers:
(example: FieldTypePluginManager)
Maybe this would be nicer for annotation namespaces:
Comment #14
RobLoachLooks 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.
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.
Do you know if we have an issue to move from
lib
tosrc
? 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.Comment #15
donquixote CreditAttribution: donquixote commentedI 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.
Comment #16
donquixote CreditAttribution: donquixote commented#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)
Comment #17
donquixote CreditAttribution: donquixote commented#14 (RobLoach):
What do you mean with "hard bindings" in this context?
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.
Comment #18
PanchoRe #14:
In #1971198-151: [policy] Drupal and PSR-0/PSR-4 Class Loading I tried to distill the arguments already discussed in #1400748: Proposal for unified namespace organization.
I think that #1971198: [policy] Drupal and PSR-0/PSR-4 Class Loading would be a good place to finalize naming conventions and more general specifications, while keeping this for the technical implementation.
Comment #19
RobLoachI'd love to keep this as simple as possible, and decouple as much as we can so that we don't run into a spaghetti mess.
I'd actually recommend focusing on making the Krautoload project flawless in your eyes on GitHub first, before bringing it in. It would mean reaching a wider audience, and allow independent testing decoupled from Drupal.
If you're concerned about timing, just follow Semantic Versioning and put out a
0.0.1-alpha.1
and other alpha/beta releases, people would understand that the API is subject to change, but would know that a future is there for the project. That would also allow Drupal to target specific versions based on our needs.Oh, I meant hard bindings to the class loader. You're interacting directly with the loader instead of depending on the already registered namespaces.
src
vslib
is a completely separate discussion. Not quite sure why that would have to happen here.Naming of any kind does not matter. As long as we can target a specific version (composer.json and semver), then we can upgrade at our choosing.
Comment #20
donquixote CreditAttribution: donquixote commented#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..
Comment #21
donquixote CreditAttribution: donquixote commentedBetter..
Comment #22
donquixote CreditAttribution: donquixote commentedNew stuff:
->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.(See Project name: "krautoload" vs "donquixote/krautoload" vs "krautoload/krautoload". on github)
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:
See #2033611: Port AnnotatedClassDiscovery to PSR-4, and make it agnostic of directory information.
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
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.
Comment #23
donquixote CreditAttribution: donquixote commentedIt seems for every valid patch I have to upload at least 1x garbage..
Comment #24
donquixote CreditAttribution: donquixote commentedComment #25
donquixote CreditAttribution: donquixote commentedWe 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.
Comment #25.0
donquixote CreditAttribution: donquixote commentedMention that it can work with a class map compiled by composer.
Comment #26
donquixote CreditAttribution: donquixote commentedI updated the issue summary and the "arguments" section, I hope this is not too opinionated.
Comment #26.0
donquixote CreditAttribution: donquixote commentedUpdated issue summary: Krautoload has evolved. Removed "performance" section, this is sufficiently covered elsewhere. Add more thoughts to the "alternative".
Comment #28
donquixote CreditAttribution: donquixote commentedEDIT: Yeah, pass!
For patch notes, see #22.
Comment #29
donquixote CreditAttribution: donquixote commentedLeft to do:
This makes us less vulnerable to changes in Krautoload (it is still "alpha").
The plan:
SearchableNamespaces(+Interface)
extending the one from Krautoload, so plugin managers don't need to hard-reference Krautoload.ClassLoaderAdapter(+Interface)
andNamespaceInspectorAdapter(+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:
The cool thing: There will be only one class to change, to switch from PSR-0 to PSR-4 for modules.
Comment #29.0
donquixote CreditAttribution: donquixote commentedUpdated issue summary.
Comment #30
PanchoNice. There seems to be quite much progress...
Just tell us when we should really start reviewing concept and details!
Comment #31
donquixote CreditAttribution: donquixote commented@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?
Comment #32
donquixote CreditAttribution: donquixote commentedMore stuff!
tests/Drupal/$extension/Tests
->tests/src
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!
Comment #33
donquixote CreditAttribution: donquixote commented@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)
Comment #34
donquixote CreditAttribution: donquixote commentedHere 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.
Comment #35
tstoecklerWow, I like the file structure there. I also like the moving the test classes to /tests/src. Keeps all the test-related stuff in one place. Don't really care about src/ vs. lib/
Comment #35.0
tstoecklerNo longer point to the drupal-vendor branch, which is now outdated.
Comment #35.1
donquixote CreditAttribution: donquixote commentedDescribe how Krautoload will be embedded in Drupal.
Comment #36
donquixote CreditAttribution: donquixote commentedThere are two problems with this:
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.
Comment #37
eigentor CreditAttribution: eigentor commentedI don't understand no nothing of the technical stuff, but I love the name!
Comment #37.0
eigentor CreditAttribution: eigentor commentedUpdated issue summary.
Comment #38
sunUnless I'm mistaken, this appears to be obsolete and duplicated by #2083547: PSR-4: Putting it all together now?
Comment #39
donquixote CreditAttribution: donquixote commentedYes, this can be closed.