Problem/Motivation

#2083547: PSR-4: Putting it all together adds support for PSR-4 for modules with the intention of converting all modules to PSR-4.

See the Documentation page for PSR-4 in Drupal 8.

Proposed resolution

Convert all the modules.

Remaining tasks

User interface changes

None

API changes

Filepaths change; functional code does not.

Postponed until

#2271801: CustomBlockLocalTasksTest is located in wrong directory (fixed)
#2248149: Use __DIR__ instead of drupal_get_path() for local PSR-0/PSR-4 directories. (fixed)
#2273311: FieldDefinitionTestBase::getNamespacePath() breaks with PSR-4 (fixed)
#2273689: switch-psr4.sh does not reliably remove empty PSR-0 directories. (fixed)

Nice to have, but not a blocker

#2271897: FieldDefinition::getSchema() should not blindly trust array value to be a class.

This change is scheduled for May 27, 2014.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
donquixote’s picture

This issue is going to happen only shortly before beta1 release. And in an ideal world, this will be nothing more but running the switch-psr4.php script.

We can do some preparation to make this easier:
#2248149: Use __DIR__ instead of drupal_get_path() for local PSR-0/PSR-4 directories.

In fact this was already part of the patch in #2083547: PSR-4: Putting it all together, but new occurances of drupal_get_path() were introduced in #2121299: Migrate in Core: Drupal 6 to Drupal 8.

xjm’s picture

Issue summary: View changes
jibran’s picture

sun’s picture

xjm’s picture

Title: [PP-1] [meta] Move all module code to PSR-4 » [May 27] Move all module code to PSR-4
Issue summary: View changes
Status: Postponed » Active

This change is now tentatively scheduled for May 27, 2014. D8 alpha 12 will be released May 28.

donquixote’s picture

Status: Active » Needs review
FileSize
1.4 MB

Let's see if this works!
One file that needed to be manually corrected before the move is CustomBlockLocalTasksTest.php.
See the first part of the patch (made with git format-patch).

I also had to manually remove mode changes from the patch. Could not figure out how to force format-patch to ignore that.
(yes, I have core.fileMode=false)
Files with odd file modes are:
core/modules/ban/src/BanIpManagerInterface.php
core/modules/simpletest/src/Tests/SimpleTestTest.php
core/modules/system/src/Tests/Database/DeleteTruncateTest.php

No idea if this is just me..

This is to let the testbot have a look. We need to roll this again shortly before it gets committed.
This patch will grow out of date very very fast!

EDIT:
CustomBlockLocalTasksTest was added here:
https://drupal.org/node/2102125

Status: Needs review » Needs work

The last submitted patch, 8: D8-2247991-8-psr4-move.patch, failed testing.

donquixote’s picture

Status: Needs work » Needs review
FileSize
930.9 KB

git format-patch is not my friend, it seems.
Let's try an old-school patch.

We could split out the moving of CustomBlockLocalTasksTest to another issue.. but for now it is included in the patch. (and thus almost invisible)

Status: Needs review » Needs work

The last submitted patch, 10: D8-2247991-10-psr4-move.patch, failed testing.

sun’s picture

donquixote’s picture

Drupal\node\Tests\Plugin\views\field\NodeBulkFormTest          1 passes
PHP Fatal error:  Class name must be a valid object or a string in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Field/FieldDefinition.php on line 479
PHP Warning:  Invalid argument supplied for foreach() in /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh on line 651

Warning: Invalid argument supplied for foreach() in /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh on line 651
Drupal\quickedit\Tests\Access\EditEntityAccessCheckTest        4 passes

Looking at Drupal\Core\Field\FieldDefinition::getSchema():

      // Get the schema from the field item class.
      $definition = \Drupal::service('plugin.manager.field.field_type')->getDefinition($this->getType());
      $class = $definition['class'];
      $schema = $class::schema($this);

We should not blindly trust array values like this. At least throw an exception.

donquixote’s picture

I cannot reproduce. I don't even know which test I need to run. I tried NodeBulkFormTest and EditEntityAccessCheckTest. Both on the commandline and on the web UI. No fail.
Can someone help?
(I'm going to retest, but don't see why this would help)

donquixote’s picture

Status: Needs work » Needs review

10: D8-2247991-10-psr4-move.patch queued for re-testing.

donquixote’s picture

Status: Needs review » Needs work

The last submitted patch, 10: D8-2247991-10-psr4-move.patch, failed testing.

donquixote’s picture

Status: Needs work » Needs review
FileSize
930.51 KB
931.51 KB

