Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
#2083547: PSR-4: Putting it all together introduces support for PSR-4 in modules, as documented at https://drupal.org/node/2246699. PSR-0 is currently still supported.
Proposed resolution
TBC. Please see #2083547-208: PSR-4: Putting it all together to #223 for previous discussion.
Remaining tasks
The patch should:
- Remove support for PSR-0 autoloading for modules.
- Remove PSR-0 support in plugin discovery in modules.
- Remove PSR-0 support in test discovery (simpletest, phpunit) in modules.
- Remove PSR-4 support from FieldDefinitionTestBase -- see #2273311: FieldDefinitionTestBase::getNamespacePath() breaks with PSR-4.
User interface changes
N/A
API changes
See https://drupal.org/node/2246699.
Postponed until
Comment | File | Size | Author |
---|---|---|---|
#33 | D8-2247287-33-drop-psr0.patch | 15.35 KB | donquixote |
Comments
Comment #1
donquixote CreditAttribution: donquixote commentedRe-posting from #2083547-208: PSR-4: Putting it all together
And from #2083547-225: PSR-4: Putting it all together
EDIT:
I also want to link to the alternative change notice draft I posted in #2083547-218: PSR-4: Putting it all together
https://piratenpad.de/p/Drupal-PSR-4-change-notice
Why is this on a separate site?
I post this here because it contains a useful summary of the proposed changes.
Comment #2
catchYes I don't think it was ever agreed to leave PSR-0 support for modules in, we should absolutely remove that once core is converted.
Comment #3
Damien Tournoud CreditAttribution: Damien Tournoud commentedAgreed with PSR-4 only for module namespaces, and whatever else for 3rd party code, once we support that properly (probably via composer).
Comment #4
ianthomas_ukI've changed the proposed resolution in the issue summary to "Drop PSR-0 support shortly before beta1.".
So what we're really after here are any good arguments against that policy. I opened this to keep noise down on the other issue, as there were people arguing for both 'yesterday' and 'never'.
FWIW I'm in favour of the current policy, because it gives modules a chance to update, but we don't end up with some modules using 'lib' and others using 'src', which would be rather confusing.
Comment #5
catchI don't think there's anything to decide here, the original decision was taken on the basis of not having dual support, so this is just a case of following up once that + the move-around patch is committed.
Comment #6
larowlan-1 for me.
Core shouldn't ship with psr0 modules but that shouldn't stop contrib in my opinion.
Edit:@donquixote points out that this leaves us with extra cruft and handling. So I retract my -1 and quit whining.
Comment #7
donquixote CreditAttribution: donquixote commented@larowlan: Anyone who is -1 about this should at least respond to the points mentioned in #1.
Comment #8
larowlan@donquixote, you are right, amended my comment.
Apologies.
Comment #9
webchickThanks for spinning off a sub-issue for dedicated discussion around this, since this seemed to raise some hackles in the other issue.
IMO, what we definitely *don't* want is contrib being a mishmash of:
There should be one standard way for all Drupal modules to register classes, and it should be used consistently across both core and contrib so people instantly know where to look for what they're looking for when coming to a module fresh. My understanding is that one standard way, based on analysis of what other projects are doing on Github, is
modules/foo/src/Class.php
. And it sounds like that's what this patch is proposing to do, by eliminating the ability to do the other two options. So +1 for that, learnability++.However, what we definitely *do* want is to support contrib pulling in other libraries that happen to use PSR-0 file/folder structure. For example, the Drupal Mollom module pulling in the hypothetical generic, non-Drupal-specific Mollom library from Github or whatever that uses PSR-0. So as long as this patch doesn't prevent that from happening, I think we are good here?
Comment #10
webchickIncidentally, my vast preference would be for this change to happen alongside the conversion of all modules to PSR-4, and for both changes to happen just prior to beta1. Keeping a transitional semi-BC layer around longer than that is only going to make porting modules (which we hope to have a huge push around right after beta) confusing and difficult.
Comment #11
Crell CreditAttribution: Crell commentedFile me as another "me too" to the original plan: Eliminating PSR-0 for modules entirely for consistency, on whatever schedule minimizes the amount of work people have to do.
Comment #12
pounardI don't think eliminating PSR-0 brings consistency. My opinion is that we should leave contrib live with whatever the developer chooses.
If you think about it, classloading is just about loading classes, and this duplicate logic is supposed to be contained into the classloader, I don't think it's so much duplication, the only important point is that it remains isolated in the loader.
In the end, the probably best way of handling class loading is to remain Composer compatible in all ways, if core ships its own loader, it should be fully compatible (PEAR, PSR-0 and PSR-4) if not, it sound rather useless doing our own if the main feature of it is loosing features.
Comment #13
donquixote CreditAttribution: donquixote commented@pounard:
The class loader is always going to be the original one from Composer, which supports both PSR-0 and PSR-4 (and classmap).
The question here is which namespaces we register to the class loader by default.
Please read the second part of post #1.
Comment #14
xjmThis is a beta blocker.
Comment #15
xjmComment #16
xjmThis is postponed on #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4, which is itself postponed on "break everyone's stuff later rather than now".
Comment #17
xjmComment #18
pounard@#19 I have to admit I didn't fully read anything else than "Drop PSR-0" which not a good title if the question is not about dropping PSR-0 but automatically registering it for modules.
I'm OK about registering namespaces explicitely, but it'd have to be something simple (such as a flag in the module .yml file), but this sounds quite surprising:
If the module is registered and has registered classes, no matter how they are registered, plugin system and unit testing should be able to find them, no matter how classes are loaded. I quite don't understand why plugin discovery wouldn't be able to find classes if they are loaded using PSR-0 instead of PSR-4 ? If the plugin system relies on the module loader, it sounds like an encapsulation problem?
Comment #19
catch@pounard, the plugin system looks in specific directories for plugins, not every single directory containing a class.
Clarifying the title.
Comment #20
pounardIt's not based on namespaces but on folders?
Comment #21
catchIt uses namespaces + a directory suffix.
Comment #22
xjmComment #23
donquixote CreditAttribution: donquixote commentedComment #24
donquixote CreditAttribution: donquixote commentedHow urgent is this issue, exactly?
Is it important to commit it before the next alpha / DrupalCon, or would it be ok to keep the dual support around for another e.g. 3 weeks?
E.g. the switch-psr4.sh script is designed to also port contrib modules, so it could still be useful for a transition phase.
On the other hand, porting contrib modules is not that hard because you usually only have one directory to move.
One idea I had though was to add a test to make sure that no further PSR-0 class files are added in future core patches, and maybe a requirements check that contrib does not add PSR-0 folders?
Maybe this could even be useful when we no longer register PSR-0 folders, because it would give a more useful warning/message than just a class not loading.
Looks like devel and admin_menu still use PSR-0.
I don't know which other modules to check.
Comment #25
Eric_A CreditAttribution: Eric_A commentedI did a quick test in contrib and it seems that contrib testing right now does not discover PSR-4 PHPUnit.
A quick search did not reveal any issue for this in the testbot or PIFR queue, but maybe I'm missing something.
See #1674290: PSR-0 tests in contrib modules are not found for an earlier story on contrib test discovery.
Comment #26
sunSee this related issue, @Eric_A.
Comment #27
xjmWe'll need to postpone this on #2273481: Contrib PSR-4 PHPUnit tests are not picked up by PIFR as well. With that, it might not be possible to get this in until after Austin.
Comment #28
webchickWell. Or we could not. PSR-4 affects 100% of module developers, PHPUnit in 8.x modules affects maybe 20 people currently. We definitely need to add that to the list of d.o blockers for 8.0 though.
Comment #29
webchickAh, ok. This is the "drop PSR-0 support" not the "move stuff" issue. That works then.
Comment #30
effulgentsia CreditAttribution: effulgentsia commentedComment #31
int CreditAttribution: int commented#2273481: Contrib PSR-4 PHPUnit tests are not picked up by PIFR is now fixed
Comment #32
catchComment #33
donquixote CreditAttribution: donquixote commentedHere is a patch. I hope I found all places that need to change.
I left some style inconsistencies, where we should decide which is the best solution.
Comment #34
donquixote CreditAttribution: donquixote commentedThe script is still there. I think we should keep it around for another few weeks so contrib has a chance to use it.
Comment #35
xjmComment #36
xjmThat sounds reasonable @donquixote. Let's file a postponed followup issue and schedule it for a later date?
Comment #37
donquixote CreditAttribution: donquixote commented@xjm: I agree.
#2277739: Remove core/scripts/switch-psr4.sh
Comment #38
donquixote CreditAttribution: donquixote commentedGreen!
Issue summary still says postponed until #2247381: Update all existing change records for PSR-4 support.
But the issue is un-postponed since #31.
Is #2247381 really a blocker for this issue, or should we change the issue summary?
Comment #39
catchPersonally I think this change is orthogonal to the change notice, both are marked as beta blockers and there's no code dependency at all (obviously).
Comment #40
cweagansI agree with catch. This seems like a separate task. Assigning to myself to review the patch.
Comment #41
cweagansI've looked through this and everything looks great. Tests are passing. I think this is ready to go.
Comment #42
xjmComment #43
xjmComment #44
alexpottCommitted 65b30c0 and pushed to 8.x. Thanks!