Goal:
Module-provided class files should be organized in PSR-4 instead of PSR-0, to avoid redundant nested directories.
See #1971198: [policy] Drupal and PSR-0/PSR-4 Class Loading

The following issues contain piecemeal work for PSR-4:
#2033611: Port AnnotatedClassDiscovery to PSR-4, and make it agnostic of directory information.
#2083267: Port discovery of test classes to PSR-4
#2083045: Script to move class files to their new PSR-4 location

This one is about merging those different pieces into one.

Branch which usually reflects the last patch:
https://github.com/donquixote/drupal/compare/8.x-psr4
(attention: regular rebase happening!)

Suggested way to commit this (#52):

  1. merge or cherry-pick all or part of the history on github, WITHOUT any files moved around. (because any patch with so many files moved around, or even a PR, will quickly get out of date and generate many conflicts). OR just apply the patch (again, without files moved around). Note: cherry-pick or rebase will get us a nicer commit history.
  2. run the core/scripts/switch-psr4.sh
  3. commit the move
  4. rebase --interactive, and remove the commit that added the script
  5. rebase --interactive, to fine-tune commit messages (optional)
  6. run all tests locally, to be sure
  7. push upstream

Important follow-up:
#2094843: Port PHPUnit test classes in (module dir)/tests/ to PSR-4

Files: 
CommentFileSizeAuthor
#197 D8-2083547-197-psr4-src-shortcut.patch57.79 KBdonquixote
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,052 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new85.31 KB
FAILED: [[SimpleTest]]: [MySQL] 57,251 pass(es), 249 fail(s), and 125 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 8.x-psr4-combined-2083547-1.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new85.65 KB
FAILED: [[SimpleTest]]: [MySQL] 57,185 pass(es), 263 fail(s), and 137 exception(s).
[ View ]

StatusFileSize
new86.31 KB
PASSED: [[SimpleTest]]: [MySQL] 58,139 pass(es).
[ View ]

I have a 10GB patch after running the move script :)
That's too big, I would say.

Patch should be way smaller if you generate it with "git diff -M50"
(or alternatively, put this in your .gitconfig

[diff]
  renames = copies

?)

StatusFileSize
new788.7 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch D8-custom-psr4-combined-moved-7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

After running the core/scripts/switch-psr4.sh

Status:Needs review» Needs work

The last submitted patch, D8-custom-psr4-combined-moved-7.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new788.94 KB
FAILED: [[SimpleTest]]: [MySQL] 57,871 pass(es), 11 fail(s), and 0 exception(s).
[ View ]
new86.43 KB
FAILED: [[SimpleTest]]: [MySQL] 58,151 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, D8-psr4-combined-moved-2083547-9.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new789.29 KB
FAILED: [[SimpleTest]]: [MySQL] 58,203 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new86.78 KB
PASSED: [[SimpleTest]]: [MySQL] 58,248 pass(es).
[ View ]

Status:Needs review» Needs work

The last submitted patch, D8-psr4-combined-moved-2083547-12.patch, failed testing.

Dreditor eat my notes but:

1) There are 2 plugin manager in your patch that don't exist anymore
2) I don't think the script belongs inside the patch. It's usefull for contrib but I don't think it belongs in core

3) I see some inline comments written as

/*
*
*/

we don't do that.

4) You're missing @file blocks
5) Some files don't end with a newline

Thanks!
What about the "big picture" of things?

I don't think the script belongs inside the patch. It's usefull for contrib but I don't think it belongs in core

The idea is to remove the script and other things in a follow-up.
Also, this script allows everyone to easily test this at home.

Alternatively, we could let the script live and remove it before we commit it to core.

There are 2 plugin manager in your patch that don't exist anymore

indeed.
- LayoutManager
- PluginUIManager

This is the evil commit:
https://github.com/donquixote/drupal/commit/6d0c876ff520f98d1b24130ae970...

I see some inline comments written as

/*
*
*/

we don't do that.

