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

CommentFileSizeAuthor
#235 D8-2083547-233-psr4-src-shortcut-squashed.patch66.85 KBdonquixote
#197 D8-2083547-197-psr4-src-moved.patch900.6 KBdonquixote
#197 D8-2083547-197-psr4-src-shortcut.patch57.79 KBdonquixote
#197 D8-2083547-197-psr4-src.patch49.66 KBdonquixote
#194 D8-2083547-194-psr4-src-moved.patch899.05 KBdonquixote
#194 D8-2083547-194-psr4-src-xmlrpc.patch50.74 KBdonquixote
#194 D8-2083547-194-psr4-src.patch49.56 KBdonquixote
#190 D8-2083547-190-psr4-src-moved.patch848.73 KBdonquixote
#190 D8-2083547-190-psr4-src.patch43 KBdonquixote
#185 D8-2083547-185-psr4-src-moved.patch899.51 KBdonquixote
#185 D8-2083547-185-psr4-src.patch53.07 KBdonquixote
#164 D8-psr4-moved-2083547-164.patch848.92 KBdonquixote
#164 D8-psr4-2083547-164.patch51.38 KBdonquixote
#151 D8-psr4-composer-moved-2083547-151.patch841.23 KBdonquixote
#151 D8-psr4-composer-2083547-151.patch62.43 KBdonquixote
#145 D8-psr4-phpunit-moved-2083547-145.patch810.4 KBdonquixote
#145 D8-psr4-phpunit-2083547-145.patch54.78 KBdonquixote
#142 D8-psr4-phpunit-moved-2083547-142.patch808.39 KBdonquixote
#142 D8-psr4-phpunit-2083547-142.patch52.56 KBdonquixote
#139 D8-psr4-moved-2083547-139.patch779.15 KBdonquixote
#139 D8-psr4-2083547-139.patch49.18 KBdonquixote
#120 D8-psr4-2083547-120.patch46.75 KBdonquixote
#113 D8-psr4-moved-2083547-113-fixed.patch768.26 KBdonquixote
#113 D8-psr4-2083547-113-fixed.patch46.2 KBdonquixote
#111 D8-psr4-moved-2083547-113.patch768.35 KBdonquixote
#111 D8-psr4-2083547-113.patch46.29 KBdonquixote
#82 D8-psr4-moved-2083547-82.patch763.25 KBdonquixote
#82 D8-psr4-2083547-82.patch43.93 KBdonquixote
#76 D8-psr4-moved-2083547-75.patch759.59 KBdonquixote
#76 D8-psr4-2083547-75.patch43.47 KBdonquixote
#64 D8-psr4-replace-autoload-2083547-64.patch44.88 KBdonquixote
#64 D8-psr4-replace-autoload-moved-2083547-64.patch754.11 KBdonquixote
#61 D8-psr4-replace-autoload-2083547-60.patch44.63 KBdonquixote
#61 D8-psr4-replace-autoload-moved-2083547-60.patch762.06 KBdonquixote
#57 D8-psr4-replace-autoload-2083547-57.patch44.63 KBdonquixote
#57 D8-psr4-replace-autoload-moved-2083547-57.patch761.32 KBdonquixote
#55 D8-psr4-combined-2083547-54.patch46.15 KBdonquixote
#55 D8-psr4-combined-moved-2083547-54.patch762.83 KBdonquixote
#37 D8-psr4-combined-2083547-37.patch40.02 KBdonquixote
#37 D8-psr4-combined-moved-2083547-37.patch750.59 KBdonquixote
#35 D8-psr4-combined-2083547-35.patch43.45 KBdonquixote
#35 D8-psr4-combined-moved-2083547-35.patch749.09 KBdonquixote
#33 D8-psr4-combined-2083547-33.patch43.45 KBdonquixote
#33 D8-psr4-combined-moved-2083547-33.patch749.09 KBdonquixote
#24 D8-psr4-combined-2083547-24-vs-15.interdiff.txt9.08 KBdonquixote
#24 D8-psr4-combined-2083547-24.patch83.81 KBdonquixote
#24 D8-psr4-combined-moved-2083547-24.patch786.95 KBdonquixote
#15 D8-psr4-combined-2083547-15.patch86.96 KBdonquixote
#15 D8-psr4-combined-moved-2083547-15.patch789.47 KBdonquixote
#12 D8-psr4-combined-2083547-12.patch86.78 KBdonquixote
#12 D8-psr4-combined-moved-2083547-12.patch789.29 KBdonquixote
#9 D8-psr4-combined-2083547-9.patch86.43 KBdonquixote
#9 D8-psr4-combined-moved-2083547-9.patch788.94 KBdonquixote
#7 D8-custom-psr4-combined-moved-7.patch788.7 KBdonquixote
#4 D8-custom-psr4-combined-4.patch86.31 KBdonquixote
#3 D8-custom-psr4-combined-3.patch85.65 KBdonquixote
#1 8.x-psr4-combined-2083547-1.patch85.31 KBdonquixote
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