Patch did no longer apply.

I am also adding an additional patch where #2271897-1: FieldDefinition::getSchema() should not blindly trust array value to be a class. is merged in.

The last submitted patch, 18: D8-2247991-18-psr4-move.patch, failed testing.

donquixote’s picture

The last submitted patch, 18: D8-2247991-18-psr4-move-FieldDefinition.patch, failed testing.

donquixote’s picture

Let's try again with format-patch.
This time I won't try to be smart and will not remove any mode changes.

Commits in the first patch:
- Correct the folder name for CustomBlockLocalTasksTest.
- Make PathFieldDefinitionTest safe for PSR-4. Clean this up later.
- Improve core/scripts/switch-psr4.sh, to make sure that remaining empty PSR-0 directories are removed.
- Move module-provided class files to PSR-4.

Additional commit in the other patch, merged in:
- Throw exceptions in case that $definition['class'] is not a class of the expected type. (FieldDefinition::getSchema())

The last submitted patch, 22: D8-2247991-22-psr4-move.patch, failed testing.

The last submitted patch, 22: D8-2247991-22-psr4-move-FieldDefinition.patch, failed testing.

donquixote’s picture

Status: Needs review » Needs work

The last submitted patch, 25: D8-2247991-25-psr4-move-FieldDefinition.patch, failed testing.

donquixote’s picture

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

Status: Needs review » Needs work

The last submitted patch, 27: D8-2247991-27-psr4-move-FieldDefinition.patch, failed testing.

xjm’s picture

@donquixote, as you're rolling these, can you also post a -do-not-test.patch version with all the changes other than the file moves, so that we can review those changes easily? Thanks for your work on this.

adammalone’s picture

Uploading a diff created with

git diff --diff-filter=M

to show non-move changes.

adammalone’s picture

The last submitted patch, 27: D8-2247991-27-psr4-move-FieldDefinition.patch, failed testing.

donquixote’s picture