Not really. These are docblock for local variables, with /**, not /*.

+          /**
+           * @var DirectoryIterator $fileinfo
+           */
+          foreach (new DirectoryIterator($dir) as $fileinfo) {

The purpose is to tell IDEs like phpstorm about the type of a local variable, which it cannot or does not determine by itself.
This will enable it to identify typos in method names, invalid parameters, etc.
I found this being suggested somewhere on stackexchange/stackoverflow, I think.

If it is general consensus to not to this, then I am ok to remove it.
However, I don't think we can blindly apply the rules for inline comments, since this is not really an inline comment.
What we could do is remove it now, and discuss this as a general practice separately.

You're missing @file blocks

True.
I never liked @file. But I am going to add it.

Some files don't end with a newline

True.

Can you please answer in 1 comment and not 5. You have the habbit to do this in every issue and It's very disturbing and makes me want to ignore the issue even if I'm interested in it. I'm not qualified to give a global opinion I'm just trying to make the patch look reviewable for others.

StatusFileSize
new786.95 KB
PASSED: [[SimpleTest]]: [MySQL] 58,451 pass(es).
[ View ]
new83.81 KB
PASSED: [[SimpleTest]]: [MySQL] 58,216 pass(es).
[ View ]
new9.08 KB

Changes:
- rebase on latest 8.x
- fixed most issues raised by aspilicious in #17
- added some more comments
- changed git history

Cleaned-up history:
https://github.com/donquixote/drupal/compare/D8-psr4-combined-2083547-24
https://github.com/donquixote/drupal/compare/D8-psr4-combined-moved-2083...

NOTE: This is not a complete review; I had to stop after about 15 mins in, but pasting what I have so far since I'm not sure when I can next look at this.

Applied the patch to a separate D8 install and it's amazing how less "heavy" this feels now when poking around in any random module's directory. Great work!

One thing for the next re-roll is renaming "src" back to "lib" ... that never reached consensus in the other issue, and it's easier to leave it as-is than hold PSR-4 up on bikeshedding. Plus, I'm not sure it's actually less confusing to tell people "Sometimes OO code is found in lib and sometimes in src."

Also...

+++ b/core/authorize.php
@@ -23,7 +23,7 @@
-require_once __DIR__ . '/vendor/autoload.php';
+require_once __DIR__ . '/autoload.php';

In general, we should be looking at ways to make it as easy as possible to move back to Composer once they have PSR-4 support, since ideally we do not want to maintain this code long-term. That probably means making the changes to /core/vendor/autoload.php directly; not sure if we can do that, but that would eliminate a lot of busy-work from this patch and make it easier to review.

+++ b/core/autoload.php
@@ -0,0 +1,15 @@
+ * @file
+ * Drupal-specific autoload.php to be used instead of the one generated by

Probably also add a @todo: Remove this when PSR-4 support is added to Composer [link to pull request]

+++ b/core/includes/bootstrap.inc
@@ -2761,7 +2761,9 @@ function drupal_classloader($class_loader = NULL) {
-  $loader->add('Drupal\\' . $name, DRUPAL_ROOT . '/' . $path . '/lib');
+  $loader->addPsr4('Drupal\\' . $name . '\\', DRUPAL_ROOT . '/' . $path . '/lib/Drupal/' . $name);
+  $loader->addPsr4('Drupal\\' . $name . '\\', DRUPAL_ROOT . '/' . $path . '/lib');
+  $loader->addPsr4('Drupal\\' . $name . '\\', DRUPAL_ROOT . '/' . $path . '/src');

I don't really like this change; it exposes unnecessary implementation details to anyone working with the class loader. I'd rather just keep ->add() and use a parameter or something to denote PSR-4 if we must... ideally this wouldn't be necessary though, and add would "just work."

+++ b/core/lib/Drupal/Component/Plugin/Discovery/AbstractAnnotatedClassDiscovery.php
@@ -17,21 +17,7 @@
  * Defines a discovery mechanism to find annotated plugins in PSR-0 namespaces.
...
+abstract class AbstractAnnotatedClassDiscovery implements DiscoveryInterface {

Does this comment need to be updated to reflect this change?

I would also rather not rename this class if possible. I'm not sure that "Abstract" adds much to the meaning. If already we do this elsewhere, then ok.

+++ b/core/scripts/switch-psr4.sh

I don't see a reason to check this script into core. Does it have use once Drupal 8.0 is released? If not, let's not add it to the list of things we need to maintain, document, unit test, etc.

One thing for the next re-roll is renaming "src" back to "lib".

Fine with me. I am happy if I don't have to make that decision myself :)
This needs one change in the script. And then either now or later we can reduce the 3 paths per namespace to just 2.

In general, we should be looking at ways to make it as easy as possible to move back to Composer once they have PSR-4 support, since ideally we do not want to maintain this code long-term. That probably means making the changes to /core/vendor/autoload.php directly; not sure if we can do that, but that would eliminate a lot of busy-work from this patch and make it easier to review.

core/vendor/autoload.php is being overwritten on composer install, and also some other composer commands.
Having a separate core/autoload.php allows us to either use the composer loader, or switch to something else, at any time.
Even if Composer gets PSR-4, I outlined elsewhere that we can get significant performance improvements with a slightly modified loader - and still benefit from the data generated by Composer.

"eliminate a lot of busy-work"
https://github.com/donquixote/drupal/commit/5d208e4d8f865643cfa5b409a421...
I don't think it is that much.

For moving back, I personally think the core/autoload.php could stay, even if we swap the class loader back to Composer's.
The above diff does not move us too far away from Composer IMO.
Others may disagree. But then we'd have to find another way to activate our own loader instead of Composer's.

I don't really like this change; it exposes unnecessary implementation details to anyone working with the class loader. I'd rather just keep ->add() and use a parameter or something to denote PSR-4 if we must... ideally this wouldn't be necessary though, and add would "just work."

There are two things here.

Register core namespaces as PSR-4:
https://github.com/donquixote/drupal/commit/93a0226c10adb93aeb5cec4e62b0...
We could omit this commit, if we want to.
The motivation was perfomance: The new loader gives priority to classes registered with PSR-4. But we cannot do that via the composer.json yet. So, core namespaces would be registered as PSR-0 and suffer a penalty.

The new method addPsr4():
https://github.com/composer/composer/issues/1884#issuecomment-21624099
I sometimes prefer if others make those decisions for me :)

Does this comment need to be updated to reflect this change?

Yes it does!

I would also rather not rename this class if possible. I'm not sure that "Abstract" adds much to the meaning. If already we do this elsewhere, then ok.

Actually I did not rename, but refactor, so now we have 3: Component\..\AnnotatedClassDiscovery, Component\..\AbstractAnnotatedClassDiscovery, and Core\..\AnnotatedClassDiscovery.
https://github.com/donquixote/drupal/commit/baf4615cf989359e53784a75382e...
We don't actually need the Component one anymore, but if Component stuff is meant as a toolkit for others then why not, let's have an instantiable (non-abstract) discovery class in Drupal\Component.

In the original one I noticed that Core discovery inherits some attributes from Component discovery, which are then not used.
There were also a few other inconsistencies. I had the feeling it is nicer like this. It is not absolutely required, but makes the rest more manageable imo.

I don't see a reason to check this script into core. Does it have use once Drupal 8.0 is released? If not, let's not add it to the list of things we need to maintain, document, unit test, etc.

I am ok to remove this from the patch once everything else is settled.
For now I think it helps if people have the script available to try this at home.

We could also, instead of committing the patch, merge the history that I published on github. Then it will be easy to just skip the commit that adds the script.

The person who commits this into core should probably
- merge or apply the changes, with the script included, but with no files moved yet
- if it was done with merge, reset HEAD^ to get the script out of git.
- run the script
- remove the script
- maybe compare with the branch/tag that already has the files moved?
- commit

  1. No tests.
  2. +++ b/core/lib/Drupal/Core/Autoload/DrupalAutoloaderInit.php
    @@ -0,0 +1,62 @@
    +    unset($map['Drupal\Core']);
    +    unset($map['Drupal\Component']);
    +    unset($map['Drupal\Driver']);

    Just change the composer.json file and update.

  3. +++ b/core/lib/Drupal/Core/Autoload/DrupalAutoloaderInit.php
    @@ -0,0 +1,62 @@
    +  public static function getLoader() {
    [..]
    +    $coreDir = dirname(dirname(dirname(dirname(__DIR__))));

    Try public static function registerLoader($drupal_root) and then it will have a meaningful name and be a teeny bit more testable.

    I'm not entirely sure what you're supposed to do with the loader once this method has handed it back to you. It's registered it, and it's going to keep it in a static private property with no setter, so it's going to leak memory (the entire classmap as a giant array) if you ever call spl_autoload_register() again.

    What should happen is that you just do

    $loader = new DrupalClassLoader($drupal_root);
    $loader->addCore();
    $loader->addContrib();
    $loader->addSimpleTest();
    $loader->addPHPUnit(); // PHPUnit tests can have modules
    $loader->addOtherStuffICantThinkOfRightNow();
    $loader->register();
    // U R DUN.

    Each method essentially becomes a policy document on how Drupal is arranged.
  4. +++ b/core/lib/Drupal/Core/Autoload/DrupalAutoloaderInit.php
    @@ -0,0 +1,62 @@
    +    // @todo This can be updated once Composer provides a autoload_files.php.
    +    require $vendorDir . '/kriswallsmith/assetic/src/functions.php';

    This is causing me to be unable to run PHPUnit, which says this:

    $ ./vendor/bin/phpunit
    PHP Fatal error:  Cannot redeclare assetic_init() (previously declared in /vagrant/drupal/core/vendor/kriswallsmith/assetic/src/functions.php:20) in /vagrant/drupal/core/vendor/kriswallsmith/assetic/src/functions.php on line 26

@webchick:

$loader->addPsr4() I don't really like this change; it exposes unnecessary implementation details to anyone working with the class loader. I'd rather just keep ->add() and use a parameter or something to denote PSR-4 if we must... ideally this wouldn't be necessary though, and add would "just work."

Disagree. Anyone working with the class loader *should* know about the implementation details, because they matter. If you don't want that DX, then don't bake it in. See point 3 above.

Also: Cyclomatic complexity.

And while I'm here... #2025883: Drupal's PHPUnit bootstrap.php does not register module namespaces out of /core Drupal's kind of being designed without contrib in mind. Please make this new hot rod autoloader something that PHPUnit can use to bootstrap so we can write high-quality tests for our modules.

#2084513: Annotation class loading could be more elegant.
This will help to reduce the patch size a lot!
I am going to wait with the next patch until this is approved, and hopefully committed.

No tests.

What else do we need to test, that is not already covered by existing tests?
I do not disagree, just need some pointers.

Just change the composer.json file and update.

Sure, we can do that.
Just saying, in a follow-up we want to move this stuff back into composer.json and have Composer register it as PSR-4. But only once Composer actually supports PSR-4 in composer.json.

Try public static function registerLoader($drupal_root)

The motivation here was to have a direct copy of the stuff generated by Composer, with minimal modifications.
This will also make it easier to adapt to changes coming from Composer.

<blockquote>$loader->addCore();
$loader->addContrib();
$loader->addSimpleTest();
$loader->addPHPUnit(); // PHPUnit tests can have modules
$loader->addOtherStuffICantThinkOfRightNow();
$loader->register();</blockquote>

I say no.
Composer will generate 4 files in core/vendor/composer/.
- autoload_classmap.php
- autoload_namespaces.php
- include_paths.php
- autoload_files.php (since https://github.com/composer/composer/pull/2139)

We should use these files, so we don't have to babysit our DrupalAutoloaderInit when new libraries are added.
Also, this allows us to fully benefit from a composer-generated class map, if we want to.

Each method essentially becomes a policy document on how Drupal is arranged.

I don't get that one :)

Cyclomatic complexity

?
It is going to be reduced with #2084513: Annotation class loading could be more elegant..
Otherwise, what do you mean?

... PHPUnit ...

Is this a new problem introduced with this patch, or is it something that existed before?

Re: Tests....

Maybe I'm looking in the wrong place or applying the wrong patch, but D8-psr4-combined-2083547-24.patch doesn't have any tests in it, unit or otherwise.

Re: PHPUnit....

That issue I linked to has existed since I started delving into how to do PHPUnit on D8, for the past few months. The problem with re-loading assetic developed when I applied #24.

Re: Other stuff....

$loader->addCore();
$loader->addContrib();
$loader->addSimpleTest();
$loader->addPHPUnit(); // PHPUnit tests can have modules
$loader->addOtherStuffICantThinkOfRightNow();
$loader->register();

I say no.
Composer will generate 4 files in core/vendor/composer/.
- autoload_classmap.php
- autoload_namespaces.php
- include_paths.php
- autoload_files.php (since https://github.com/composer/composer/pull/2139)
We should use these files, so we don't have to babysit our DrupalAutoloaderInit when new libraries are added.
Also, this allows us to fully benefit from a composer-generated class map, if we want to.

That's fine for classloading during Drupal bootstrap without contrib. In fact, you just described addCore(). :-)

SimpleTest and PHPUnit both need a different class loading strategy, however, and they have divergent needs from each other and from Drupal bootstrap. You have SimpleTest classes and test modules, and you have PHPUnit test cases and support classes. These shouldn't be mapped by Composer into a file that gets loaded into memory on every page view. Similarly, contrib has to be discovered every time unless we're generating that and caching it somewhere. Each module also will (ideally) have its own PHPUnit and SimpleTest tests, each with their own hierarchy of support classes, and these must be bootstrapped or not, at different times.

Basically DrupalAutoloaderInit as it exists in #24 locks out these other strategies, since we can't pass it a loader with our special stuff already set up, and we can't access the loader once it's generated. Therefore we can't leverage the code for, eg, PHPUnit's bootstrap, which wants all of core and contrib and the various /tests namespaces to be loaded but not /Tests (SimpleTest).

In my ideal world, different bootstraps will be able to use the same code for similar class loading functionality with extra code only addressing their special cases, especially if some of the cases are PSR-0 and some are PSR-4. In that ideal world, I can pick which areas to load in some high-level way so that a) it's easier to test and b) it's easier to work with. Then much of the logic moves into a classloader class where everyone can get at it. That class could be a classloader factory or a subclass of ClassLoader or whatever makes the most sense, but it's not a hard-wired static and I can inject its dependencies, such as a pre-existing loader and a Drupal root path.

Does this make me a dreamer? :-)

Maybe I'm looking in the wrong place or applying the wrong patch, but D8-psr4-combined-2083547-24.patch doesn't have any tests in it, unit or otherwise.

This observation is correct. There are no (new) tests in this patch.
A lot of the tests that already exist have been testing class loading and discovery with PSR-0. These same tests do now cover class loading and discovery with PSR-4.

This being said, it is not unlikely that we need new tests for some things. I just need some help to identify these!

That's fine for classloading during Drupal bootstrap without contrib. In fact, you just described addCore(). :-)

I think I now get a better idea what you are after :)

<?php
$loader
->addCore();
$loader->addContrib();
$loader->addSimpleTest();
?>

But, why do these need to be methods on the loader?
At the moment there are different places in core which add stuff to the class loader. But the loader itself only has a few generic methods to add classes and namespaces.
We could attempt to centralize this stuff some more, but none of it needs to live directly on the loader class. E.g. we could get very close to what you suggest with a wrapper class. But then again, I still don't think this is something we should do :)

In fact I did follow this approach in a previous patch (the "Krautoload" stuff). But then found it to be overengineered.
Krautoload had a wrapper with stuff like addDrupalExtension(), etc.

The other question is, how much of that is actually related to or required for the switch from PSR-0 to PSR-4?
Anything that is not, should rather be discussed in a follow-up.
(this may also apply to some stuff that I put into the patch myself)

Basically DrupalAutoloaderInit as it exists in #24 locks out these other strategies, since we can't pass it a loader with our special stuff already set up, and we can't access the loader once it's generated.

How is that?
autoload.php does return a loader object, and then we can do with it what we want..

eg, PHPUnit's bootstrap

I know very little about PHPUnit's integration in Drupal. I hope someone else can comment on this :)
Or I will study it somewhere.

Basically DrupalAutoloaderInit as it exists in #24 locks out these other strategies, since we can't pass it a loader with our special stuff already set up, and we can't access the loader once it's generated.

How is that?
autoload.php does return a loader object, and then we can do with it what we want..

How do I use DrupalAutoloaderInit to get ahold of the loader we've already generated?

There are only two ways:

1) I can't.

2) I can have it generate a whole new one to register and return to me, gobbling up memory and time.

That's because it's a static singleton hidden in a class without an accessor.

This is relevant to the PSR-4 stuff, because it would be great to not have to re-write all the PSR-4 stuff for the PHPUnit loader, and just use the code you've made.

But, why do these need to be methods on the loader?

That's just to illustrate something easy. It could be a factory/wrapper/what-have-you. The point is that others can use your PSR-4 code if you let them.

I know very little about PHPUnit's integration in Drupal.

PHPUnit's integration with Drupal is pretty straightforward, and we want to keep it that way. :-) The thing is, we want to have as few dependencies as possible, so we don't want to bootstrap Drupal. However, we want to bootstrap Drupal's namespaces in a classloader so that the tests can find their classes in use statements, without loading config or a database or anything.

Sorry if that wasn't clear.

BTW, the patch needs a re-roll.

+  public static function getLoader() {
+    if (null !== self::$loader) {
+      return self::$loader;
+    }

How is this "a class without an accessor" ?

StatusFileSize
new749.09 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch D8-psr4-combined-moved-2083547-33.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new43.45 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]

Status:Needs review» Needs work

The last submitted patch, D8-psr4-combined-moved-2083547-33.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new749.09 KB
PASSED: [[SimpleTest]]: [MySQL] 58,603 pass(es).
[ View ]
new43.45 KB
PASSED: [[SimpleTest]]: [MySQL] 58,171 pass(es).
[ View ]

Neither patch applies to the latest 1024ba83931516f38f916f99189fd6d0cc01dabe, or 490e8753dbb70b8abce5607536230341b9fccdc7 which was committed before your post.

Then again, I'm stupid. :-)

StatusFileSize
new750.59 KB
PASSED: [[SimpleTest]]: [MySQL] 58,096 pass(es).
[ View ]
new40.02 KB
PASSED: [[SimpleTest]]: [MySQL] 58,387 pass(es).
[ View ]

It is green for a reason :)

Anyway, this needs a reroll after #2087551: ViewsHandlerDiscovery: Remove $this->pluginNamespaces setup from constructor. and #2087725: Block module tests in tests/lib/Drupal instead of tests/Drupal went in.