donquixote’s picture

Status: Active » Needs review
FileSize
85.31 KB

Status: Needs review » Needs work

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

donquixote’s picture

Status: Needs work » Needs review
FileSize
85.65 KB
donquixote’s picture

donquixote’s picture

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

yched’s picture

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

[diff]
  renames = copies

?)

donquixote’s picture

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.

donquixote’s picture

Status: Needs work » Needs review
FileSize
788.94 KB
86.43 KB

Status: Needs review » Needs work

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

donquixote’s picture

Status: Needs review » Needs work

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

aspilicious’s picture

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

donquixote’s picture

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

donquixote’s picture

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.

donquixote’s picture

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

donquixote’s picture

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.

donquixote’s picture

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.

aspilicious’s picture

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.

donquixote’s picture

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

webchick’s picture

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.

donquixote’s picture

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

Mile23’s picture

  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.

donquixote’s picture

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

Mile23’s picture

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? :-)

donquixote’s picture

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

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

Mile23’s picture

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.

donquixote’s picture

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

How is this "a class without an accessor" ?

Status: Needs review » Needs work

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

donquixote’s picture

Mile23’s picture

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

Then again, I'm stupid. :-)

donquixote’s picture

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.

donquixote’s picture

Status: Needs work » Needs review
pwolanin’s picture

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.

Mile23’s picture

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.

donquixote’s picture

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.

donquixote’s picture

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

pwolanin’s picture

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.

donquixote’s picture

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

webchick’s picture

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:

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.

webchick’s picture

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.

donquixote’s picture

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.

Crell’s picture

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.

webchick’s picture

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.

webchick’s picture

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

donquixote’s picture

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.

donquixote’s picture

Issue summary: View changes

link to scripts issue instead of annotation namespaces are stupid issue

Crell’s picture

- !!! 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". :-)

donquixote’s picture

Status: Needs work » Needs review
FileSize
762.83 KB
46.15 KB

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

chx’s picture

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

donquixote’s picture

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.

Crell’s picture

Status: Needs work » Needs review

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

donquixote’s picture

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

donquixote’s picture

Status: Needs review » Needs work

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

donquixote’s picture

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

donquixote’s picture

Status: Needs work » Needs review
FileSize
754.11 KB
44.88 KB

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

Mile23’s picture

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.

donquixote’s picture

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

webchick’s picture

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.

Codenator’s picture

cosmicdreams’s picture

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

donquixote’s picture

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

donquixote’s picture

Issue summary: View changes

Updated issue summary.

jcisio’s picture

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

donquixote’s picture

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

jcisio’s picture

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

Crell’s picture

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

webchick’s picture

Issue tags: -8.x-alpha4

:(

webchick’s picture

Issue summary: View changes

describe pending decisions in issue summary

donquixote’s picture

Issue summary: View changes
FileSize
43.47 KB
759.59 KB
donquixote’s picture

Issue summary: View changes

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

Crell’s picture

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

Crell’s picture

Status: Needs review » Needs work

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

donquixote’s picture

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.

donquixote’s picture

Status: Needs review » Needs work

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

Crell’s picture

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.

donquixote’s picture

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.

donquixote’s picture

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.

nyl_auster’s picture

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
webchick’s picture

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

Mile23’s picture

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

donquixote’s picture

@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

Crell’s picture

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

Mile23’s picture

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.

catch’s picture

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?

chx’s picture

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.

dsnopek’s picture

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.

nyl_auster’s picture

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.

Crell’s picture

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

Mile23’s picture

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?

ParisLiakos’s picture

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.

nyl_auster’s picture

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

Mile23’s picture

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

webchick’s picture

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

Mile23’s picture

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.

donquixote’s picture

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

alexpott’s picture

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.

xjm’s picture

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.

Dries’s picture

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

webchick’s picture

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

alexpott’s picture

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.

donquixote’s picture

@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

donquixote’s picture

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

    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.

donquixote’s picture

Status: Needs work » Needs review
donquixote’s picture

Issue summary: View changes

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

Crell’s picture

Status: Needs review » Reviewed & tested by the community

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

Crell’s picture

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?

donquixote’s picture

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.

tim.plunkett’s picture

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.

donquixote’s picture

Status: Needs work » Needs review
FileSize
46.75 KB

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)

