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:

User interface changes

N/A

API changes

See https://drupal.org/node/2246699.

Postponed until

CommentFileSizeAuthor
#33 D8-2247287-33-drop-psr0.patch15.35 KBdonquixote
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

donquixote’s picture

Re-posting from #2083547-208: PSR-4: Putting it all together

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.

And from #2083547-225: PSR-4: Putting it all together

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.

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?

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.

I post this here because it contains a useful summary of the proposed changes.

catch’s picture

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

Damien Tournoud’s picture

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.

Agreed with PSR-4 only for module namespaces, and whatever else for 3rd party code, once we support that properly (probably via composer).

ianthomas_uk’s picture

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

catch’s picture

Title: [policy, no patch] Decide if and when to remove PSR-0 support » Drop PSR-0 support for modules
Status: Active » Postponed

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

larowlan’s picture

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

donquixote’s picture

@larowlan: Anyone who is -1 about this should at least respond to the points mentioned in #1.

larowlan’s picture

@donquixote, you are right, amended my comment.
Apologies.

webchick’s picture

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

- modules/foo/src/Class.php
- modules/foo/lib/Drupal/foo/Class.php
- modules/foo/some/other/thing/Class.php

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?

webchick’s picture

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

Crell’s picture

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

pounard’s picture

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

I don't think eliminating PSR-0 brings consistency. My opinion is that we should leave contrib live with whatever the developer chooses.

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.

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.

donquixote’s picture

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

xjm’s picture

Title: Drop PSR-0 support for modules » [meta] Drop PSR-0 support for modules
Issue tags: -beta target +beta blocker

This is a beta blocker.

xjm’s picture

Title: [meta] Drop PSR-0 support for modules » Drop PSR-0 support for modules
xjm’s picture

Title: Drop PSR-0 support for modules » [PP-2] Drop PSR-0 support for modules

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

xjm’s picture

pounard’s picture

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

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.

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?

catch’s picture

Title: [PP-2] Drop PSR-0 support for modules » [PP-2] Drop automatic PSR-0 support for modules

@pounard, the plugin system looks in specific directories for plugins, not every single directory containing a class.

Clarifying the title.

pounard’s picture

It's not based on namespaces but on folders?

catch’s picture

It uses namespaces + a directory suffix.

xjm’s picture

Issue summary: View changes
donquixote’s picture

Issue summary: View changes
donquixote’s picture

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

Eric_A’s picture

Looks like devel and admin_menu still use PSR-0.
I don't know which other modules to check.

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

xjm’s picture

Issue summary: View changes

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

webchick’s picture

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

webchick’s picture

Ah, ok. This is the "drop PSR-0 support" not the "move stuff" issue. That works then.

effulgentsia’s picture

Issue summary: View changes
int’s picture

Status: Postponed » Active
catch’s picture

Title: [PP-2] Drop automatic PSR-0 support for modules » Drop automatic PSR-0 support for modules
donquixote’s picture

Status: Active » Needs review
FileSize
15.35 KB

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

donquixote’s picture

The script is still there. I think we should keep it around for another few weeks so contrib has a chance to use it.

xjm’s picture

Issue summary: View changes
xjm’s picture

That sounds reasonable @donquixote. Let's file a postponed followup issue and schedule it for a later date?

donquixote’s picture

donquixote’s picture

Green!

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?

catch’s picture

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

cweagans’s picture

Assigned: Unassigned » cweagans
Issue summary: View changes

I agree with catch. This seems like a separate task. Assigning to myself to review the patch.

cweagans’s picture

Assigned: cweagans » Unassigned
Status: Needs review » Reviewed & tested by the community

I've looked through this and everything looks great. Tests are passing. I think this is ready to go.

xjm’s picture

xjm’s picture

Issue summary: View changes
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 65b30c0 and pushed to 8.x. Thanks!

diff --git a/core/tests/Drupal/Tests/Core/Plugin/DefaultPluginManagerTest.php b/core/tests/Drupal/Tests/Core/Plugin/DefaultPluginManagerTest.php
index ac0f772..86cb5cc 100644
--- a/core/tests/Drupal/Tests/Core/Plugin/DefaultPluginManagerTest.php
+++ b/core/tests/Drupal/Tests/Core/Plugin/DefaultPluginManagerTest.php
@@ -65,8 +65,7 @@ protected function setUp() {
     );
 
     $this->namespaces = new \ArrayObject();
-    $this->namespaces['Drupal\plugin_test']
-      = DRUPAL_ROOT . '/core/modules/system/tests/modules/plugin_test/src';
+    $this->namespaces['Drupal\plugin_test'] = DRUPAL_ROOT . '/core/modules/system/tests/modules/plugin_test/src';
   }
 
   /**

  • Commit 65b30c0 on 8.x by alexpott:
    Issue #2247287 by donquixote: Drop automatic PSR-0 support for modules.
    

Status: Fixed » Closed (fixed)

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