I also changed the history:
https://github.com/donquixote/drupal/compare/D8-psr4-combined-2083547-37
https://github.com/donquixote/drupal/compare/D8-psr4-combined-moved-2083...

The most controversial stuff, imo, is in the class loader. So I put these in the end, to make them easier to omit on merge.

Status:Needs review» Needs work

The last submitted patch, D8-psr4-combined-moved-2083547-37.patch, failed testing.

Status:Needs work» Needs review

Why this sort of change to method names?

-        $this->registerNamespaces($namespaces_before);
+        $this->registerNamespacesPsr4($namespaces_before);

I don't want to know or care that it's specifically PSR-4 when I'm using the code.

You pretty much have to care which PSR version you're loading with.

That said, the reason you'd *need* to care is if you're writing or maintaining the autoloader, or if you're trying to autoload something outside of Drupal.

The reason for the rename was that we can support PSR-0 and PSR-4 in parallel for a transition phase.
The class loader will keep add() and addPsr4().
But DrupalKernel can be cleaned up either now or in a follow-up, so the registerNamespacesPsr4() could be renamed to registerNamespaces().

This being said:
registerNamespacesPsr4() has a different behavior than registerNamespaces(). This could be a reason to keep the name.
But then again, we have other things that are not being renamed in the patch. E.g. the "container.namespaces" service now has a different meaning / behavior, but it is not renamed to container.namespaces.psr4.

I'd like to have some more opinions about this.