webchick’s picture

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

donquixote’s picture

Issue summary: View changes
donquixote’s picture

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.

Crell’s picture

PSR-4 is now officially official:

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

Let's do this.

Dries’s picture

Post moved to meta.

chx’s picture

I have moved the discussion into the meta.

webchick’s picture

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.

donquixote’s picture

Status: Needs work » Needs review
FileSize
49.18 KB
779.15 KB

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.

donquixote’s picture

donquixote’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

donquixote’s picture

Status: Needs review » Needs work

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

Damien Tournoud’s picture

Status: Needs work » Needs review
Crell’s picture

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

donquixote’s picture

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.

donquixote’s picture

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.

donquixote’s picture

Status: Needs work » Needs review
FileSize
62.43 KB
841.23 KB

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.

donquixote’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

donquixote’s picture

Status: Needs work » Needs review
clemens.tolboom’s picture

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 :-/

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

clemens.tolboom’s picture

@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 :)

jbrown’s picture

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

Crell’s picture

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

clemens.tolboom’s picture

@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 :)

Mile23’s picture

Dries’s picture

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.

donquixote’s picture

Status: Needs work » Needs review
FileSize
51.38 KB
848.92 KB
aspilicious’s picture

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');
aspilicious’s picture

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

donquixote’s picture

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

donquixote’s picture

@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

aspilicious’s picture

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

donquixote’s picture

donquixote’s picture

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

sun’s picture

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.

joelpittet’s picture

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

donquixote’s picture

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

webchick’s picture

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.

pounard’s picture

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.

barraponto’s picture

Issue summary: View changes
ianthomas_uk’s picture

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.

donquixote’s picture

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.

donquixote’s picture

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.

Mile23’s picture

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

donquixote’s picture

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.

Mile23’s picture

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.

donquixote’s picture

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.

donquixote’s picture

FileSize
53.07 KB
899.51 KB

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.

joelpittet’s picture

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

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

donquixote’s picture

Status: Needs work » Needs review
FileSize
43 KB
848.73 KB

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.

sun’s picture

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

donquixote’s picture

pwolanin’s picture

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

webchick’s picture

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.

donquixote’s picture

joelpittet’s picture

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!

sun’s picture

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.

donquixote’s picture

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

aspilicious’s picture

*finally*

Berdir’s picture

Hiding all other patches then..

xjm’s picture

Status: Reviewed & tested by the community » Needs work

This issue needs a draft change record before it can be considered RTBC. :)

donquixote’s picture

Ok, can someone help with that? @sun maybe?

The following two doc pages can be a starting point:
PSR-4 namespaces and autoloading in Drupal 8 (proposed)
Draft: New doc page about PSR-0 in Drupal 8

EDIT:
We need to change /lib/ to /src/ in this doc page.

jhodgdon’s picture

Something to consider in the change notices: Presumably there was a big change notice (or several?) early on in the D8 cycle that explained how to use the old PSR-0 way of doing things. When/if this patch goes in, we'll want to either edit that change notice so that it points to this new one as an update, or delete it and redirect to this new one.

So, the task here is:

a) Make a new change notice about PSR-4, for this issue's changes, in "draft" status. Ideally, it should explain how to use PSR-4 without referring to PSR-0 (except maybe to say that earlier in the D8 dev cycle we adopted PSR-0). Change notices should be usable by people updating from 7, not really aimed at 8-to-8 updates.

b) Find any old notices about PSR-0 that exist.

c) Determine whether each existing PSR-0 change notice should be deleted/redirected, or edited.

d) Update the issue summary with a section in Tasks that explains specifically what needs to be done with each of the existing PSR-0 change notices when the patch is committed (if there are edits that need to be done, explain exactly which text needs to be replaced and provide replacement text; if they are to be deleted/redirected, explain where the redirect is going to).

xjm’s picture

Thanks @jhodgdon, excellent overview.

It should be noted that the "old PSR-0 way of doing things" will still exist and be relevant. The addition of PSR-4 support does not remove our PSR-0 support, so the original change record should probably just be edited to include a reference to the new one. The more tedious part will be updating every single place we reference a class filepath in the change records as we move specific modules to PSR-4, after this patch is in:
https://drupal.org/list-changes/published/drupal?keywords_description=%2...

Edit: Not to mention that I think moving files will break the links to them on api.d.o?