@xjm (#30), @typhonius (#31):

can you also post a -do-not-test.patch version with all the changes other than the file moves, so that we can review those changes easily?

I originally wanted to post the patches with format-patch, to make it possible to see the individual steps / commits.
Unfortunately, format-patch doesn't seem to like me very much, git or testbot-git refuse to apply them.
(I posted format-patch patches elsewhere, and it worked fine)

Solution:
github tag to view history:
https://github.com/donquixote/drupal/compare/D8-2247991-32-psr4-move

Attached:

  1. One patch with everything included, made with git diff (psr4-move.patch).
  2. One patch with everything except the move, made with git diff (psr4-prepare-move.patch).
    I am not adding -do-not-test here, because i actually want these changes to work independent of the move, so I want this to be tested.
  3. One patch with everything except the move, made with git format-patch (psr4-prepare-move.format-patch.patch).

Regarding FieldDefinitionTestBase and PathFieldDefinitionTest:
The problem is we need both the module name and the module dir, or the directory for psr4/psr0.
Unfortunately, there is no reliable way to determine a module name from a PSR-4 directory returned with getNamespacePath().

Therefore I replaced this method with getModuleInfoFilePath(). An info file tells us about module name and module directory.
Then instead of this method telling us whether to look in the psr-0 or the psr-4 directory, we simply look in both - like the rest of all plugin discovery code. We can clean this up when PSR-0 support ends.
https://github.com/donquixote/drupal/commit/df501b4c1209fb1a015c2e7efbd9...

OPTIONAL: In a subsequent commit, I added a default implementation for getModuleInfoFilePath(), which assumes that the plugins to test are in the same module as the test class. This allows child classes to no not specify this at all.
https://github.com/donquixote/drupal/commit/f005a680de5bd6ed014b476c2ad1...

It all depends what other tests we want to add that inherit FieldDefinitionTestBase.

See #2257769: Adding an Entity Reference field in the Field UI throws a PHP notice; fails to add field
I have no problem discussing this elsewhere.

I should add that @typhonius has been helpful on IRC, and also had some ideas.

adammalone’s picture

Continuing some observations over on #2257769: Adding an Entity Reference field in the Field UI throws a PHP notice; fails to add field, donquixote and I came up with three potential solutions to the issue.

Neither of us really liked the idea of a method that returned an array of module name and module directory. The alterations provided above in #34 are of the info.yml file method. This would provide a single method from which both module name and module directory may be enumerated.

The final option would be for the test class to provide the module name itself (in addition to the namespace path as it currently does). A quick mock up of that and diff against #27 is here: https://gist.github.com/typhonius/bc254b8fab56d228d068. I'm unsure which would be a best practice really.

xjm’s picture

Thanks @donquixote. So it sounds like https://drupal.org/files/issues/D8-2247991-32-psr4-prepare-move.patch is the one to review in dreditor:

  1. +++ b/core/tests/Drupal/Tests/Core/Field/FieldDefinitionTestBase.php
    @@ -28,15 +28,22 @@
    +
    +    $module_info_file = $this->getModuleInfoFilePath();
    +    if (!preg_match('#^(.*)/([^/]+)\.info\.yml$#', $module_info_file, $m)) {
    +      throw new \Exception("Unexpected module info file.");
    

    Why do we need to do this validation here? The preg_match() seems especially out of place. It would belong in getModuleInfoFilePath() if anything, no?

  2. +++ b/core/tests/Drupal/Tests/Core/Field/FieldDefinitionTestBase.php
    @@ -28,15 +28,22 @@
    +    list(, $module_dir, $module_name) = $m;
    +
    +    $namespaces = new \ArrayObject(
    +      array(
    +        "Drupal\\$module_name" => array(
    +          $module_dir . '/src',
    +          $module_dir . '/lib/Drupal/' . $module_name,
    +        ),
    +      )
    +    );
    

    Can we add back the comment here about supporting both PSR-0 and PSR-4, along with an @todo referencing https://drupal.org/node/2247287?

  3. +++ b/core/tests/Drupal/Tests/Core/Field/FieldDefinitionTestBase.php
    @@ -72,15 +79,105 @@ public function setUp() {
    +   * @throws \Exception
    

    This should probably throw a typed exception. We should also document when the exception is thrown in the docblock.

I have lots more comments but no time to get into it now so will review more later.

donquixote’s picture

I created a separate issue for the FieldDefinitionTestBase problem.
#2273311: FieldDefinitionTestBase::getNamespacePath() breaks with PSR-4

So it sounds like https://drupal.org/files/issues/D8-2247991-32-psr4-prepare-move.patch is the one to review in dreditor

I would rather look at the one with "format-patch" in the name, where you see step-by-step commits with commit message.
I only uploaded the other one because I had some bad experience with format-patch stuff not being accepted by testbot. Seems like it was not justified in this case.

Why do we need to do this validation here? The preg_match() seems especially out of place. It would belong in getModuleInfoFilePath() if anything, no?

The idea is that test cases can override the getModuleInfoFilePath() method, so we don't know what kind of junk will be returned. And we still need the module name and the namespace paths.
This being said, in #2273311: FieldDefinitionTestBase::getNamespacePath() breaks with PSR-4 I am suggesting an alternative, where the method is getModuleAndPath(), which would return array('path', 'core/modules/path').

Can we add back the comment here about supporting both PSR-0 and PSR-4, along with an @todo referencing #2247287: Drop automatic PSR-0 support for modules?

Good point.

This should probably throw a typed exception. We should also document when the exception is thrown in the docblock.

Generally yes. I just never know where to put these exception classes.
Over to #2273311: FieldDefinitionTestBase::getNamespacePath() breaks with PSR-4, and IRC if you want.

xjm’s picture

Thanks @donquixote.

I would rather look at the one with "format-patch" in the name, where you see step-by-step commits with commit message.
I only uploaded the other one because I had some bad experience with format-patch stuff not being accepted by testbot. Seems like it was not justified in this case.

Well, we prefer git diff-formatted patches in the issue queue generally because format-patch patches tend to confuse reviewers. Please always provide a git diff patch. :)

Bumped #2273311: FieldDefinitionTestBase::getNamespacePath() breaks with PSR-4 to major for more visibility. I'm wondering if we should instead pull the changes out of this patch related to that problem, file a critical bug for the problem (potentially with a test), and then decide between the solution here or #2273311: FieldDefinitionTestBase::getNamespacePath() breaks with PSR-4 based on the usecase rather than abstract architectural discussion.

xjm’s picture

Discussed with @donquixote and we'll move each separate bug out into its own quick blocking issue, but keep this open to show the patch stays green with the combined fixes.

xjm’s picture

Title: [May 27] Move all module code to PSR-4 » [PP-1] [May 27] Move all module code to PSR-4
Issue summary: View changes
Status: Needs review » Postponed

Actually; let's set this postponed on #2273311: FieldDefinitionTestBase::getNamespacePath() breaks with PSR-4 and any other hard blockers we separate out, so others don't have the same confusion I did. We can just put it back to NR temporarily whenever we want to test a patch again.

Let's put https://github.com/donquixote/drupal/commit/fd30a64de60a2041d69e42f03fca... in its own issue too. That one will block this since presumably it rolls this patch incorrectly in HEAD.

https://github.com/donquixote/drupal/commit/da6ffcf45587269741d175dadb8b... is covered by #2271897: FieldDefinition::getSchema() should not blindly trust array value to be a class. but I don't think it actually blocks this, right? It's just a separate bugfix we discovered in the process of debugging this, and this patch will work without that change. Correct?

https://github.com/donquixote/drupal/commit/f005a680de5bd6ed014b476c2ad1... should also be a separate issue. Edit: wait, is that the same as #2273311: FieldDefinitionTestBase::getNamespacePath() breaks with PSR-4, or different? It seems to be fixing the same bug?

donquixote’s picture

Issue summary: View changes
xjm’s picture

Title: [PP-1] [May 27] Move all module code to PSR-4 » [PP-2] [May 27] Move all module code to PSR-4
Issue summary: View changes
Parent issue: #2251085: [meta] Drupal 8 pre-beta major disruption window »
xjm’s picture

Status: Postponed » Needs review
FileSize
931.03 KB

Alright, let's test this again now.

xjm’s picture

Title: [PP-2] [May 27] Move all module code to PSR-4 » [May 27] Move all module code to PSR-4
Issue summary: View changes
xjm’s picture

donquixote’s picture

Status: Needs review » Reviewed & tested by the community

#43 looks good.

curl https://drupal.org/files/issues/psr4-2247991-43.patch | git apply
find | grep lib$
find | grep tests/Drupal$

shows that no PSR-0 folders are left over (on latest 8.x I just fetched).

donquixote’s picture

And thanks xjm for coordinating and moderating these issues!

donquixote’s picture

Maybe we should add a unit test to make sure that no PSR-0 classes are added to core/modules in the future?
This can be a follow-up and does not need to block this move.

xjm’s picture

@donquixote, I think we should just do #2247287: Drop automatic PSR-0 support for modules as soon as possible actually.

Now we just wait for May 27. :)

webchick’s picture

Awesome!! GREAT work!

chx’s picture

This will work with sandboxes.

curl https://drupal.org/files/issues/psr4-2247991-43.patch|git apply --index      
git commit -am 'test the psr4 move'      
git checkout autoescape
git merge 8.x

tons of renames roll by and no conflict in sight.

int’s picture

Today is the big day... :-D

donquixote’s picture

One more file to move:
core/modules/views/tests/{Drupal/views/Tests => src}/ViewExecutableFactoryTest.php

donquixote’s picture

Berdir’s picture

53: D8-2247991-53-psr4-move.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 53: D8-2247991-53-psr4-move.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
931.26 KB

We'll want to run the script immediately before we test and commit it later today. Just making sure everthing still works though.

xjm’s picture

Status: Needs review » Reviewed & tested by the community
xjm’s picture

Assigned: Unassigned » xjm

I'll work with a core committer to get this in smoothly later today, so assigning to myself.

webchick’s picture

Ok, should hopefully have time to get this in about 3-4 hours from now. I will *not* have time to review/commit any other patches before then though. :(

webchick’s picture

Slight change of plans... xjm is going to work on a script to automatically re-roll all of the various patches that are in a NR/RTBC state (where possible) after this goes in, and we'll reconvene in about 3 hours or so to get this one in.

Eyeballing the RTBC criticals at the moment, I don't see any that look pick-offable before then, and I believe the other core maintainers are either asleep or doing other things this evening, so the latest patch should stand. Hopefully xjm's script can re-roll those easily after this lands!

chx’s picture

If you want I can write you a sed script which "rerolls" all patches :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok. :) The moment of truth:

COMMITTED AND PUSHED TO 8.X YEAAAHHH!

Awesome work on this everyone. This makes my soul happy.

  • Commit d0d3e53 on 8.x by webchick:
    Issue #2247991 by xjm, typhonius, donquixote: [May 27] Move all module...
Dave Reid’s picture

Daniel Norton’s picture

Title: [May 27] Move all module code to PSR-4 » [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4
Related issues: +#2078801: Some comments: PSR-0 underscore, PSR-4 lib vs src

Title change to make it more conspicuous and easier to find files that have moved, where they moved and why.

donquixote’s picture

Issue summary: View changes
YesCT’s picture

xjm’s picture

Assigned: xjm » Unassigned

Status: Fixed » Closed (fixed)

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

Mile23’s picture