@Mile23 (#41):

You pretty much have to care which PSR version you're loading with.

That said, the reason you'd *need* to care is if you're writing or maintaining the autoloader, or if you're trying to autoload something outside of Drupal.

The reason we have to care is that we build the array of namespace directories, and we need to do that either with PSR-0 or with PSR-4 in mind. We do that both for the autoloader and for class discovery.

This applies whether we maintain our own autoloader or not.

As much as possible I think documenting the method's parameters and behavior in the code comments is preferred to appending 'psr4' this to the method name.

@pwolanin: ok, i will change this.

Something else:
http://getcomposer.org/doc/articles/scripts.md
The post-autoload-dump could be used to avoid the API break by having our own custom core/autoload.php.
Instead, we can register a script that will replace Composer's freshly generated core/vendor/autoload.php with our own.
This will make this entire thing even more reversible.

So, we will have a reversible temporary Drupalism.

Pasting this review. once again didn't get all the way through, but there's more here now.

+++ b/core/authorize.php
@@ -25,7 +25,7 @@
-require_once __DIR__ . '/vendor/autoload.php';
+require_once __DIR__ . '/autoload.php';

I pushed back on this because we ultimately do want to rely on Composer's autoloader and not do our own thing. Changing this here, then changing it back later, means that modules out there providing standalone scripts for Drupal (e.g. Drush) need to update their code twice.

donquixote said that he feels even if we go to full-on Composer, we might still want to keep this file around though, even as just a "dumb wrapper" (e.g. https://github.com/donquixote/drupal/commit/ace538d9078a4b095ccdf406b4fc...). This would provide the flexibility to advance the needs of our classloader for other things without being blocked on Composer. (For example, the (for-now) Drupal module namespace-specific predictor logic that helps improve performance: #2058867-7: Consider introducing a native ClassLoader for performance and PSR-4.

+++ b/core/autoload.php
@@ -0,0 +1,15 @@
+use Drupal\Core\Autoload\DrupalAutoloaderInit;
+
+require_once __DIR__ . '/lib/Drupal/Core/Autoload/DrupalAutoloaderInit.php';
+
+return DrupalAutoloaderInit::getLoader();

This layer of indirection looks weird, but it is standard with what Composer's doing, minus the 98237982173891279323 on the class name which we don't need here since There Can Be Only One™!

+++ b/core/includes/bootstrap.inc
@@ -2761,7 +2761,10 @@ function drupal_classloader($class_loader = NULL) {
-  $loader->add('Drupal\\' . $name, DRUPAL_ROOT . '/' . $path . '/lib');
+  $loader->addPsr4('Drupal\\' . $name . '\\', array(
+    DRUPAL_ROOT . '/' . $path . '/lib/Drupal/' . $name,
+    DRUPAL_ROOT . '/' . $path . '/lib',
+  ));

I still don't really like this; I feel like PSR-0/PSR-4 should be a flag on the add() function instead.

This would be akin to in Drupal having both a $node->saveWithRevisions() and a $node->saveWithoutRevisions(). (as well as a loadWith/WithoutRevisions() and a deleteWith/WithoutRevisions) It complicates the API.

+++ b/core/lib/Drupal/Core/Autoload/ClassLoader.php
@@ -0,0 +1,377 @@
+ * ClassLoader implements a PSR-0 class loader
...
+ * See https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-0.md

And PSR-4.

The rest of the docblock (code example, etc.) needs to be updated for PSR-4, too.

+++ b/core/lib/Drupal/Core/Autoload/ClassLoader.php
@@ -0,0 +1,377 @@
+ * This class is loosely based on the Symfony UniversalClassLoader.
+ *
+ * @author Fabien Potencier <fabien@symfony.com>
+ * @author Jordi Boggiano <j.boggiano@seld.be>

If only loosely based, then probably remove the @author attribution.

Also, we just got done switching the default auto-loader from Symfony to Composer, in large part due to performance. Why did we not start with Composer's autoloader instead?

(From an IRC discussion, it sounds like we *did* and Composer's autoload originated from Symfony's originally. We should probably change the comments here to make it more clear where this came from.)

+++ b/core/lib/Drupal/Core/Autoload/ClassLoader.php
@@ -0,0 +1,377 @@
+  // PSR-4
...
+  // PSR-0

(nit-pick) Here and in general, all comments need to have periods at the end.

+++ b/core/lib/Drupal/Core/Autoload/ClassLoader.php
@@ -0,0 +1,377 @@
+  private $prefixLengthsPsr4 = array();
+  private $prefixDirsPsr4 = array();
+  private $fallbackDirsPsr4 = array();
...
+  private $prefixesPsr0 = array();
+  private $fallbackDirsPsr0 = array();
...
+  private $useIncludePath = false;
+  private $classMap = array();

Each of these properties should be documented in full PHPDoc way. (@var type and description)

+++ b/core/lib/Drupal/Core/Autoload/ClassLoader.php
@@ -0,0 +1,377 @@
+  const PREDICTOR_INDEX = 9;

Comment? What is this for? Why 9?

+++ b/core/lib/Drupal/Core/Autoload/ClassLoader.php
@@ -0,0 +1,377 @@
+  public function getPrefixes() {
...
+  public function getPrefixesPsr4() {

All of these methods need PHPDoc, too.

+++ b/core/lib/Drupal/Core/Autoload/ClassLoader.php
@@ -0,0 +1,377 @@
+  public function getFallbackDirs() {
+    return $this->fallbackDirsPsr0;
+  }
+
+  public function getFallbackDirsPsr4() {
+    return $this->fallbackDirsPsr4;

once again, I'd prefer this to be like:

<?php
function getFallbackDirs($psr0 = TRUE) {
  if (
$psr0) {
    return
$this->fallbackDirsPsr0;
  }
  else {
    return
$this->fallbackDirsPsr4;
  }
}
?>

Or am I missing a cluestick?

+++ b/core/lib/Drupal/Core/Autoload/ClassLoader.php
@@ -0,0 +1,377 @@
+  public function add($prefix, $paths, $prepend = false) {
...
+  public function addPsr4($prefix, $paths, $prepend = false) {

Same here. These functions really seem to be almost identical. They should be combined into one with the psr4 vs. psr0 parts handled in a condition.

+++ b/core/lib/Drupal/Core/Autoload/ClassLoader.php
@@ -0,0 +1,377 @@
+      if ('\\' !== $prefix[$length - 1]) {
+        throw new \Exception("A non-empty PSR-4 prefix must end with a namespace separator.");
+      }

Why do we do this sort of validation here but not in the psr0 equivalent?

+++ b/core/lib/Drupal/Core/Autoload/ClassLoader.php
@@ -0,0 +1,377 @@
+  public function setUseIncludePath($useIncludePath) {
...
+  public function getUseIncludePath() {

(minor) It's weird to have two verbs right next to each other in a method name. Should likely be "setIncludePath" and "getIncludePath" (and the property named just $includePath). Or is this something we inherited from Composer? If so, then leave it.

+++ b/core/lib/Drupal/Core/Autoload/ClassLoader.php
@@ -0,0 +1,377 @@
+    if (isset($class[self::PREDICTOR_INDEX])) {

Please comment how on earth this predictor stuff works. :)

+++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.php
@@ -47,7 +53,11 @@ class AnnotatedClassDiscovery extends ComponentAnnotatedClassDiscovery {
+      if ('/' !== $subdir[0]) {
+        $subdir = '/' . $subdir;
+      }
+      $this->directorySuffix = $subdir;

Can you comment this check? In general, an inline comment every "stanza" of code (5-6 lines) would be really nice. There's some heavy stuff in here.

+++ b/core/scripts/run-tests.sh
--- /dev/null
+++ b/core/scripts/switch-psr4.sh

If we're keeping this in core (which once again I don't believe we should) then it needs to pass the docs gate and right now it does not.

Status:Needs review» Needs work

Also, in IRC catch said he was willing to commit this in advance of Composer being ready (hooray!) but wants to do it as a locally patched version of the Composer in /vendor/ to ensure we're tracking with what upstream is doing.

Also, in IRC catch said he was willing to commit this in advance of Composer being ready (hooray!) but wants to do it as a locally patched version of the Composer in /vendor/ to ensure we're tracking with what upstream is doing.

There are pros and cons to this, and I would like some more information before I am convinced.

Option A, as in the patch in #37:
We fork the proposed classloader from the PR to Drupal\Core\Autoload\..

Pros:
- Allows custom performance tweaks (if we want that)
- Still uses all the generated autoload data from core/vendor/composer/*
- Can use a generated classmap for core and vendor, if the local developer runs autoload-dump.
- mostly reversible to a pure-Composer solution, once Composer gets PSR-4 (only the scripts that include core/autoload.php would need to be changed).

Cons:
- needs a custom core/autoload.php to be used instead of core/vendor/autoload.php
- not as easy to pull updates from Composer upstream. (but how often do we actually do this?)

Option B, as proposed by catch:
We fork the PR to a locally patched version of Composer.

Pros:
- Easier to pull updates from Composer upstream. (but how exactly would that work?)
- We don't need a custom core/autoload.php (although it might still be useful)
- reversible to a pure-Composer solution, once Composer gets PSR-4.

Cons:
- !!! Anyone who wants to run composer dump-autoload on D8 locally needs a modified version of Composer !!!
- This means, you need a modified Composer to generate a classmap for core + vendor locally.
- If we ditch the custom core/autoload.php and AutoloaderInit, then we need some other place to e.g. register Core namespaces as PSR-4 instead of PSR-0.
- Using a patched Composer is more fragile than simply using the ClassLoader from the PR. The effects of having the modified ClassLoader within Composer still need to be evaluated.
- unclear which branch we should be following. So far there is only a PR, that could change in unpredictable directions.

Title:PSR-4: Putting it all together.PSR-4: Putting it all together

For the time being, the top priority for this issue is to make it work and get this committed before Prague next week, where we'll be training a whole crapload of new developers on D8. Whatever approach gets us there fastest is what I support.

To that end, I don't care if we build in future-optimization points for lookup, etc. That we could change later and affect about 15 people, not the 1800 who will be in Prague. We can also fork and unfork Composer without affecting module developers. I'd even accept a small performance regression for the time being if it gets this, as long as we get something committed in the next 72 hours. (After that, pre-sprints start ramping up.)

Also to that end, addPsr4() doesn't bother me. Also, it appears that Composer is going to use the Psr4 suffix (https://github.com/composer/composer/pull/2121#discussion_r6345487) so there's no good reason for us to not follow suit (especially since using pure-composer again in the future is the goal; or at least my goal). Also note Jordi's follow-up there asking for a migration path :-), after which he's fine going ahead and merging that PR anyway, so I don't expect us to be in a forked state (either forked state) for very long.

So yes, let's take the approach that gets this issue committed by Friday, whatever that is.

Priority:Normal» Critical

Bumping to critical. I agree committing this by Friday would be the most amazing thing in the world.

I withdraw my objections to the naming (or maybe I can make them over there, but in any case...). If that's the way Composer is headed, let's do it.

Also in IRC, catch voiced support for doing the simplest thing that could possibly work to get PSR-4 support into Composer (and likely Composer would like this better too). That means deferring things like performance optimizations until post-commit (this allows us to try and get them accepted upstream, too).

So...
A: "I want to get to Rome, is this faster by bus or by train?"
B: "I don't care, use whichever is faster"
This is the feedback I was hoping for :)

So yes, let's take the approach that gets this issue committed by Friday, whatever that is.

The fastest approach is if Crell and/or catch reply to the concerns mentioned in #48 and we come to an agreement of "do X now, do Y later". Once we have that agreement, I will post a new patch + history asap.

The other thing to agree on is how to finally get this in:
I'd suggest, for whoever commits this (catch)
- merge or cherry-pick all or part of the history on github, WITHOUT any files moved around. (because any patch with so many files moved around, or even a PR, will quickly get out of date and generate many conflicts). OR just apply the patch (again, without files moved around). Note: cherry-pick or rebase will get us a nicer commit history.
- run the core/scripts/switch-psr4.sh
- commit the move
- rebase --interactive, and remove the commit that added the script
- rebase --interactive, to fine-tune commit messages (optional)
- run all tests locally, to be sure
- push upstream

EDIT: multiple changes to the recommended procedure on git.

Issue summary:View changes

link to scripts issue instead of annotation namespaces are stupid issue

- !!! Anyone who wants to run composer dump-autoload on D8 locally needs a modified version of Composer !!!

This is a concern for me, since we don't know the timeframe for Composer merging PSR-4 support. I really don't want to make keeping our dependencies up to date harder than it is now. So "whatever gets us to Rome fastest but doesn't break composer.phar install". :-)

Status:Needs work» Needs review
StatusFileSize
new762.83 KB
PASSED: [[SimpleTest]]: [MySQL] 57,919 pass(es).
[ View ]
new46.15 KB
PASSED: [[SimpleTest]]: [MySQL] 57,913 pass(es).
[ View ]

@pwolanin (#40):

Why this sort of change to method names?

-        $this->registerNamespaces($namespaces_before);
+        $this->registerNamespacesPsr4($namespaces_before);

I don't want to know or care that it's specifically PSR-4 when I'm using the code.

I am going to keep it this way.
The class loader has a method addPsr4() for PSR-4, in addition to add() for PSR-0. This was decided with Seldaek on the Composer PR.
I personally support this decision 100%, and could try to justify it here. But that does not even matter, because this is what Composer will do - so we can as well skip this debate.

The only purpose of registerNamespaces() is to call add() on the class loader. And the only purpose of DrupalKernel::addNamespacesPsr4() is to call addPsr4() on the class loader. So imo we should keep this.

(and anyway, this is going to be easy to change afterwards)

@webchick (#46):
I added a lot of docblock comments.
A lot of stuff happens "because that's how Composer does it". But I am going to comment anyway, because there are sometimes interesting reasons for it.

Why do we do this sort of validation here but not in the psr0 equivalent?

The boring reason is just backwards compatibility for PSR-0.

The more interesting reason:
A prefix has a different role in PSR-0 than in PSR-4.
In each, we check if the fully-qualified class name (= class including the namespace) begins with the given prefix.
In PSR-0 we don't need the prefix anymore after that step.
In PSR-4, after finding a match, we cut the prefix off the class name, and append what remains to the registered directory.
So, it really matters how much we cut off. E.g. for Drupal\Core\Foo\Bar, we want to cut off the Drupal\Core\\, so Foo\Bar remains. If we'd only cut off Drupal\Core, then \Foo\Bar would remain. So this matters a lot more for PSR-4.

Besides that, for PSR-0, we can also have prefixes that end with '_' instead of '\\'.

It's weird to have two verbs right next to each other in a method name.

I fully agree. Composer..
But I also think we can all survive it pretty well :)

New patch history:
D8-psr4-combined-2083547-54
D8-psr4-combined-moved-2083547-54

I specifically made the history so that any commit someone may not like can be removed with interactive rebase.

Suggested plan for committing this is still as in #52 (and now in the issue summary).

> This is a concern for me, since we don't know the timeframe for Composer merging PSR-4 support.

Soon! https://github.com/composer/composer/pull/2121#issuecomment-24387396 said "I'm fine merging this, but there is one thing blocking it in my head and that is the migration path from target-dir+psr-0 to only psr-4 autoloading. Basically I would like to see a PR for #913 before this one is merged". Well, https://github.com/composer/composer/pull/2279 is a PR just for that :)

StatusFileSize
new761.32 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch D8-psr4-replace-autoload-moved-2083547-57_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new44.63 KB
PASSED: [[SimpleTest]]: [MySQL] 58,609 pass(es).
[ View ]

As in #45, we can avoid having to change scripts to use core/autoload.php.
Instead, we use a post-autoload-dump script to replace the core/vendor/autoload.php generated by composer.

New git history:
D8-psr4-replace-autoload-2083547-57
D8-psr4-replace-autoload-moved-2083547-57

Status:Needs review» Needs work

The last submitted patch, D8-psr4-replace-autoload-moved-2083547-57.patch, failed testing.

Status:Needs work» Needs review

I kinda expected the moved version to fail. Review the other one.

Yes please. I'd like to get some feedback before I reroll this the nth time.
I'd also like some feedback about the proposed way to commit this (see the issue summary).

StatusFileSize
new762.06 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
new44.63 KB
PASSED: [[SimpleTest]]: [MySQL] 58,620 pass(es).
[ View ]

Reroll.
Just to get rid of that red.

New git history:
D8-psr4-replace-autoload-2083547-60
D8-psr4-replace-autoload-moved-2083547-60

Status:Needs review» Needs work

The last submitted patch, D8-psr4-replace-autoload-moved-2083547-60.patch, failed testing.

https://github.com/donquixote/drupal/commit/53ce202d4ffb85921b6b7e2b942d...
This commit may cause false positives on simpletest discovery.
Simpletest should NOT scan the module_dir/tests/.. directory for simpletest classes.

Besides, there are a few quirks with regard to PHPUnit integration, which lead me to the decision to leave anything in (module dir)/tests untouched for now, and do this in a separate issue.
#2094843: Port PHPUnit test classes in (module dir)/tests/ to PSR-4

Status:Needs work» Needs review
StatusFileSize
new754.11 KB
PASSED: [[SimpleTest]]: [MySQL] 58,541 pass(es).
[ View ]
new44.88 KB
PASSED: [[SimpleTest]]: [MySQL] 58,649 pass(es).
[ View ]

Interdiff steps from unsquashed history:

This commit was removed:
autoload: Register test namespaces as PSR-4.
Postponed to #2094843: Port PHPUnit test classes in (module dir)/tests/ to PSR-4.

Change in simpletest.module, where test classes are discovered:
Simpletest should NOT scan (module dir)/tests for test classes
(module dir)/tests/ is reserved for phpunit tests, simpletest is not scan it for discovery.

Edit core/phpunit.xml.dist:
phpunit exclude folders to respect new PSR-4 paths.
!!! Please review this carefully. !!!

-      <exclude>./modules/config/tests/config_test/lib/Drupal/config_test</exclude>
-      <exclude>./modules/views/tests/views_test_data/lib/Drupal/views_test_data</exclude>
+      <exclude>./modules/config/tests/config_test/lib</exclude>
+      <exclude>./modules/views/tests/modules/views_test_data/lib</exclude>

There is something fishy with views_test_data. views/tests/views_test_data does not exist, it is views/tests/modules/views_test_data. So the original exclude is bogus.

Edit core/tests/bootstrap.php
autoload: register modules as PSR-4 in tests/bootstrap.php
This is needed so that phpunit can find the moved module class files.

Edit the switch-psr4 script:
do not move class files in 'tests' yet.
Postponed to #2094843: Port PHPUnit test classes in (module dir)/tests/ to PSR-4.

--------------

Simplified history, where the above fixes are squashed, rebased on newest 8.x:
D8-psr4-replace-autoload-2083547-64
D8-psr4-replace-autoload-moved-2083547-64

Edit core/phpunit.xml.dist: phpunit exclude folders to respect new PSR-4 paths.

See #2056607: /core/phpunit.xml.dist only finds top-level contrib modules

I only left in ./modules/config/tests/config_test/lib/Drupal/config_test because PHPUnit crashes otherwise. I think it's really a problem with how config's tests are structured, and the exclusion is a band-aid type solution.

Ideally, there would be no excludes at all, since our file structure will take care of it.

Edit the switch-psr4 script: do not move class files in 'tests' yet.

Yes, at least run the tests first, after everything else has moved. Kinda why they're there. :-)

Change in simpletest.module, where test classes are discovered:
Simpletest should NOT scan (module dir)/tests for test classes
(module dir)/tests/ is reserved for phpunit tests, simpletest is not scan it for discovery.

+1 in theory, but I think that needs to stay so the web-based runner can work. SimpleTest grabs PHPUnit tests and adds them to a 'PHPUnit' group for display on the Testing page.

+1 in theory, but I think that needs to stay so the web-based runner can work.

Nope.
Prior to the proposed patch, simpletest does not scan the (module dir)/tests/Drupal/(module name)/Tests/.. PSR-0 folders. It did only scan (module dir)/lib/Drupal/(module name)/Tests/..

In earlier versions of the patch, I did wrongly add (module dir)/tests/Drupal/(module name)/Tests/.. to the scanned folders, which would result in a change of behavior, which is not intended.

The fix in #64 is merely to revert this mistake.

Squashed with the other commit for simpletest discovery, you get
https://github.com/donquixote/drupal/commit/8b51b00de67175ea25cd0c72037b...
which should, as I see it, reproduce the same behavior as before the patch, just with support for (module dir)/lib as a PSR-4 directory.

Issue tags:+8.x-alpha4

Ok, we ran out of time to get this in prior to DrupalCon Prague. Let's please not miss the chance to get it in before alpha4. This is one of the biggest DX wins we could possibly do, and BADCamp's kind of the last big hurrah this year for 100s of people to port their modules.

#7: D8-custom-psr4-combined-moved-7.patch queued for re-testing.

Note that the resolution to this issue by way of adding PSR-4 support to composer is also very very close to completion. See:

https://github.com/composer/composer/pull/2121
https://github.com/composer/composer/pull/2295

#69:
The main obstacle atm is we want to have PSR-4 support in composer.json, and I would like to see the AutoloadGenerator being refactored first. See https://github.com/composer/composer/pull/2121#issuecomment-25892369

Adding the addPsr4() mechanics to the loader without allowing to define PSR-4 in composer.json would be quite pointless for projects other than Drupal, esp since it adds a small performance penalty to PSR-0 class loading (because PSR-4 is first).

Issue summary:View changes

Updated issue summary.

#70 Why can't we hardcode PSR4 paths for each component? Generally speaking we should not be blocked by other projects.

#71: ?
My post #70 only refers to #69, the idea that we wait for Composer. I wanted to dampen (but not totally suffocate) the hopes for a very quick resolution of the PR in Composer.

If we do our own loader, as proposed in the patch in #64, then there is no obstacle anywhere.

Yes sorry, I followed this issue but not read #69 and #70 carefully, so I was thinking we changed the direction and astonished.

We should move forward with our own customized-Composer autoloader for now, in whatever way makes it easiest for us to swap back to the vanilla Composer once upstream has native PSR-4 support.

Are there any concerns with the patch in #64? (The first one, presumably, since the second almost certainly doesn't apply now, by design.)

Issue tags:-8.x-alpha4

:(

Issue summary:View changes

describe pending decisions in issue summary

Issue summary:View changes
StatusFileSize
new43.47 KB
PASSED: [[SimpleTest]]: [MySQL] 58,725 pass(es).
[ View ]
new759.59 KB
PASSED: [[SimpleTest]]: [MySQL] 58,757 pass(es).
[ View ]

Issue summary:View changes

"Pending decisions" are pretty much settled, or noone really has any strong opinion. Having them open only slows us down.

  1. +++ b/core/lib/Drupal/Core/Autoload/ClassLoader.php
    @@ -0,0 +1,522 @@
    +   *   E.g. a possible value of this variable could be:
    +   *
    +   *   array(
    +   *     'Dr' => array('Drupal\Core\\' => 12),
    +   *     'Dm' => array(
    +   *       'Drupal\Component\\' => 17,
    +   *       'Drupal\number\\' => 14,
    +   *     ),
    +   *     'S' => array('Symfony\\' => 8),
    +   *   )

    How does a prefix of Dm result in handling Drupal\Component?

  2. +++ b/core/lib/Drupal/Core/Autoload/ClassLoader.php
    @@ -0,0 +1,522 @@
    +   * Registers a set of PSR-0 directories for a given prefix,
    +   * replacing any others previously set for this prefix.

    Too long shortdesc. (Unless that is borrowed from Composer, in which case I'll make an exception.)

  3. +++ b/core/lib/Drupal/Core/Autoload/GeneratorScript.php
    @@ -0,0 +1,28 @@
    +  public static function postAutoloadDump(Event $event) {
    +    $core_dir = dirname(dirname(dirname(dirname(__DIR__))));
    +    // Copy Drupal's
    +    copy($core_dir . '/autoload.drupal.php', $core_dir . '/vendor/autoload.php');
    +  }

    Copy Drupal's what? Who? Sentence seems incomplete.

  4. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -636,7 +637,28 @@ protected function getModuleFileNames() {
    +  protected function getModuleNamespacesPsr4($moduleFileNames) {
    +    $namespaces = array();
    +    foreach ($moduleFileNames as $module => $filename) {
    +      // @todo Remove lib/Drupal/$module, once the switch to PSR-4 is complete.
    +      $namespaces["Drupal\\$module"][] = DRUPAL_ROOT . '/' . dirname($filename) . '/lib/Drupal/' . $module;
    +      $namespaces["Drupal\\$module"][] = DRUPAL_ROOT . '/' . dirname($filename) . '/lib';
    +    }
    +    return $namespaces;
    +  }

    Wise move on that @todo. That should make it a bit less bumpy.

The main point here is how we're layering in the Drupal logic temporarily until Composer sorts itself out. I'm comfortable with the mechanism used for that here as a temporary measure. I didn't look at every line since most lines come from Composer anyway, so are not ours to review.

So, simple fixes and then we should commit this post-haste. We're running out of time in which to do so reasonably (if we haven't already passed that point).

Status:Needs review» Needs work

Not being able to review and change status at the same time is itself a critical bug. :-/

How does a prefix of Dm result in handling Drupal\Component?

The algorithm picks the characters at position [0] and [9].
The number 9 is a strategical choice for the Drupal use case, where we assume that a lot of namespaces differ in this character. It is also the first string position where "Drupal\Component" differs from "Drupal\Core".

There are many options for the implementation of the internal lookups. This particular one has proven to be fast in benchmarks, and it works.

I am sure there could be an even faster algorithm, but this needs more work and thinking, so it should rather be in a follow-up.

Benchmarks in #2058867-7: Consider introducing a native ClassLoader for performance and PSR-4.
Symfony PSR-0 loader: 648.619 for findFile()
PSR-4 loader proposed for Composer: 277.369 for findFile()
Modified PSR-4 loader in Drupal: 105.088 for findFile()
(the smaller the better, but don't ask if these are ms per ?)
The difference gets bigger if we install more (contrib) modules.

Too long shortdesc. (Unless that is borrowed from Composer, in which case I'll make an exception.)

The original description in the Composer PSR-0 loader is "Registers a set of classes, replacing any others previously set." This is confusing, since we register namespaces, not classes. And it fails to distinguish PSR-0 from PSR-4. This is why I changed it.

I am going to change it again to split it to 2 lines. See the following patch.

Copy Drupal's what? Who? Sentence seems incomplete.

Fixed in following patch.

https://github.com/donquixote/drupal/compare/D8-psr4-norebase-2083547-81
has all the changes.
(look at the last 3 commits)

I am going to rebase this to merge the commits into older ones.

Status:Needs work» Needs review
StatusFileSize
new43.93 KB
PASSED: [[SimpleTest]]: [MySQL] 58,858 pass(es).
[ View ]
new763.25 KB
FAILED: [[SimpleTest]]: [MySQL] 58,957 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 82: D8-psr4-moved-2083547-82.patch, failed testing.

Status:Needs work» Reviewed & tested by the community

The main patch is passing; the fail is only on the mass-move patch. The way the patch is written though a mass-move is unnecessary, because both PSR-0 and PSR-4 style are supported for modules.

I am therefore marking this RTBC, as this gets us PSR-4 support for modules without breaking nearly every patch in the queue. I recommend scheduling a follow-up Git-merge-based mass-migration for sometime *after* the next alpha (probably right after it). Removing the PSR-0 for modules afterward should be trivial. That also gives the existing experimental contribs one alpha in which to catch up.

The small patch includes the script to automate moving everything else; I'm fine with having that in core temporarily until we actually move things, then we can take it back out. Or see the summary, but we should try to not break every patch in the queue without advanced notice.

I was able to reproduce the failed test for the big patch on simplytest.me, but then on my own test environment the test passed.

I assume this is some bogus unrelated to this issue.

In general we want the big patch to pass. It proves that the small patch is a sufficient preparation for the move.
But in this case I'd say go ahead.

Crell is right about contrib, this will allow contrib modules to start using PSR-4. And then let's do the "big move" in core at a strategical time.

As mentioned in the issue summary, the small patch can either be applied as a patch, or merged in via https://github.com/donquixote/drupal/compare/D8-psr4-2083547-82 to keep the git history.

Just to say i'm really happy to see this patch coming in core. Directories structure is actually very displeasing to work with, for now. I made my first very simple module to test D8 API and it's insane to have my little Block saying "hello" five ( ! ) levels down. I know the idea was to use PSR-0 standards for interoperability purposes but such a directory structure means clearly one thing : PSR-0 does not fit all Drupal needs, for sure...

|-- config
|   `-- foomodule.settings.yml
|-- foomodule.info.yml
|-- foomodule.module
|-- foomodule.routing.yml
|-- lib
|   `-- Drupal
|       `-- foomodule
|           |-- controller.php
|           |-- FoomoduleConfigForm.php
|           `-- Plugin
|               `-- Block
|                   `-- foomoduleBlock.php

Assigned:Unassigned» catch
Issue tags:+alpha target

THIS MAKES ME SO HAPPY!!!! :D :D :D

So as much as I would love to just commit this right effing now, it really does feel like it needs catch's sign-off here. So assigning to him. But, if we're getting uncomfortably close to alpha5 and this still isn't in yet, I'll probably eat some karma. ;)

Targeting for next alpha. For real this time. :D

Is there decent, concise documentation somewhere on how PSR-4 works and where to use it vs. PSR-0?

@Mile23:
The current version of the proposed spec is here:
https://github.com/php-fig/fig-standards/blob/master/proposed/psr-4-auto...

The intended meaning has never changed. The wording is being discussed for months.
You can search for "PSR-4" in this Google group: https://groups.google.com/forum/#!forum/php-fig

The only practical change here is that modules lose 2 levels of otherwise-empty directories under $module/lib. Otherwise there's no difference. (Unless you had underscores in your class names, which almost no one does anymore.)

Then the answer is: "No, there is not concise documentation on how PSR-4 works and where to use it vs. PSR-0."

Could someone who understands it please write that handbook page, and/or make a patch with a docblock describing it, so that it can be easily found? It's a Drupalism. Folks used to PSR-0 need to know whether/how to amend their thinking.

Assigned:catch» Unassigned

PSR-4 isn't a Drupalism, but it's an as-yet-unadopted standard.

I'm not really comfortable committing this while the FIG group is still wringing their hands about this. I have absolutely no confidence that they'll actually get this done by the time we'll release any longer, and while I find empty directories distasteful that's the strongest word I can find about them (I know other people feel a lot more strongly about it). Unassigning myself.

Haven't reviewed the patch recently.

I'm also still concerned that this includes a lot of changes that won't necessarily make it into the Composer PR that eventually gets merged - but possibly I'm wrong on that and most of this has already been reviewed there and kept up to date?

In #1498574: [policy, no patch] Require PHP 5.4 we are so eager to blaze a road -- no other major project requires PHP 5.4 (phpunit alone will but that's really a developer-facing project). Yet, here we are hesitant to forge ahead and perhaps show the way -- only to the php-fig. We have no problems of running into problems with any number of hosting companies/situations and yet we have a problem over a standards group that spends its time standardizing spaces? Give me a break and get this in.

Some standards bodies require two independent implementations in the wild before a standard is officially adopted. :-) I haven't been following the PHP-FIG discussion, but I don't see anything fundamentally wrong with using a as-yet-unadopted standard in real world code. This happens all the time, especially in the web world! Anyway, just my 2 cents.

PSR-4 ( ...) it's an as-yet-unadopted standard.

We have already a yet-adopted standard with PSR-0. As far as i understand, first goal of PRS-4 in Drupal is to get rid of those ugly nested directories, so it seems not so important if PSR-4 becomes an adopted standard or not; as long as it plays well with already in place PSR-0 autoloader.

1) This is not the place to discuss the pros/cons of PSR-4 itself. There is already copious discussion in #1971198: [policy] Drupal and PSR-0/PSR-4 Class Loading about that. Don't fork that discussion here, and please read it before you post here in confusion.

2) PSR-0 was published by the PHP Standards Group (since renamed PHP-FIG) in 2009. It's very widely adopted.

3) The practical definition of PSR-4 ("PSR-0 with fewer directories and no underscores") hasn't changed since, um, March. The wording has been rejiggered many times, but nothing has changed that would impact us. Composer is going to have PSR-4 support; donquixote is working on that, too. I think most (or at least a lot) of the code here is from his PR to Composer upstream which has gotten a nod from Jordi already. (I believe the only thing still needed there was adding support for using PSR-4 from the composer.json file, which doesn't much matter to us in this case.)

4) FIG doesn't have a "needs 2 implementations" requirement, although speaking informally it certainly wouldn't hurt. :-)

5) As the co-sponsors of PSR-4 in FIG, I am confident that we'll get it approved prior to 8.0. However, every day we don't commit it is another day that people are writing "your first Drupal 8 module" blog posts and articles with something we know is going to be wrong soon. So we need to either pull the trigger and commit or pull the trigger and commit to not doing so, no matter what FIG does. Dries has already indicated he wants to move forward here (see the referenced previous discussion thread), so we should take option 1.

(I agree with catch that the extra directories at present are "distasteful" at worst; but as long as we're using a standard autoloading mechanism rather than a custom one I don't care greatly either way. Plus, being the first project to adopt a new PSR, one that we're helping to write, has a certain cache.)

Needs docs. That's all I'm saying. Apparently Drupal would need some stuff under PSR-0 file system, and some stuff under PSR-4 file system. That's the Drupalism: Which and where?

You cant be sure if something gets adopted or gets stuck forever.
Moving on now would mean we are introducing something that might end up to be Drupalism. And come on.. We push this and risk, for what?

So people that port their modules feel better?
We have worse underlying issues than that, which are actually technical problems.

This issue should be really postponed till PSR-4 is adopted.

All that was already discussed here, as Crell said. https://drupal.org/node/1971198

something that might end up to be Drupalism

Well, i could say that creating something like "/mymodule/lib/Drupal/mymodule/class.php" is some kind of drupalism too, in a simple php framework, all you may need with PSR-0 is "/vendorName/packageName/class.php

It's 6 of one, 12/2 of another.

Adopt it and get it over with. It can change later if it's the Worst Thing Ever.

Docs. That's all we need. :-)

Ok, looks like catch doesn't want to pull the trigger. Guess that'll be me then. :) However, I can't in good consciousness commit this today, because I'm going to be largely offline, but I do plan to commit this by the end of the week so it makes alpha5.

Mile23, could you be more specific about a) what docs are missing and b) where you expect them? The only parts I see that are obviously missing docs in a quick scan through are the function in the shell script to put everything to PSR-4 (which is part of why I don't want to actually commit that to core).

In general, though, where we're heading toward is PSR-4 being the preferred method of class structuring within modules (since obviously they are already part of "Drupal" and "modulename" this information being re-encoded in directories is superfluous), and PSR-0 being the preferred method everywhere else (e.g. core/lib, vendor/, etc.). I agree with Crell though about splitting out the "let PSR-4 be possible" from the "actually move everything and break every patch in the queue."

webchick: I ask because I'd like to see a page describing the rules for using PSR-4 vs. PSR-0 within Drupal and contrib.

Here are some questions to answer in such a document:

Should /core/lib be PSR-4? Should core modules be PSR-4? Can contrib modules use PSR-0? Should /tests be PSR-4? Should contrib /tests be PSR-0?

"Let PSR-4 be possible" is good for this patch and for alpha release, but not so great as a policy.

@Mile23:
The patch makes PSR-4 possible only for (module dir)/lib, but not anywhere else.

For core/lib and other places, we have a de facto "constrained PSR-0" which means PSR-0 without underscores in the class name. This way, core/lib with PSR-0 is equivalent to core/lib/Drupal/Core and core/lib/Drupal/Component with PSR-4. For performance reasons, this patch actually treats core/lib/Drupal/Core and core/lib/Drupal/Component as PSR-4 directories. This does not require any moving around of class files.

(module dir)/tests is not covered by this patch, since I figured it would need more discussion. See #2094843: Port PHPUnit test classes in (module dir)/tests/ to PSR-4.

In a future clean-up step, after a transition phase, PSR-4 will become the only thing for (module dir)/lib.

I agree this should all be explained in a doc page somewhere on drupal.org.

Status:Reviewed & tested by the community» Needs work

Sorry for this incredibly nit-picky review but we do have a docs gate and one of the ways people learn our coding standards is by copying existing code.

  1. +++ b/core/lib/Drupal/Core/Autoload/ClassLoader.php
    @@ -0,0 +1,528 @@
    +   * @param bool $prepend
    +   *   Whether to prepend the directories.

    Should mention that is optional and defaults to FALSE.

  2. +++ b/core/lib/Drupal/Core/Autoload/ClassLoader.php
    @@ -0,0 +1,528 @@
    +   * @param bool $prepend
    +   *   Whether to prepend the directories

    Should mention that is defaults to FALSE.

  3. +++ b/core/lib/Drupal/Core/Autoload/ClassLoader.php
    @@ -0,0 +1,528 @@
    +   * @throws \Exception

    SHould be a empty line above.

  4. +++ b/core/lib/Drupal/Core/Autoload/ClassLoader.php
    @@ -0,0 +1,528 @@
    +   *
    +   * @param string $prefix
    +   *   The classes prefix.
    +   * @param array|string $paths
    +   *   The location(s) of the classes.
    +   * @param string $prefix
    +   *   The prefix.
    +   * @param array|string $paths
    +   *   The PSR-0 base directories

    Duplicated params

  5. +++ b/core/lib/Drupal/Core/Autoload/ClassLoader.php
    @@ -0,0 +1,528 @@
    +   *   The prefix/namespace, with trailing '\\'
    ...
    +   *   The PSR-4 base directories

    Missing fullstops.

  6. +++ b/core/lib/Drupal/Core/Autoload/ClassLoader.php
    @@ -0,0 +1,528 @@
    +   * @throws \Exception

    Should be an empty line above.

  7. +++ b/core/lib/Drupal/Core/Autoload/ClassLoader.php
    @@ -0,0 +1,528 @@
    +  /**
    +   * Turns on searching the include path for class files.
    +   *
    +   * @param bool $useIncludePath
    +   */
    +  public function setUseIncludePath($useIncludePath) {
    +    $this->useIncludePath = $useIncludePath;
    +  }

    This can be use to turn on or off... perhaps instead of "Turns on..." we could use "Toggles..."

  8. +++ b/core/lib/Drupal/Core/Autoload/ClassLoader.php
    @@ -0,0 +1,528 @@
    +   * Can be used to check if the autoloader uses the include path to check
    +   * for classes.

    "Can be used to check" should be "Checks"

  9. +++ b/core/lib/Drupal/Core/Autoload/ClassLoader.php
    @@ -0,0 +1,528 @@
    +   * @param bool $prepend
    +   *   Whether to prepend the autoloader or not

    Missing fullstop and should mention that is it optional and defaults to FALSE.

  10. +++ b/core/lib/Drupal/Core/Autoload/DrupalAutoloaderInit.php
    @@ -0,0 +1,115 @@
    +   *   The name of the class to load.
    +   *   If this is anything other than "Drupal\Core\Autoload\ClassLoader", this
    +   *   method will do nothing.

    Unnecessary line break after "load." Should be reflowed.

  11. +++ b/core/lib/Drupal/Core/Autoload/DrupalAutoloaderInit.php
    @@ -0,0 +1,115 @@
    +    // Temporarily register an autoload callback to load the
    +    // Drupal\Core\Autoload\ClassLoader class.
    +    // This could be done with a simple require_once, but instead it is done
    +    // in the same way as Composer does it.

    Unnecessary line break after "class."

  12. +++ b/core/lib/Drupal/Core/Autoload/DrupalAutoloaderInit.php
    @@ -0,0 +1,115 @@
    +    // Register core namespaces as PSR-4 instead of PSR-0,
    +    // to let the autoloader handle them with priority.
    +    // These mappings need to be equivalent with the PSR-0 mappings specified in
    +    // composer.json. This is only possible because class names in Drupal core
    +    // do not contain underscores.

    Comment needs reflowing - unnecessary line breaks after "PSR-0," and "priority."

  13. +++ b/core/lib/Drupal/Core/Autoload/GeneratorScript.php
    @@ -0,0 +1,28 @@
    +   * Called after composer autoload-dump.
    +   * Replaces the core/vendor/autoload.php with a Drupal-specific autoload.php.

    Looks like these lines should be the other way around and with a space between them.

  14. +++ b/core/lib/Drupal/Core/Autoload/GeneratorScript.php
    @@ -0,0 +1,28 @@
    +} ¶

    EmpSpace at EOL

  15. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -636,7 +637,28 @@ protected function getModuleFileNames() {
    +   * Gets the namespaces of each enabled module,
    +   * each with their PSR-4 directories.
    +   *
    +   * @param array $moduleFileNames
    +   * @return array

    Not quite Drupal standards here

  16. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -636,7 +637,28 @@ protected function getModuleFileNames() {
    +   * Gets the namespaces of each enabled module,
    +   * each with its PSR-0 directory.

    Should be on one line

  17. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -636,7 +637,28 @@ protected function getModuleFileNames() {
    +   * @param array $moduleFileNames
    +   * @return array

    Needs an empty line between them and should have some description for the param at least.

Be sure to see #1971198: [policy] Drupal and PSR-0/PSR-4 Class Loading for the full pros and cons of this change. Edit: if they are actually documented there, that is.

(Cross-post from #1971198: [policy] Drupal and PSR-0/PSR-4 Class Loading.)

While PSR-4 reduces the complexity of the directory structure, it may also increase the learning curve. I'd love to see a better overview of the pros and cons of PSR-4 (included the issue summary). I just want to make sure everyone understand the trade-off we're making.

Issue tags:-alpha target

Over in #1971198-209: [policy] Drupal and PSR-0/PSR-4 Class Loading, Dries noted that he'd like to see a summary of pros and cons before pulling the trigger on this. This is fair enough. However, it likely means this won't make alpha5 (scheduled for Monday). So removing the target tag for now. :(

I just want to clarify my position on PSR-4 since my review in #105 could be interpreted as tacit support. I'm not sure that there huge benefits to made by introducing PSR-4 especially since the point of PSR-0 was to make it easy to share code between PHP projects and this could be seen as a step back. I realise that many Drupal modules are not coded in such a way but this might make that dream less likely. OTOH I concede that whilst Drupal core is not using composer correctly and puts all of vendor and components in the same git repository we're not exactly making it easy even with PSR-0. I'm with @catch on this - empty directories are distasteful but I think we have more pressing issues.

@alexpott (#105): Thanks for the review, working on it!

@alexpott (#109): I think this issue is the wrong place.
Here a response to the interoperability argument: #1971198-210: [policy] Drupal and PSR-0/PSR-4 Class Loading

StatusFileSize
new46.29 KB
PASSED: [[SimpleTest]]: [MySQL] 57,931 pass(es).
[ View ]
new768.35 KB
FAILED: [[SimpleTest]]: [MySQL] 57,907 pass(es), 1 fail(s), and 7 exception(s).
[ View ]

Fixing the stuff mentioned in #105 (alexpott).
Disclaimer:
- DrupalKernel has some existing code style issues. I am fixing it only in places that are relevant for this patch.
- Drupal\Core\Autoload\ClassLoader is copied/adapted from a Composer PR (and from the existing Composer class loader) and has some lowerCamel method parameters. I am not going to change those.

1, 2, 9: Added (optional). I won't add a default value in the comment.
https://drupal.org/node/1354 says:

Optional parameters are indicated by (optional); include information about the default value only if it is not obvious from the function signature.

3, 6: Empty line added.

4: Duplicate params probably from merge/rebase. Fixed.

5, 9: Missing fullstops added.

7, 8: Documentation copied from Composer. I disagree with it myself, but not sure I want to change it.

10, 11, 12, 16: Thought this linebreak would improve readability. But fixed to conform with conventions.

13: Lines switched and space.

14: Space at EOL fixed.

15: This was because I copied the lowerCamelCase variable name from getModuleNamespaces(). Changing now in both places.

17: getModuleFileNames() was there before the patch. But fixing anyway.

----------------

Besides, I am confused why these patches (e.g. #76, #82) never failed.
In DrupalAutoloaderInit::getLoader() there is

<?php
   
unset($map['Drupal\Core']);
    unset(
$map['Drupal\Component']);
    unset(
$map['Drupal\Driver']);
   
$loader->setPsr4('Drupal\Core\\', $baseDir . '/core/lib/Drupal/Core');
   
$loader->setPsr4('Drupal\Component\\', $baseDir . '/core/lib/Drupal/Component');
   
$loader->setPsr4('Drupal\Driver\\', $baseDir . '/drivers/lib/Drupal/Driver');
?>

but $baseDir is not defined anywhere.

I am fixing this, but it is still weird.

------------

EDIT: Forget this patch and the github stuff!!
Going to fix.

Status:Needs work» Needs review

StatusFileSize
new46.2 KB
PASSED: [[SimpleTest]]: [MySQL] 57,883 pass(es).
[ View ]
new768.26 KB
PASSED: [[SimpleTest]]: [MySQL] 58,010 pass(es).
[ View ]

Issue summary:View changes

Issue summary update:
Branch renamed to https://github.com/donquixote/drupal/compare/8.x-psr4

Status:Needs review» Reviewed & tested by the community

Bot can object. I still want to get this into the next alpha.

Status:Needs review» Reviewed & tested by the community

I quite clearly marked this RTBC. It is in the previous comment. Why wasn't the node updated?

Doc should go into issue summary of the meta issue, and a change notice.
And maybe into a doc page.
Either way I don't see it should stop this being committed - ? The meta issue was open long enough to debate this..

So +1 for RTBC (if bot is happy)

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 111: D8-psr4-moved-2083547-113.patch, failed testing.

Priority:Critical» Normal
Parent issue:» #1971198: [policy] Drupal and PSR-0/PSR-4 Class Loading

This cannot be RTBC while there is no consensus.

We have a critical issue to decide if PSR-4 is even desirable for D8, this is an implementation issue.

Status:Needs work» Needs review
StatusFileSize
new46.75 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch D8-psr4-2083547-120.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Ok, I get out of the rtbc-or-not discussion. I am far too convinced of this thing.

I noticed two flaws in the patch in core/tests/bootstrap.php, that were not detected by testbot.
Interdiff to #113: https://github.com/donquixote/drupal/commit/9443ec43970de8c12916d9d7e688...

History: https://github.com/donquixote/drupal/compare/D8-psr4-2083547-120
(I don't put a "moved" patch this time, the one in #113 is good enough)

Issue tags:+alpha target

The PSR-4 vote is under way (unanimous so far apart from one project), and due to expire on December 3. Since this falls within the timeframe of our next alpha release (Dec 16), tentatively tagging as an alpha target. For hopefully the last effing time. :P

Issue summary:View changes

Please don't miss the follow-up:
#2094843: Port PHPUnit test classes in (module dir)/tests/ to PSR-4
I postponed PHPUnit stuff in tests/Drupal/modulename/Tests/, because I was not sure whether there could be cross-namespace classes, e.g. in tests/Drupal/othermodule/, or just tests/OtherNamespace.

PSR-4 is now officially official:

https://groups.google.com/forum/#!msg/php-fig/L8oCDQCzDcs/Z-6_9U0hpJIJ

Let's do this.

Post moved to meta.

I have moved the discussion into the meta.

120: D8-psr4-2083547-120.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 120: D8-psr4-2083547-120.patch, failed testing.

The last submitted patch, 120: D8-psr4-2083547-120.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new49.18 KB
FAILED: [[SimpleTest]]: [MySQL] 59,617 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
new779.15 KB
FAILED: [[SimpleTest]]: [MySQL] 59,354 pass(es), 2 fail(s), and 2 exception(s).
[ View ]

https://github.com/donquixote/drupal/tree/D8-psr4-2083547-139
https://github.com/donquixote/drupal/tree/D8-psr4-moved-2083547-139

I improved the switch-psr4.sh, to make it easier to reroll patches.
(assuming these are in a local git branch)

Patch reroll survival guide:

git checkout 8.x-local-dev
git fetch origin
git rebase origin/8.x
php core/scripts/switch-psr4.sh
git add -A .
git commit -m"Move locally added files to their new PSR-4 location"

There might be special cases where this does not work, but it did seem ok for basic local modifications.
Optionally this can be spiced up with interactive rebase with squash/fixup, to make it appear as if locally added files were directly created in the PSR-4 path, not in a PSR-0 path.

EDIT: See also this answer on stackoverflow.

The last submitted patch, 139: D8-psr4-2083547-139.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 139: D8-psr4-moved-2083547-139.patch, failed testing.

StatusFileSize
new52.56 KB
PASSED: [[SimpleTest]]: [MySQL] 59,664 pass(es).
[ View ]
new808.39 KB
FAILED: [[SimpleTest]]: [MySQL] 59,474 pass(es), 1 fail(s), and 2 exception(s).
[ View ]

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 142: D8-psr4-phpunit-moved-2083547-142.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new54.78 KB
PASSED: [[SimpleTest]]: [MySQL] 59,626 pass(es).
[ View ]
new810.4 KB
PASSED: [[SimpleTest]]: [MySQL] 59,720 pass(es).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 145: D8-psr4-phpunit-moved-2083547-145.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

This patch just got a whole lot easier:

http://seld.be/notes/psr-4-autoloading-support-in-composer

Thanks for sticking with it, Andreas!

(Although we should now redo this patch to just use Composer's PSR-4 support directly and call it a day, IMO.)

I think we should keep the current patch as-is.
Jordi explicitly warned in his blog post to not jump on the new thing too quickly. The main reason, we want to wait a little (e.g. one month) until everyone has updated to the new version of Composer.

The current patch works, it is tested, and we can go with it any moment. It even has a performance benefit.
Going back to the Composer loader can be done any time in the future, even post-beta, without breaking any APIs.

As soon as Dries makes a decision, I want this to go in asap, which I think will be easiest if we stick with the current patch.

I did a new benchmark, this time making sure that xdebug is disabled
(thanks @msonnabaum for his healthy doubtfulness).

It turns out that the performance difference between our Drupal-optimized loader and the Composer loader is not as big as originally thought. Apparently I did something wrong with previous benchmarks. Probably because I had xdebug enabled? I don't want to think that but this is the most likely explanation.

The surprising thing: With the Composer loader we can have ~50x strpos() (with 100 modules registered) whereas with the Drupal-optimized loader it would be around ~2x strpos() for the same number of modules. But these additional strpos() make a much smaller difference if xdebug is disabled. It is still measurable, and it becomes significant if we crank up the number of modules to 200 or 300. But in general it is far too small to make a fuss about it.

@Crell:
I agree now we should switch to the native Composer loader, for the reasons you mentioned. This means that for some intermediate time, people who did not update their local Composer will not be able to run composer install or composer generate classmap. But this is going to be a temporary thing.

Status:Needs work» Needs review
StatusFileSize
new62.43 KB
PASSED: [[SimpleTest]]: [MySQL] 59,557 pass(es).
[ View ]
new841.23 KB
PASSED: [[SimpleTest]]: [MySQL] 59,592 pass(es).
[ View ]

Removing the custom ClassLoader stuff, and using the Composer ClassLoader from the new Composer release with PSR-4 instead.

This is all rebased, so don't even start to compare with previous patches.

Cleaned-up history:
https://github.com/donquixote/drupal/compare/D8-psr4-composer-2083547-151
https://github.com/donquixote/drupal/compare/D8-psr4-composer-moved-2083...

Status:Needs review» Needs work

The last submitted patch, 151: D8-psr4-composer-moved-2083547-151.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 151: D8-psr4-composer-moved-2083547-151.patch, failed testing.

Status:Needs work» Needs review

Is there discussion about contrib modules needing external libraries like Graph API : #2168563: How to load graph and graph-uml php libraries?

afaik src/ will have contribs own code and lib/ could contain external libraries but I don't want those in there as I expect competing contrib modules will break when both require/have the same libraries.

My current code seems not the best way :-/

<?php
function graphapi_loader() {
 
$loader = drupal_classloader();
 
$loader->add('Fhaculty', DRUPAL_ROOT . '/sites/all/libraries/graph/lib');
 
$loader->add('Fhaculty', DRUPAL_ROOT . '/sites/all/libraries/graph-uml/src', TRUE);
}
?>

What do I miss?

@jbrown is #157 a reply to my question in #156 ?

If so that's not the answer as we are talking Drupal 8 _and_ we want to support all Drupal site builders (without shell access).

It triggered me to check for https://drupal.org/project/libraries

Thanks anyway :)

I am using Composer Manager with Drupal 8. It does tend to require shell at the moment.

Contrib using Composer is an entirely separate question from this thread. Please keep this thread focused.

@Crell that was exactly my question in #156: do we have discussions for contrib using php libraries. Both composer_manager and libraries have discussions about WSOD and hesitations about how to solve that.

Please direct me to a core issue for contrib using libraries :)

Priority:Normal» Critical
Status:Needs review» Needs work
Issue tags:+beta blocker

We made a decision in #1971198: [policy] Drupal and PSR-0/PSR-4 Class Loading to go forward with PSR-4 for modules. That means this issue is now a beta blocker. The work to change the directories around, however, should be deferred to a separate, postponed, "beta blocker" critical issue that we will look at later once the beta is in better shape.

Note that Composer was just updated in #2175447: Update composer to latest version, which pulled in the upstream PSR-4 code, so that can be factored out of this patch.

Status:Needs work» Needs review
StatusFileSize
new51.38 KB
PASSED: [[SimpleTest]]: [MySQL] 59,894 pass(es).
[ View ]
new848.92 KB
PASSED: [[SimpleTest]]: [MySQL] 59,874 pass(es).
[ View ]

I tested this patch on contrib and it works and I love it :).
To help contrib converting I suggest adding the following directories to the switch-psr4 script:

process_extensions_base_dir(DRUPAL_ROOT . '/modules');
process_extensions_base_dir(DRUPAL_ROOT . '/profiles');

Or better... make it an optional argument for the script

@aspilicious:
The idea is that you only convert those modules to PSR-4 that you maintain yourself.
For other modules, you wait until that maintainer has published a PSR-4 version.

This is possible because of the intermediate time where both PSR-0 and PSR-4 will be supported likewise.

So what we need is a script or drush command that will port a specific module or module package to PSR-4.

This being said: Porting a contrib module to PSR-4 manually is going to be a lot less work than porting core.

@aspilicious: How about this?
https://github.com/donquixote/drupal/commit/ce1e366446ad7a73939c4ffac928...

It takes any number of directories as commandline parameters. For each parameter that is an existing directory, it will process every module found within this directory.

If no parameters are specified, the core modules and profiles will be processed instead.

E.g.
php core/scripts/switch-psr4.sh modules/contrib/devel
or
php core/scripts/switch-psr4.sh modules/contrib

Lovely :), would be a true help for contrib that is already ported.

@Dries (#163):
Does the decision confirm the exact flavor of PSR-4 that is implemented in the patch?
This would be:

  1. PSR-4 class files go into lib/, not src/.
    core/modules/action/lib/{Drupal/action → }/ActionListController.php
  2. PhpUnit tests that used to be in tests/Drupal/.. go to tests/lib/..
    core/modules/action/tests/{Drupal/action/Tests → lib}/Menu/ActionLocalTasksTest.php
  3. Class files in core/lib/ stay where they are, but they are registered to the class loader / in composer.json as PSR-4 instead of PSR-0.
      "autoload": {
        "psr-4": {
          "Drupal\\Core": "core/lib/Drupal/Core",
          "Drupal\\Component": "core/lib/Drupal/Component",
          "Drupal\\Driver": "drivers/lib/Drupal/Driver"
        },
  4. 3rd party libraries or cross-namespace classes go into separate subfolders outside of lib/ or tests/lib/, and are out of scope for this issue.

If this is 100% approved, then i can go ahead and implement this change in D7 contrib (xautoload).
If there is still any doubt about any of these points, then I would wait until it is settled in D8.

I am not trying to stir up a new debate, just want to have an explicit final blessing.

The outlined plan in #171 looks good to me.

Though as mentioned in #1971198-300: [policy] Drupal and PSR-0/PSR-4 Class Loading, I would recommend to use ./src for the PSR-4-autoloaded source code.

The original decision to go with "lib" was made at a time when .module files still contained >90% of the source code, so the argument that "src" would be confusing had a point back then. But today, the only remaining code in .module files are hook implementations. Your actual source code lives in classes.

The vast majority of packages on the net are using ./src and ./vendor, which is a clean, concise, and appropriate naming:

PSR-4-autoloaded source code of my Drupal module (auto-registered)
./src/
./src/Entity/Thingie.php

PSR-N-autoloaded vendor libraries
./vendor
./vendor/kertz/twitteroauth/twitteroauth/twitteroauth.php

Note: Registering additional namespaces for additionally supplied vendor libraries that are shipped with a module is not possible in HEAD yet, but that is a mandatory requirement for D8, because maintainers of contributed modules cannot expect that all users have access to Composer (or know how to use it). Easily possible, just needs to get done.

Sounds like there was bike-sheding going on with lib vs src as mentioned in #25 . Though FWIW I'm for src too.

Before this thread gets all bikesheddy again:
https://piratenpad.de/p/drupal-psr4-lib-vs-src

A quick scan of https://github.com/search?q=psr-4&ref=cmdform&type=Code shows src/ wins.

I'd really love to get the support for PSR-4 in sooner than later (of course postponing the widescale moving of files until closer to beta). I'm currently trying to write #2194001: PSR-0 / PSR-4 class relocating and it'd be great to not have to re-do work.

I think that #174 is just reopening the bikeshed :) Just saying, same old debate but in another place. That said I wouldn't mind if the folder was "src" in modules.

Issue summary:View changes

I'm not sure what this issue is waiting for.

Is it a review of #164?
I did have a bit of a look over it, but there's been so much discussion that it's not easy to see why things have been done a particular way. Is everything in the patch necessary immediately? e.g. could the drupal_get_path() changes be made in a separate issue to reduce the noise? That could presumably be committed either before or after this patch. This doesn't need to be held up on lib vs src, since it should be fairly easy to switch from one to the other before the patch is committed.

Is it a discussion on lib vs src?
There's been a lot of that, sumarised to a point in #1971198-151: [policy] Drupal and PSR-0/PSR-4 Class Loading and #2083547-174: PSR-4: Putting it all together. General opinion seems to be that src is preferable, but lib is easier (mainly to avoid the discussion). If anyone has strong opinions (including 'whatever, just commit it'), please add them the document linked in #174.

Is it the spec being signed off?
By which I mean an answer to #171, including an explict lib vs src call.

Is it a review of #164?

The patch is certainly far from being "overreviewed" - so go ahead!

Is everything in the patch necessary immediately? e.g. could the drupal_get_path() changes be made in a separate issue to reduce the noise?

If you want to look at smaller chunks to review, simply go to https://github.com/donquixote/drupal/compare/D8-psr4-2083547-164.
The goal of this patch, as I see it, is to prepare the transition to PSR-4, so that all that is left is executing a script.
This is why even if the second patch I post with each comment is not being committed, I still want it to be green, to show that executing the script is safe.

The script does not help us with the drupal_get_path() to __DIR__ stuff.

Is it a discussion on lib vs src?

Is it the spec being signed off?

I think I also prefer src now. But I've done so much work over the last 8 months that was then left idle, that I really would prefer a final ok before investing yet more time into it.

but lib is easier (mainly to avoid the discussion)

While lib is easier, src is going to be more transparent, especially in D7 contrib (xautoload) where a number of modules already picked up PSR-0 with lib and even yet another pattern based losely on PEAR (underscores to directory separators), and support for all of this this will have to live on alongside PSR-4 for backwards compatibility.
Having PSR-4 stuff in a separate folder will help to reduce confusion. I even have some contrib and local modules where the folder is named "psr4", to make this distinction even more visible.

While lib is easier, src is going to be more transparent, especially in D7 contrib (xautoload) where a number of modules already picked up PSR-0 with lib and even yet another pattern based losely on PEAR (underscores to directory separators), and support for all of this this will have to live on alongside PSR-4 for backwards compatibility.
Having PSR-4 stuff in a separate folder will help to reduce confusion.

D7 contrib can re-shuffle as needed for D8, because that's the magic of autoloading. The underscores thing is part of PSR-0: https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-0.md#u...

I thought the idea was that we could mix them up, essentially that a failed attempt to load using PSR-0 strategy would result in PSR-4 attempt (or vice-versa).

The underscores thing is part of PSR-0

xautoload happens to have a variation of this underscore thing with flat directories (kind of PSR-4 with underscores), that is not compatible with PSR-0, and which this module will need to continue to support for backwards compatibility.

It also would have to support PSR-4 in /lib/ if I had rushed to implement that. But I wisely decided to make this an explicit opt-in where the module has to explicitly specify this directory.

Either way, this is all technically covered.
The only difference is how confusing it will be to look at a /lib/ folder in D7 contrib and not knowing whether it will be PSR-0 or PSR-4. This is why I said, using /src/ instead of /lib/ for PSR-4 will be a good thing for D7 contrib.

It also would have to support PSR-4 in /lib/ if I had rushed to implement that. But I wisely decided to make this an explicit opt-in where the module has to explicitly specify this directory.

OK, I guess I don't understand what's going on here at all, because the pattern thus far has been: Make a standardized directory and it gets autoloaded because yay standardization. But you seem to be saying that I can dump my classes into either of two directories and it loads a different way based on which one. But the difference between the two names expresses exactly zero of this context, because there's no way in which 'src' means 'psr-4.' Furthermore, there's no way for me to specify a classmap in my module, which might go a long way towards dealing with edge cases for strange autoloading needs.

And on top of that, we'll now have two unit test directories: src_tests/ and lib_tests/, one each for a different autoloading mechanism.

In an ideal world, we'd have something like an autoload.json file per module that would specify all this. psr-4 autoloading in source/, psr-0 in thingie/, tests in psr-0 under stuff/junk/mytests/, and use this classmap for my ultra-funky library that I made a classmap for. Useful defaults if you don't find the json file would be src/ and tests/ both using psr-4.

In an IDEAL world, Drupal would just use Composer and we'd be done.

Quick reroll of #164:
https://github.com/donquixote/drupal/compare/D8-2083547-184-psr4-lib

This is still using /lib/, and none of the requested changes to #164 have been made.

StatusFileSize
new53.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch D8-2083547-185-psr4-src.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new899.51 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,054 pass(es), 5 fail(s), and 2 exception(s).
[ View ]

Moving from /lib/ to /src/:
https://github.com/donquixote/drupal/compare/D8-2083547-185-psr4-src
https://github.com/donquixote/drupal/compare/D8-2083547-185-psr4-src-moved

Interdiff commit from #184:
https://github.com/donquixote/drupal/commit/20c32d390060c0aec5e42585e197...

@sun suggested a while ago to port one module to PSR-4 as an example. I agree with this idea, but I am going to do this in a subsequent patch.

The last submitted patch, 185: D8-2083547-185-psr4-src.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 185: D8-2083547-185-psr4-src-moved.patch, failed testing.

185: D8-2083547-185-psr4-src.patch queued for re-testing.

The last submitted patch, 185: D8-2083547-185-psr4-src.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,549 pass(es).
[ View ]
new848.73 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,543 pass(es), 4 fail(s), and 2 exception(s).
[ View ]

Let's try it again.

@sun: You suggested to move one module to PSR-4 already. Which one would you suggest?

Status:Needs review» Needs work

The last submitted patch, 190: D8-2083547-190-psr4-src-moved.patch, failed testing.

I was thinking of a module that is (1) small and (2) barely touched by anyone currently. My favorite candidate would be xmlrpc.module.

Status:Needs work» Needs review
StatusFileSize
new49.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,690 pass(es).
[ View ]
new50.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,591 pass(es).
[ View ]
new899.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,638 pass(es).
[ View ]

I agree with earlier comments that src/ vs lib/ doesn't really provide any useful information. How about psr4/ for the directory name?
ignore me

Can someone please explain to me why we're still debating this? #175 is a data-driven method of resolving this bikeshed by asking "off the island" what others use. Overwhelmingly, it is /src for psr-4 code. Unless you can trump that with other hard facts/data, let's please just get this patch done.

StatusFileSize
new49.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,065 pass(es).
[ View ]
new57.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,052 pass(es).
[ View ]
new900.6 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,104 pass(es).
[ View ]

They are all green!!! Congrats @donquixote! Not sure exactly how to test this or if it needs testing? Let me know what I can do.

I've read through the code in the two smaller patches and nothing really stood out. The dirname(__FILE__) should be speedier I bet vs drupal_get_path() calls. And I love the shorter paths! woot!

Status:Needs review» Reviewed & tested by the community

Great work, @donquixote.

The idea is to commit the patch with the -shortcut name suffix now — it refers to "Shortcut module", the only module being converted in that patch.

That gets the necessary framework changes in place, and it allows contrib to start using PSR-4 immediately. (It's only core that wants to schedule its full switch for all core modules.)

Shortcut module is a good sample, because it has (1) regular OO code, (2) an entity/plugin, (3) Simpletest tests, (4) PHPUnit tests, and its queue has less then 50 issues in total, of which only a handful were recently active, so it also allows us to see how re-rolling will work out.

One remaining question is how to commit this. Imo, it could be useful to merge instead of just applying the patch.
https://github.com/donquixote/drupal/compare/D8-2083547-197-psr4-src-sho...

*finally*

Hiding all other patches then..