Edit 2: There aren't that many that refer explicitly to PSR-0, all things considered:
https://drupal.org/list-changes/published/drupal?keywords_description=ps...

jhodgdon’s picture

If files move, and people have URLs pointing to existing pages for classes/files/functions/etc. on api.drupal.org, yes the links will be broken.

We really need to get the API module's text filter up on drupal.org, which would automagically turn functions into api.drupal.org links... but probably we need to be able to specify the version first... there are issues in the API module for that. Anyway.

donquixote’s picture

@xjm:
So far the proposed plan always was like this:
1. For a transition phase, we support PSR-4 alongside PSR-0.
2. Core modules move to PSR-4.
3. We leave some more time (months?) for contrib to do the same.
4. We eliminate PSR-0 support for modules. This would require an additional change notice.

I am not aware of any decision that says we keep PSR-0 support forever, and I understood Dries' decision as a full switch to PSR-4 for modules. Although I might have missed something.

Keeping PSR-0 forever would introduce some (survivable, but still noticeable) problems:

  • A lot of places in code (see the patch) have duplicate logic to accomodate PSR-0 and PSR-4. Not just class loading, but also test discovery and plugin discovery.
  • For simplicity's sake, the PSR-0 legacy support is not really PSR-0, but rather a PSR-4 with deeper directories. Underscores are treated in PSR-4 way, not PSR-0 way. This is ok for a transition phase (imo), because it makes no difference for classes that follow the Drupal coding standards. But we don't want to long-term support an "almost PSR-0" thing.
  • Supporting both alongside may introduce (minor, but measurable) performance issues for class loading and plugin discovery.
  • As pointed out before, it is not immediately obvious why "lib" is PSR-0 and "src" is PSR-4. It is good enough for a transition phase, but we don't want to publish "Drupal 8 for beginners" books where we have to explain this rather arbitrary difference. If we'd keep both alongside forever, it might really be preferable to name them /psr0/ and /psr4/ instead of /lib/ and /src/. But this would still feel weird.

Note that you can still register 3rd party namespaces as PSR-0. We still need a decent mechanic to organize 3rd party code, but this will be independent of PSR-4 for modules.

