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
- Move all class files in this issue in one massive patch, scheduled for May 27. The patch should only contain file moves, and should be rolled and then committed as soon as it passes testbot, with no commits in between.
- #2247381: Update all existing change records for PSR-4 support then needs to be unpostponed to update any mentions in documentations of filepaths for module class files. At least these change records:
https://drupal.org/list-changes/published/drupal?keywords_description=%2... - Post g.d.o/core post announcing the conversion and providing instructions for updating patches.
- #2247287: Drop automatic PSR-0 support for modules to remove PSR-0 support and the move script.
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.
Comment | File | Size | Author |
---|---|---|---|
#57 | psr4-2247991-57.patch | 931.26 KB | xjm |
#53 | D8-2247991-53-psr4-move.patch | 931.44 KB | donquixote |
#53 | D8-2247991-53-psr4-move.interdiff.txt | 325 bytes | donquixote |
Comments
Comment #1
xjmComment #2
xjmComment #3
donquixote CreditAttribution: donquixote commentedThis 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.
Comment #4
xjmComment #5
jibranSee #7269-89: [meta] Add .php extension to PHP files
Comment #6
sunComment #7
xjmThis change is now tentatively scheduled for May 27, 2014. D8 alpha 12 will be released May 28.
Comment #8
donquixote CreditAttribution: donquixote commentedLet'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
Comment #10
donquixote CreditAttribution: donquixote commentedgit 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)
Comment #12
sunCreated #2271801: CustomBlockLocalTasksTest is located in wrong directory
Comment #13
donquixote CreditAttribution: donquixote commentedLooking at
Drupal\Core\Field\FieldDefinition::getSchema()
:We should not blindly trust array values like this. At least throw an exception.
Comment #14
donquixote CreditAttribution: donquixote commentedI 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)
Comment #15
donquixote CreditAttribution: donquixote commented10: D8-2247991-10-psr4-move.patch queued for re-testing.
Comment #16
donquixote CreditAttribution: donquixote commentedNew issue created: #2271897: FieldDefinition::getSchema() should not blindly trust array value to be a class.
Comment #18
donquixote CreditAttribution: donquixote commentedPatch 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.
Comment #20
donquixote CreditAttribution: donquixote commentedSee #2257769-20: Adding an Entity Reference field in the Field UI throws a PHP notice; fails to add field for why this fails.
I need some feedback about this!
Comment #22
donquixote CreditAttribution: donquixote commentedLet'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())
Comment #25
donquixote CreditAttribution: donquixote commentedComment #27
donquixote CreditAttribution: donquixote commentedComment #28
donquixote CreditAttribution: donquixote commentedComment #30
xjm@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.Comment #31
adammaloneUploading a diff created with
git diff --diff-filter=M
to show non-move changes.
Comment #32
adammalone27: D8-2247991-27-psr4-move-FieldDefinition.patch queued for re-testing.
Comment #34
donquixote CreditAttribution: donquixote commented@xjm (#30), @typhonius (#31):
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:
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.
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.
Comment #35
adammaloneContinuing 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.
Comment #36
xjmThanks @donquixote. So it sounds like https://drupal.org/files/issues/D8-2247991-32-psr4-prepare-move.patch is the one to review in dreditor:
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?
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?
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.
Comment #37
donquixote CreditAttribution: donquixote commentedI created a separate issue for the FieldDefinitionTestBase problem.
#2273311: FieldDefinitionTestBase::getNamespacePath() breaks with PSR-4
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.
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')
.Good point.
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.
Comment #38
xjmThanks @donquixote.
Well, we prefer
git diff
-formatted patches in the issue queue generally because format-patch patches tend to confuse reviewers. Please always provide agit 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.
Comment #39
xjmDiscussed 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.
Comment #40
xjmActually; 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?
Comment #41
donquixote CreditAttribution: donquixote commentedComment #42
xjmComment #43
xjmAlright, let's test this again now.
Comment #44
xjmComment #45
xjmComment #46
donquixote CreditAttribution: donquixote commented#43 looks good.
shows that no PSR-0 folders are left over (on latest 8.x I just fetched).
Comment #47
donquixote CreditAttribution: donquixote commentedAnd thanks xjm for coordinating and moderating these issues!
Comment #48
donquixote CreditAttribution: donquixote commentedMaybe 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.
Comment #49
xjm@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. :)
Comment #50
webchickAwesome!! GREAT work!
Comment #51
chx CreditAttribution: chx commentedThis will work with sandboxes.
tons of renames roll by and no conflict in sight.
Comment #52
int CreditAttribution: int commentedToday is the big day... :-D
Comment #53
donquixote CreditAttribution: donquixote commentedOne more file to move:
core/modules/views/tests/{Drupal/views/Tests => src}/ViewExecutableFactoryTest.php
Comment #54
donquixote CreditAttribution: donquixote commentedComment #55
Berdir53: D8-2247991-53-psr4-move.patch queued for re-testing.
Comment #57
xjmWe'll want to run the script immediately before we test and commit it later today. Just making sure everthing still works though.
Comment #58
xjmComment #59
xjmI'll work with a core committer to get this in smoothly later today, so assigning to myself.
Comment #60
webchickOk, 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. :(
Comment #61
webchickSlight 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!
Comment #62
chx CreditAttribution: chx commentedIf you want I can write you a sed script which "rerolls" all patches :)
Comment #63
webchickOk. :) The moment of truth:
COMMITTED AND PUSHED TO 8.X YEAAAHHH!
Awesome work on this everyone. This makes my soul happy.
Comment #65
Dave ReidComment #66
Daniel Norton CreditAttribution: Daniel Norton commentedTitle change to make it more conspicuous and easier to find files that have moved, where they moved and why.
Comment #67
donquixote CreditAttribution: donquixote commentedComment #68
YesCT CreditAttribution: YesCT commented#2286241: Move stray SystemLocalTasksTest to PSR-4 location
Comment #69
xjmComment #71
Mile23Just a heads-up: This issue introduced a commented-out method, and here's an issue about it: #2411961: Figure out what to do with Drupal\field\Tests\FormTest::testFieldFormMultiple()
And here's where it was found: #2392669: Clean-up field module test members - ensure property definition and use of camelCase naming convention