@jhodgdon (#205):
I agree with that the D7 -> D8 transition should have priority in change notices. But I would still suggest that they should document (if only as a side note) the history of D8 development. For my taste, there should be separate notices for steps 1., 2. and 4. above, but only the one in 4. should focus on the D7 -> D8 switch. The other two change notices for 1. and 2. can still be useful to understand why some doc pages or some modules might still use /lib/ and PSR-0.

hass’s picture

Please eliminate PSR-0 support for modules before beta1 release. Otherwise both will be used. Let's force psr4... And commit it now... :-)

jhodgdon’s picture

+1000 to #209.

jibran’s picture

Well I am not a big fan of PSR-4 but I think #209 is a reasonable thing to do or else we'll end up PSR-0 PSR-4 combine mess which will be a big DrupalWTF and not good for DX.

donquixote’s picture

@hass, @jhodgdon (*), @jibran:
What exactly are you suggesting?
1. Skip the "transition phase" mentioned in #108, and eliminate PSR-0 support immediately?
2. Skip or postpone the change notice, so we can commit immediately?

I don't agree with either of these.
We can discuss the length of the transition phase, and eliminate PSR-0 support before beta1 if this is general consensus.
We can also decide to *deprecate* PSR-0 during the transition phase.
But skipping the transition phase does not serve any purpose. It would mean
- we have to reopen development on the patch #197, which is designed to only do step 1. mentioned in #108 (or at maximum steps 1. and 2.).
- we temporarily break devel and admin menu and other popular D8 contrib for no good reason, because they have no time to upgrade.
- we generally make the transition of core and core modules and pending core patches more difficult.

The plan as in #108 with a however-long transition phase has been the basis of this issue for months, so let's please let's please stick to it, and stop throwing it over for no reason.

(*) @jhodgdon: I know you are not really on this boat, but your post makes it look like it, this is why I mention you.

donquixote’s picture

If you are actually interested in speeding this up, then you can help with the change notice..

jhodgdon’s picture

Sorry. I meant in #210 that I would be in favor of making sure this is done before Beta, if contrib developers will need to use it.

And if the plan is to not have PSR-0 available at Release, then we need to make this absolutely clear to contrib developers by the time of Beta. If we plan to release with both PSR-0 and PSR-4 available, with no particular preference between them, then this is less urgent.

sun’s picture

Status: Needs work » Reviewed & tested by the community

This the maximum extent of change notices I can agree with:

  1. 8.x → 8.x: https://drupal.org/node/2246699

    Short and concise. Explains nothing else than the relevant change for developers that are working with D8 already or in the process of porting their modules. Including the agreed-on deadline for the final switch. Everything else is off-topic.

  2. Updating all existing change notices + handbook pages, after this patch has been committed.

    → Properly explaining the 7.x → 8.x change for everyone who has not been working with D8 code yet.

hass’s picture

D8 core breaks my contrib modules every few weeks. I see no reason why devel or admin menu should not be victims. However I have not said to remove it immediatly. Implement psr4, deprecate psr0 at the same time and finally remove before beta. Beta should be many weeks away... That should be enough time for updating contrib for psr4.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

From the change record:

Support for PSR-0 classes located in ./lib directories will be removed prior to the first beta release.

What about non-module classes? And I thought that components would still use PSR-0? We should include examples in the change record for these as well, since change records are for both core and contrib developers.

The change record looks sufficient other than that, but it will still be a massive undertaking to clean up references to PSR-0 and PSR-0-structured filepaths in outstanding change records, and those that need updates should be listed on the issue so that the champions of this issue can make those changes once it's live.

donquixote’s picture

My experience with change notice writing is not sufficient to decide how much detail they need to have.
However, here is an attempt for a comprehensive change notice that will give some background and hopefully clear up all open questions.
https://piratenpad.de/p/Drupal-PSR-4-change-notice
(some links still need to be filled in..)

jhodgdon’s picture

RE #218 - We have draft change notices on drupal.org. It would be vastly preferable to use those rather than "piratenpad" or other outside sites, because others can then edit them here.

RE #215 - If a Drupal 7 contrib module developer would have to follow a large chain of change notices in order to figure out how to convert his or her contrib module to Drupal 8, that is not acceptable. The primary purpose of change notices is between major versions, not between some point in the Drupal 8 development cycle and another point in the Drupal 8 development cycle. So saying that an 8-to-8 change notice is the only change notice you would be comfortable with is really not OK. We need to have clear guidance for module developers who are converting their contrib modules and who haven't been involved in Core development.

sun’s picture

Status: Needs work » Needs review

@jhodgdon: Yes, that's exactly what #215 stated. The 8.x → 8.x change notice is just a "nice-to-have" to address the concern of informing D8 (contrib) developers.

However, we cannot change existing change notices before this patch has been committed. That would be illogical, because people looking through change notices before this patch gets committed would be terribly confused.

The last ~20 comments discussed bureaucracy overhead only. Already now, that's more than 10% of all comments on this issue. If @donquixote wasn't contributing to core for some more time already, then he'd certainly run away screaming by now. I don't know why that is necessary and I hope that we don't end up with a ratio of 20%.

pounard’s picture

That's a bad idea eliminating PSR-0 I don't see the point here. If someone wants to use a PSR-0 architecture for its module or wants to package external libraries what would happen here? I don't see why the module should be forced to finish into the Drupal\[module]\ vendor prefix. You should consider flexibility here instead of forcing things, I don't see why an external module (let's say one I could maintain myself on github) could not name its classes \SomeVendor\CustomModule. The Drupal loader should be able to use a composer-like file for the module provider to be able to set its own custom namespace instead of forcing everything.

Forcing PSR-4 and module vendor namespace is a very bad DX. The Drupal loader should be flexible and composer/PSR compatible, and be able to load pretty much everything. If some company produces a very specific and non free module it should be able to keep its own vendor name and namespace (I'm not advocating for the non free software, I tend to defend free software) but anyway, this patch is IMHO just bad for contrib.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

At the very least that should be a separate issue.

What we need first is the ability to organize our files in a PSR-4 structure. That is what this patch does. It is green, it has been reviewed and it has a change notice that documents what changes for module developers.

As has been said above: Anything else is should be handled in a follow-up!
- What happens to files in /core/lib? I would say they stay PSR-0, maybe others disagree, but in any case: Let's discuss that after we have the initial step. Let's not discuss and document everything that is related to namespaces in this issue.
- Should we allow PSR-0 at all or forbid it? There are cases for and cases against. Again: Let's explore that in a follow-up. There is large consensus that we need to have some time period where we support both anyway to not break every module out there. So it's perfectly fine to discuss whether to keep it that way, or whether to remove PSR-0 support in a follow-up. Again, documentation can follow once we know what we want.
- Which change notices need to be updated? I would say more than 50% of all change notices need to updated, i.e. every single one that contains a use statement for a module. Therefore compiling a list beforehand does not provide very much benefit IMO. Once this is, we just have to go through all of them and check. I volunteer to spend up to 3 hours on that. Also it might not make sense to update all of them straightaway, since we are not updating all modules straightaway.

RTBC, RTBC, RTBC. This thing is so RTBC it's not even funny.

pounard’s picture

@#222 https://drupal.org/node/2246699 states that PSR-0 will be dropped, which means it's not a follow-up matter, either clear the notice from this or I'll continue to argue in here.

ianthomas_uk’s picture

I have opened #2247287: Drop automatic PSR-0 support for modules to discuss the future of PSR-0, and mentioned that the future of PSR-0 support is still being discussed in the CR (https://drupal.org/node/2246699).

donquixote’s picture

@jhodgdon: sun already posted a CR draft. I don't want to edit that so radically when I don't know yet that my changes are a good idea. I also don't want to spam drupal.org with alternative CR drafts. Everyone can edit the thing I posted in #218. And once it gets some feedback, we can either ignore it or work it into the existing CR, or even have it replace the text of the existing CR.

@sun, @jhodgon: About having too many change notices:
Imo, if every change record has a good intro, giving a background/context and links to related issues and CRs, then I don't see how too many of them could be a problem. If Google sends you to the wrong CR, there will be a link that takes you to the one you were really looking for. But maybe this is not the place to discuss this.

@pounard: After the transition phase, you can still use PSR-0 for 3rd party code or even for your module, BUT

  • You will be responsible to register this stuff to the class loader. Right now we have no recommended mechanic for this, but we are using the original Composer ClassLoader, and I am sure some future issue will deal with ways to handle 3rd party code.
  • Plugin discovery will not look into your PSR-0 folders.
  • Drupal's test discovery (simpletest and phpunit) will not look into your PSR-0 folders.

Your SomeVendor\CustomModule\\ suggestion was not supported with Drupal's default PSR-0 for modules, and neither is it with Drupal's default PSR-4 for modules. So nothing changes. Different issue.

We might introduce ways to tell Drupal to look into custom locations for discovery, and more comfortable ways to register 3rd party code. But this is totally independent of this issue.
See also #208.

@tstoeckler: Yes, a lot of this should go into separate issues, but for a lot of things we actually have the answers, since this has been discussed to death in the past. And the patch introduced here is actually not as "neutral" as it might look..

  • core/lib/ files stay where they are, but internally they are wired up as PSR-4 instead of PSR-0. So, Drupal\Core\\ -> core/lib/Drupal/Core, etc. The only difference this makes is how underscores in the class name are treated - which we don't have due to Drupal's coding standards.
    One reason to do this is performance (the class loader handles PSR-4 before PSR-0, but that's a minor issue). I am not sure about plugin discovery in Drupal\Core\\, don't even remember if this is possible.. but in this case it is also useful to not have to worry about the special underscore handling of PSR-0.
  • Keep PSR-0? Yes, this could be decided later, BUT the patch in #197 would look different if we would aim for long-term PSR-0 support. Please read #208.
  • Yes, if this is going to be a long list of change notices then we should not have this as a reason to slow down this patch.

@ianthomas_uk:
Ok, I will repost in this other issue.

@webchick:
Assuming you are the one who is going to commit this - what do you think about the merge vs apply? I imagine that this might make it easier for git to apply patches written for PSR-0, and rebase commits/branches written for PSR-0.

@everyone:
I think a lot of the discussion above could have been avoided if you guys would read more of the comments. Not even all of them but the most recent ones, and the stuff I keep repeating all over.

Berdir’s picture

@webchick:
Assuming you are the one who is going to commit this - what do you think
about the merge vs apply? I imagine that this might make it easier for git to
apply patches written for PSR-0, and rebase commits/branches written for
PSR-0.

That is irrelevant, works just as well when this is committed as a patch.

donquixote’s picture

@Berdir

That is irrelevant, works just as well when this is committed as a patch.

If the same file is moved *and* modified, then it is preferable to do this in two separate commits. Especially if the modification is big enough that git can no longer recognize the moved file. Plus, it will make it easier to look at commit diffs.
Merging will give you all this for free, because the tag I posted is already organized into separate commits.

Crell’s picture

For big reorgs like this merge is absolutely the superior approach. We've done it before, such as for /core-mageddon.

joachim’s picture

An alternative to merge which has an identical end-result is to use 'git format-patch' to make a multi-commit patch. 'git apply' then re-creates all the commits that are in the single patch file.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Please see above. The change record still has not been updated for the reason I marked this NW in #217. Edit: Just add what the current effect is (even if it's "none") for core/lib/Drupal/Core and core/lib/Drupal/Component; it's not an essay I'm asking for. It will take someone who knows less than 5 minutes and then you can set this RTBC again. :)

The D8-to-D8 change record is not a "nice to have"; it is essential. Edit: Because Drupal 8 core has over 2000 contributors that this affects and there are over 400 modules with 8.x versions already.

xjm’s picture

Status: Needs work » Reviewed & tested by the community
Related issues: +#2247381: Update all existing change records for PSR-4 support

Thanks @tstoeckler: https://drupal.org/node/2246699/revisions/view/7165649/7166209

I added an explicit followup for the change record updates since it's such a big undertaking.

webchick’s picture

If you want me to do a merge, I need some copy/pasta commands. :) I have no idea which git repo this is against, etc.

donquixote’s picture

@webchick:
The history in #197 is like here (after a rebase on the freshest 8.x, no conflicts):
https://github.com/donquixote/drupal/compare/D8-2083547-233-psr4-src-sho...
(this is a tag, not a branch, but this does not matter for merging)
To merge this, you would
git pull --no-ff https://github.com/donquixote/drupal.git D8-2083547-233-psr4-src-shortcut
(or you could first fetch and look at the diff, before you merge)

Now you see that this tag has quite a lot of commits, and it might be a good idea to squash some of them. Also the commit messages might not be what we like to see. So, after some discussion with sun on IRC, this is what I came up with after interactive rebase:
https://github.com/donquixote/drupal/compare/D8-2083547-233-psr4-src-sho...
There are now only 4 commits left.
To merge this, you would
git pull --no-ff https://github.com/donquixote/drupal.git D8-2083547-233-psr4-src-shortcut-squashed
(or you could first fetch and look at the diff, before you merge)

If you want, I can further improve the commit messages, or change how this is sliced up into separate commits. Let me know on IRC.

Whether you use --no-ff or not is up to you. It will define how the history looks like after the pull. A nice way to look at this history is the "git lola" alias, or
git log --graph --decorate --pretty=oneline --abbrev-commit --all

joachim’s picture

@donquixote: I would suggest you do

$ git format-patch --stdout 8.x > PATCHFILENAME

That will give you a single patch file that contains multiple commit which you can upload here.

Webchick can then do one of:

A.

$ git am PATCHFILENAME

which puts the sequence of commits from PATCHFILENAME onto the 8.x branch

or:

B.

$ git checkout -b BRANCHNAME
$ git am PATCHFILENAME
$ git checkout 8.x
$ git merge --no-ff BRANCHNAME

which puts the sequence of commits in a feature branch.

BTW: the testbot works fine with patches in this format. I think they're a good idea for when a patch for an issue starts to get really big as they show a sequence of changes you can step through.

donquixote’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
66.85 KB

@joachim: This seems fun.
I still prefer merges, but this method integrates better with the usual Drupal issue queue work flow.
One disadvantage is it actually creates distinct commits that won't align with the original commits. But who cares, after tons of rebase.

I am setting this to "needs review" one last time, so the testbot can do its job.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Testbot also tests RTBC patches (and retests them automatically, even). :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome, thanks for the Git help, folks. One of these days I will learn. :P Or, you know, we'll just switch to Subversion. ;) KIDDING.

Here are some comments I hit when I was going through. None of them are commit-blockers, and I feel strongly about committing this ASAP so we can ship an alpha (or two) that allows contrib to format their modules' directories the way they're meant to upon release.

I also want to give a huge shout-out to donquixote and acknowledge his efforts and perseverance here. Thank you so much for sticking with this issue through thick and thin (mainly thick :P), improving upstream Composer support, class loader documentation, performance, and various other aspects in the process. Hope whatever your next Drupal 8 mission is can be a bit less harrowing and more fun. :)

  1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -422,9 +422,9 @@ protected function initializeContainer() {
    -        $namespaces_after = $this->classLoader->getPrefixes();
    +        $namespaces_after = $this->classLoader->getPrefixesPsr4();
    ...
    -        $this->registerNamespaces($namespaces_before);
    +        $this->registerNamespacesPsr4($namespaces_before);
    

    In general, not thrilled with this kind of change, since PSR-4 *is* our namespace convention now (soon), but this is borrowed from upstream Composer code which does the same thing, so I think it's fine.

  2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -697,6 +698,28 @@ protected function getModuleFileNames() {
    +      // @todo Remove lib/Drupal/$module, once the switch to PSR-4 is complete.
    +      $namespaces["Drupal\\$module"][] = DRUPAL_ROOT . '/' . dirname($filename) . '/lib/Drupal/' . $module;
    

    In the above discussion, this seemed to be pretty contentious. Thanks for spinning off a sub-issue to discuss it: #2247287: Drop automatic PSR-0 support for modules

  3. +++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Tests/d6/MigrateActionConfigsTest.php
    @@ -40,7 +40,7 @@ public function setUp() {
    -      drupal_get_path('module', 'migrate_drupal') . '/lib/Drupal/migrate_drupal/Tests/Dump/Drupal6ActionSettings.php',
    +      dirname(__DIR__) . '/Dump/Drupal6ActionSettings.php',
    

    A little unclear why we're doing these (and other Migrate test-related) changes here, when I thought the idea was just to do Shortcut as a sample and leave everything else in place until we do the huge move. It's going to break #2121299: Migrate in Core: Drupal 6 to Drupal 8 which is unfortunate. :( I assume this is because it breaks something without it.

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

    I believe I already said this like 500 comments ago or something, but I still *really* don't like the idea of committing this script to core. It is very handy in the short-term, but only in the interim for core + the ~500 modules outside of core that started porting to D8 prior to beta1. In the longer-term, it's going to make no sense to include by the time D8 ships and will only be confusing. So let's keep it in for now, but remove it before RC. Sound good?

Great work on the change notice, great work on figuring out a middle-ground here that we can get into core and proceed ahead with.

COMMITTED AND PUSHED TO 8.X (merged from the D8-2083547-233-psr4-src-shortcut-squashed branch). YEAAAAAAAHHHHH!!!! :)

  • Commit 4f18d62 on 8.x authored by donquixote, committed by webchick:
    Issue #2083547 by donquixote: Docblock and code style improvements in...
  • Commit 940e0df on 8.x authored by donquixote, committed by webchick:
    Issue #2083547 by donquixote: Implement PSR-4 for module-provided class...
  • Commit cbba9ca on 8.x authored by donquixote, committed by webchick:
    Issue #2083547 by donquixote: Introduce core/scripts/switch-psr4.sh, to...
  • Commit e2491fc on 8.x authored by donquixote, committed by webchick:
    Issue #2083547 by donquixote: Move class files in core/modules/shortcut...
donquixote’s picture

@webchick: Big thanks! And thanks for the moral support and heads up along the way.

In general, not thrilled with this kind of change, since PSR-4 *is* our namespace convention now (soon), but this is borrowed from upstream Composer code which does the same thing, so I think it's fine.

Let's clean this up when we officially drop PSR-0 support.

A little unclear why we're doing these (and other Migrate test-related) changes here, when I thought the idea was just to do Shortcut as a sample and leave everything else in place until we do the huge move.

The idea here is that all that remains to do is to execute the script to move the files around, and Drupal will still work.
This is why I uploaded 2 patches each time, to verify that moving the files won't break Drupal.

I can have a look at the "Migrate in core" issue, I think I can help with the reroll.

I believe I already said this like 500 comments ago or something, but I still *really* don't like the idea of committing this script to core.

Let's remove it when we don't need it anymore. Or if a lot of people want to keep it, maybe move it to contrib?

donquixote’s picture

@webchick:
Maybe we could have done this in a separate commit - but too late now.
https://github.com/donquixote/drupal/commit/60834c2f540e49dac8b8a5cb1c55...

donquixote’s picture

And what about a "psr-4-reroll" tag?

dsnopek’s picture

Huzzah! Congrats, everyone, on getting this in. :-)

donquixote’s picture

@webchick (#237)

It's going to break #2121299: Migrate in Core: Drupal 6 to Drupal 8 which is unfortunate.

It did not break this other issue.
But, the other issue introduced some drupal_get_path() which will make the final migration harder. I posted a follow-up here:
#2248149: Use __DIR__ instead of drupal_get_path() for local PSR-0/PSR-4 directories.

hass’s picture

Sorry for asking stupid questions... but has the PSR-4 migration any effect on:

/**
 * @file
 * Contains \Drupal\google_analytics\GoogleAnalyticsSettingsForm.
 */

or namespace declarations?

namespace Drupal\google_analytics;
Berdir’s picture

No, PSR-4 only affects the physical location of the files, nothing else changes.

hass’s picture

Berdir’s picture

Nothing to do with PSR-4, testbot doesn't like the new drush version. all 8.x runs are failing due to that right now.

hass’s picture

xjm’s picture

Status: Fixed » Closed (fixed)

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