Update

PSR-4 is now implemented.
See the documentation page for PSR-4 in Drupal 8.

Quick summary

Currently, Drupal 8 is using PSR-0 to organize class files in a directory structure, both for core and for modules.
Some people complain that this results in an unnecessarily deep folder structure, causing a mental and motoric overhead. Others say that this is not a big deal, and not worth doing anything about it.

After a long discussion, it was proposed to use PSR-4 for module-provided classes, instead of PSR-0.
There is a doc page to show how PSR-4 would look like in Drupal.

PSR-4 is officially approved by the PHP Framework Interoperability Group since 16:35, Dec 3, 2013.
It is merged into Composer since Jan 2, 2014.

An implementation for this conversion in Drupal has been developed in #2083547: PSR-4: Putting it all together and is constantly being rerolled and kept ready. The patch covers all the aspects of this change (class loading, class discovery). It contains a script to make rerolling patches / local changes easier, and it allows for a conversion phase where both PSR-0 and PSR-4 are equally supported, to make life easier for contributed modules.

The move to PSR-4 has been approved/decided two times now (Dries in #111 and #302), but was then put back into "to be debated" status.

A version of Drupal with the conversion already done can be browsed on github.

Arguments pro / con

Learning curve / entrance level

Both PSR-0 and PSR-4 have aspects that could be confusing to a beginner, depending on the background and expectations.

PSR-0 is said to be more logical for people coming from other languages (python, java, ruby), where it is normal to have the full namespace structure reflected in the directory structure within a package.
On the other hand, people on IRC #composer repeatedly ask why they can't abbreviate the two redundant directories. So apparently this is controversial.
Besides, there is the special case for the underscore, which is an additional thing to learn.

The deep directories help to visualize the namespace structure when browsing within a specific module. On the other hand, they also make it take more time to completely explore a directory structure (e.g. when using a web-based repository explorer)
When browsing a complete Drupal installation, the additional subfolder levels might feel redundant and confusing.

PSR-4 lacks the 1:1 match of the directory structure with the full namespace structure. People need to get used to that.
On the other hand, the flat directories make it take less time to explore a project, e.g. with a web-based explorer.

Casual developer

This would be someone who sufficiently understands the directory structure of either PSR-0 or PSR-4, or at least is no longer scared of it, but is not always using the most efficient tools and shortcuts. It could also be an experienced developer who is forced to work with tools that don't provide fancy shortcuts.

Typical activities: Working with a filesystem explorer, clicking around in github or drupalcode.org, using a regular file editor (gedit), using the commandline (but not in the most efficient way), using a file-open dialog, looking at error logs, writing a small custom module with a simple text editor.

Most of the arguments for the supposed DX benefits of PSR-4 apply in this category. The activities above expose the developer to a more verbose directory structure, adding visual clutter, requiring more actions (clicks, page visits, etc), and more mental capacity to remember and work with the more verbose paths.

Advanced developer

This would be someone who not only sufficiently understands the directory structure of either PSR-0 or PSR-4, but also makes the most efficient use of the available tools.

Typical activities: Using an IDE with shortcuts. Using the commandline with shortcuts and autocompletion, maybe even custom commands.

It was argued that the impact of the deeper directories with PSR-0 is minimal, if you use the tools efficiently (e.g. tab-tab in the commandline).

On the other hand, the developers behind Composer or the PHP-FIG group who pushed for PSR-4 (and also invented the "target-dir" setting in Composer) seem to hate the deep directories just as much as the "casual" developer.

Interoperability?

Composer and the rest of PHP will support PSR-4 asap, so interoperability is not a thing to worry about.
It could even be said that we are better off with PSR-4, since Composer will give this priority in the autoloader.

Cost of the conversion

The main work on the patch is already done. It passes all tests and has been reviewed by a number of people.
However, switching from PSR-0 to PSR-4 also means that pending patches and local branches need to be updated / rerolled.

Conversion of core patches: For every patch, you would
- git checkout a version of drupal before the switch to psr-4, as a new (temporary) branch.
- git apply the patch, commit to the new branch
- git rebase 8.x. This should actually preserve all the local changes to existing files, because git understands if a file has been moved.
- php core/scripts/switch-psr4.sh, and commit. This is for files that were added in the patch or local branch.
- git diff 8.x to produce a new version of the patch.
- Upload the new patch.

This should work in most cases. But it still means manual intervention for every pending patch in the queue.

Conversion of contrib modules:
Besides, contrib modules that already were ported to D8 need to be ported to PSR-4, including all their pending patches in the queue.
This can be done in a similar way as the core patches above.
The good thing: There will be a conversion phase of e.g. one month or more, where the PSR-0 version of a module will still work. So there is no pressure to port these modules, and it won't suddenly break devel or admin_menu.

Implementation

Implementation details are in #2083547: PSR-4: Putting it all together.
Rough steps:

  1. Prepare a Drupal release where module-provided classes can live in PSR-0 or PSR-4 likewise.
  2. Run a script (included in that release) to move the modules to their new PSR-4 location.
  3. Use this script (and git rebase) to re-roll patches.
  4. Clean up, and make PSR-4 the only way.

Originally proposed alternatives

Other options proposed in the past can be found in older revisions of the issue summary.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

(5) is not PSR-0 compliant, even if PSR-0 loaders might work on it:

A fully-qualified namespace and class must have the following structure \\(\)*

Looking at this again, (2) is really the only decent solution for us. I have been advocating that all along. PSR-0 is just not right for plugin systems.

Damien Tournoud’s picture

Issue summary: View changes

a

RobLoach’s picture

(5) is not PSR-0 compliant, even if PSR-0 loaders might work on it:

It does work though! I mentioned that it might conflict with other vendor code as a con above. Think of the Vendor name as the module name:

\<Vendor Name>\(<Namespace>\)*<Class Name>

The \(<Namespace>\) part is optional. Conflicts could happen with other vendors/module names.

Also, just added #7: Package-Oriented Autoloader, which aims to help in this certain use case.

msonnabaum’s picture

Please don't try to get rid of lib.

7 is the only sane option, as it removes the one thing that's really unique to PSR-0. Past that, this is just structuring classes like almost every other language does.

If we want to pursue 7, I think that's fine. Anything else is just a distraction that has the potential to introduce a very confusing Drupalism. We have way bigger DX issues to deal with.

Damien Tournoud’s picture

(7) is basically a proposal to standardize (2). That's just fine.

Please don't try to get rid of lib.

I'm not sure how that's relevant? (7) can go both ways in removing lib.

RobLoach’s picture

Status: Active » Closed (works as designed)

And we're currently using #1, which works by design. This isn't an open issue, just more of an explanation of why we're using #1.

Damien Tournoud’s picture

Category: support » task
Priority: Minor » Major
Status: Closed (works as designed) » Active

Well, now that we have a decent contender, (7), let's keep the discussion going.

Crell’s picture

As the Drupal rep to FIG, I'm pushing for some variation of 7 exactly to help us reduce the number of directories in modules.

My position is "if FIG does #7, do that. If FIG can't get that together, stick with PSR-0 (option #1)".

Damien Tournoud’s picture

Well, the usual way those things work is: if a large project like Drupal adopts #7, FIG will adopt it. So, let's just move forward with this :)

msonnabaum’s picture

If the discussion is purely around 7, I think that's fine. However, I really don't want to have an issue where all of these options are on the table, because some of them are horrible.

I still get flashbacks from the original PSR-0 thread.

msonnabaum’s picture

I do agree with #8. If we like it we should move forward. If we can't influence FIG, I'm not sure why we pay attention to it.

RobLoach’s picture

If the discussion is purely around 7, I think that's fine. However, I really don't want to have an issue where all of these options are on the table, because some of them are horrible.

I've seen them suggested though, and that's why they're on this list. If they're not documented as to why we're not using them, then they're just keep on being suggested. This article is meant to show why lib is what it is, not bring all of the old suggested solutions back on the table.

If we like it we should move forward. If we can't influence FIG, I'm not sure why we pay attention to it.

Anyone can make a suggestion, and follow it up with a proper PR. Whether it's a sound solution and something that we actually use is the question. If not, then we'll just stick with lib.

if a large project like Drupal adopts #7, FIG will adopt it. So, let's just move forward with this

However, we have seen Drupal adopt a projects and then see them fade. SimpleTest is an example of this. Drupal is not an example of best practices. No project really is. PHP-FIG aims to help that... What I'm getting at is popularity != best practice.

Damien Tournoud’s picture

However, we have seen Drupal adopt a projects and then see them fade. SimpleTest is an example of this.

Drupal never adopted SimpleTest. We got inspired by it, figured out that it was crap and ended-up writing our own. The amount of time Drupal ever used Simpletest is close to zero.

In this case, we *know* that PSR-0 is not adapted to our extension structure. There should not be any question about that. We also know that before POA there was no alternative. Now is the time to move forward and define something that actually works for us.

quicksketch’s picture

I also don't think any of the options presented are valid *except* for #7. However, until it becomes an official spec as suggested in the Package-Oriented Autoloader link, it essentially *is* writing our own package manager (same as Option #2), because we'd be implementing something that isn't part of PSR-0 and isn't compatible with PSR-0 autoloaders.

I'm in agreement with Damien in comment #12 that basically we know PSR-0 is not a good fit for our needs. It's convenient that we can use existing auto-loaders, but it's shifted the problems of core (providing an autoloader) onto module developers, who now need to make completely unnecessary pattern directories before they can start coding. The number of users who will write (or use) alternative autoloaders verus those who write modules has to be outnumbered 10,000 (or more) to 1. It takes an afternoon to write an autoloader, and the consequences of simply using an off-the-shelf one will affect every Drupal developer on a daily basis for years.

The chief advantage of PSR-0 is interoperability between different systems. If something is actually reusable outside of Drupal (e.g. everything in core/lib/Drupal/Component), then by all means is should use PSR-0 and be loaded by an existing PSR-0 class loader. But at least for Drupal 8, individual Drupal modules will not be useful outside of Drupal itself. Therefor in the case of modules, it makes complete sense for us to use an autoloader that works for our needs. We do NOT need to wait on FIG to come up with a solution beforehand, because at this point we're not dealing with interoperability: we're only talking about Drupal loading its own non-reusable and specific classes. In the mean time, we can use our experience building this system to influence FIG so that our needs are addressed in the next iteration. Hopefully that means that in Drupal 9, if modules become useful outside of Drupal itself, we can switch to using new existing autoloaders that work off-the-shelf for our needs.

And lastly, *even if* somehow a module could make itself useful outside of Drupal and need to be loaded via an existing PSR-0 autoloader, we could make that a possibility for projects that use Composer simply by putting a composer.json file (example) in the module's root directory that sets a "target-dir" property (line 24 in the example) to place the module in a PSR-0 compatible directory structure. Then each module could be pulled out by Composer and reused independently, identical to what's available with Symfony components today and what we've done ourselves (#1424924: Use Composer for updating Symfony components (without removing Symfony code from repo)).

EDIT: Added additional links.

webchick’s picture

For now, just tagging. Will mull on this some.

msonnabaum’s picture

I'd like to clarify a few things about PSR-0 because I feel like we're confusing parts of it.

From my perspective, these are the two most important parts of PSR-0, and the parts we're disucssing here:

A fully-qualified namespace and class must have the following structure \\(\)*

Each namespace separator is converted to a DIRECTORY_SEPARATOR when loading from the file system.

I think the first is debateable, and mostly what we're talking about here, which is addressed in #7. This requirement is a bit awkward for modules, and it's not always found in other languages that have a similar layout.

The second however, is quite standard. A 1:1 mapping of namespace to directory structure makes a ton of sense. There's no advantage to straying from this. If we think our directory structure is too deep, then our namespaces are too deep. It has nothing to do with this requirement

Also, everything related to autoloaders have to do with the second requirement, so if we're discussing the first, we should stop talking about autoloaders.

RobLoach’s picture

We don't have to wait for FIG to figure things out. If we want to come up with a solution, we should. There's nothing holding us back from that. If the ideas align with that from the Package-Oriented Autoloader proposal, then we'd gain huge benefit in working with the guys who are talking about it.

This also isn't something that has to be limited to Drupal core either. As I mentioned above, donquixote is always open to new ideas with XAutoload. Lots of the ideas introduced there were pushed forwards into Drupal core too.

@quicksketch You might enjoy these links that are rather unrelated to class loading:

#1826054: [Meta] Expose Drupal Components outside of Drupal
This allows having Components appear in their own repository. I created a git subtree split of /core here as a demonstration of using it with /core: https://github.com/robloach/core
composer/installers#65: Install modules/themes with Composer
This is a pull request to composer/installers to allow modules to be installed with Composer.
AmyStephen’s picture

There are a number of different issues at play:

1. Shared packages

IMO, FIG and PSR-0, *only* relate to packages that can be used between projects (much like what you find on Packagist.)

FIG focuses (and should only focus, IMO) on interoperability. The reason the first two nodes are needed, for example, Drupal/Package-name, is in those situations where the package could be installed by other projects. In that case, Composer will place the package into a vendor folder and the namespacing (and matching folders) keep things separate.

2. Code that is NOT shared

IMO FIG and PSR-0 does not address that code, nor should it. Again, FIG should focus on interoperability and should avoid "meddling" in project-specific decisions.

In my case with the "Standard" Molajo package, I did NOT want Molajo/Package-name in my namespace. It's not code that anyone else is going to use so it's not likely going to collide with other namespaces. I just follow the folders for classes that aren't shared -- but are only part of my package.

For example:

namespace Extension\Theme\Foundation\Plugin\Foundation;

https://github.com/Molajo/Standard/blob/master/Extension/Theme/Foundatio...

3. Class Loader

It really comes down to an autoloader. I added a small hack to use a single class loader that could process both the PSR-0 standard -- and my approach.

https://github.com/Molajo/Standard/blob/master/ClassLoader.php#L233

Also, the SPL Loader is the basis of the class loader -- and it can handle multiple classloaders, so a project could allow composer to handle all the Vendor packages (and PSR-0) with it's delivered Classloader -- and add in another class loader specific to Drupal's needs.

+++++

Sometimes, I think FIG forgets it's scope and, in doing so, bites off more and more of those decisions best left to the project. Drupal-specific classes (those not distributed via Packagist for use outside of Drupal) should not have to comply with PSR-0, it's out of scope, IMO. There shouldn't have to be yet another PSR standard to handle these, either, since it has nothing to do with interoperability, IMO.

Glad to see you guys are discussing. I've talked with Larry about this, too. Definitely agree that those long paths are a little ridiculous for community devs when there is no concern about name collusion. But I don't see why a solution needs to involve FIG -- unless it's a casual group who are deciding to collaborate outside of the standards body on a single class loader that can handle PSR-0 -- and the way Projects want to handle their own classes.

quicksketch’s picture

The second however, is quite standard. A 1:1 mapping of namespace to directory structure makes a ton of sense. There's no advantage to straying from this. If we think our directory structure is too deep, then our namespaces are too deep. It has nothing to do with this requirement

There is an advantage to this. Our modules don't all live in a single directory. If Drupal were structured so that *all* modules were simply in /Drupal/Modules/[ModuleName] then we wouldn't have a problem with unnecessarily repetitive and deep nested directories. However since Drupal specifically categorizes modules into core/modules, modules, and sites/example.com/modules, when using PSR-0 we're forced to duplicate the entire PSR-0 directory structure again inside of each module's directory. Because of our adherence to 1:1 mapping between namespaces and directory structure, we've added (so far) 292 directories within /core/modules that only contain a single directory each. As a contributed module developer, this is a significant hassle.

Also, everything related to autoloaders have to do with the second requirement, so if we're discussing the first, we should stop talking about autoloaders.

I'm talking almost entirely about the second requirement. The scoping of our namespaces is not a problem for me. It's the nested directory structure that drives me bananas. As we move more and more towards plugins, we'll end up with "modules" that don't have .module files or /includes directories. The only PHP will be inside of [module_name]/lib/Drupal/[module_name], which is just ridiculous. It's the most commonly modified portion of code; it should live at the top of the module directory.

chx’s picture

I must be missing something. What's wrong with

  1. core/lib/Drupal/Core/ClassName.php
  2. core/modules/Drupal/system/ClassName.php
  3. modules/Drupal/pathauto/ClassName.php
  4. sites/foo/Drupal/mymodule/ClassName.php
  5. profile/Drupal/myprofile/ClassName.php
  6. drivers/Drupal/mydriver/ClassName.php

which would require very very few Drupal roots (the above show in bold all six of the possible PSR-0 roots) added to the PSR-0 autoloader (yay, performance!), would lead to shallow directory structure, wouldn't break PSR-0 and altogether great? Except for vendors but let's leave that for another day.

Edit: this seems to be 4. although that is not explained as much as I did here, and 7 has no examples either. Listed cons:

  1. We don't have control over where users install modules. - and why do we care? the above covers all the places where they could. Or are you saying that if they don't put a Drupal directory in sites/foo then it won't work? News at 11. If they didn't create a modules directory and put it there, it didn't work earlier. Cry me a river that you need to create Drupal/modules now and not just modules. Please. Then ask whether I care. Hint: no.
  2. Classes in same directory as functional code? - I won't even waste time refuting this. Code is code. No foul, no harm if it's together. To the contrary, it's much easier if it's together.
  3. Broken namespaces when the module has a dash in the name - Yeah because that was SO allowed until now. The module name goes into the function name for hooks and PHP doesn't allow dashes: ".... starts with a letter or underscore, followed by any number of letters, numbers, or underscores".

So, there is no valid argument against it AFAIK.

Gábor Hojtsy’s picture

@chx: sites/foo can have profiles, modules and themes as well. Same for core/modules + core/themes and /modules + /themes. At least the current class loader works with classes in themes.

chx’s picture

What if we put a Drupal directory in the root? Pretty (but changes the current namespaces to include modules):

<strong>.</strong>/Drupal/modules/pathauto/ClassName.php
<strong>.</strong>/Drupal/core/modules/system/ClassName.php
<strong>profiles/awesome</strong>/Drupal/themes/ClassName.php
<strong>sites/foo</strong>/Drupal/themes/sitetheme/ClassName.php

We have modules. Themes. It's all there. There are three roots altogether, dot, path/to/the/installed/profile, sites/foo. It's all there, it's consistent and even simpler than #19.

msonnabaum’s picture

/ (╯°□°)╯︵ ┻━┻

chx’s picture

I apologize. I have unpublished (but not yet deleted my comments, I will immediately also unfollow this issue. It's unfortunate it appears in the major task queue, likely I will abandon that too to avoid any contact. It is not my role to architect anything in Drupal 8 and I have overstepped these boundaries probably because this bothers me such much but then again a lot of things bother me a lot in Drupal 8 and it's easier if I do not distract the conversation.

RobLoach’s picture

Priority: Major » Minor

This was never intended to be in the major queue, nor should it be.

Damien Tournoud’s picture

Priority: Minor » Major

Well. It was intended to be since I classified it as such in #6.

Crell’s picture

Priority: Major » Normal

Take 407 at this question when we've survived this long is not major. Really.

We've rejected "roll our own spec" before, multiple times. Re-introducing "roll our own" is just wasting everyone's time. The only new data here is that FIG is likely to approve a new alternative soon, which we should consider once using it would no longer be "rolling our own".

msonnabaum’s picture

Yes, this is in no way major.

quicksketch’s picture

I totally agree with @AmyStephen in #17. Drupal modules aren't about interoperability (not yet at least). We don't need a spec for something that isn't usable outside our system. Waiting on FIG to approve something that has nothing to do with any other system doesn't make any sense. However unlike #17, since we're pretty well tied into using Symfony at this point, I don't think that we should use a single class loader that can autoload based both on PSR-0 and Drupal modules. Because Symfony's ClassLoader class uses private variables for keeping track of its prefix list, we couldn't extend their class to support non-PSR-0 directory structures even if we wanted to.

So instead, I think the approach that is most feasible is to use Symfony's ClassLoader for loading everything PSR-0. People can continue to swap that out with other PSR-0 loaders if they desire for efficiency. Then additionally we add a separate loader that can deal with the dynamic paths of modules. All the namespaces can stay the same because we define the namespaces, and the end result is nothing changes (for developers) except we eliminate "lib/Drupal/[module_name]" inside each individual module.

webchick’s picture

I can't parse that FIG discussion. How close/far away is that from being approved, and what's an example of what end product would look like? (If the issue summary could be updated to include this information, that would be great.)

Dave Reid’s picture

the end result is nothing changes (for developers) except we eliminate "lib/Drupal/[module_name]" inside each individual module

How would this support the use case of "my module wants to provide core plugin implementations of my new plugin type" where the core support for the module would live in modules/mymodule/lib/Drupal/[core module]/Plugins/... and the module's plugin lives in modules/mymodule/lib/Drupal/mymodule/Plugins/... ?

quicksketch’s picture

How would this support the use case of "my module wants to provide core plugin implementations of my new plugin type"

Hm, good question. Since the directory structure and namespaces are still tied under the proposal in #28, you'd end up needing to put the core module's implementation of your module's Plugins under your module's directory and namespace.

So taking something that is popular and likely to exist in Drupal 8, if you had Rules module implementing plugins on behalf of Node module, you'd end up with a directory structure like this:

modules/rules/Plugin/rules/event/node/NodeSaveEvent.php

And the namespace would match:

\Drupal\rules\Plugin\rules\event\node\NodeSaveEvent

In order to identify that this plugin is being provided on behalf of another module, you'd probably need to identify the original module in that plugin's @Plugin() annotation (or whatever @Plugin() ends up becoming for that plugin type after #1966246: [meta] Introduce specific annotations for each plugin type).

I admit I didn't think of this scenario. I'll try to think if there are any other options for handling it more gracefully. Ideally Rules could continue to provide something in Node module's namespace, resulting in this:

\Drupal\node\Plugin\rules\event\NodeSaveEvent

Which is what would happen currently if I'm not mistaken.

quicksketch’s picture

Status: Active » Needs review
FileSize
574.88 KB
18.38 KB

This almost certainly isn't going to apply with the speed of core development these days, but here's a starter patch that shows what a second auto-loader would look like. We have 2 autoloaders now, and 200 fewer nested directories.

I did *not* remove the /lib directory, because it would result in a few namespace conflicts when there is a [module_name]/tests directory, which would conflict with [module_name]/lib/Tests. Keeping /lib may also help consolidate PHP code to a single location (eventually), and would allow PHP code to be separated from the JS/CSS/etc files that are necessary on the front-end only.

Answering @DaveReid in #30, this patch shifts the responsibility of registering directories on behalf of other modules onto the implementing module. It does not enforce a hard location. So if "anotherModule" wanted to register classes in the Drupal\node\* namespace, it would look like this:

$class_loader = module_classloader();
$class_loader->addModule('node', drupal_get_path('module', 'anotherModule') . '/modules/node/lib');

Because this patch moves a million files, I've attached a patch-only version without renames also for easier review. We're still missing a good amount of accurate documentation and this version don't provide an APC alternative like the Symfony loader yet. It just demonstrates the amount of work needed to manage two autoloaders.

quicksketch’s picture

Oh rats, my first patch doesn't include the DrupalModuleClassLoader.php file. The second patch does though. You'll need to look at the second patch to get the full picture.

webchick’s picture

Just re-uploading the second patch without the renames since that first patch was incomplete, to get an idea of the size, etc.

webchick’s picture

Before:

Deeply nested directories.

After:

Less deeply nested directories.

Needless to say, this is pretty much my favourite patch of the century.

tim.plunkett’s picture

Except that we're back to writing and maintaining our own autoloader. Not only code, but non-PSR-0 approach.

webchick’s picture

Yes. And 25,194 developers don't have to do back-flips to achieve interoperability with code that no one on this earth will ever, ever want to interoperate with.

tim.plunkett’s picture

I also misunderstood this latest patch:

We will now be running two autoloaders, side by side. One for standard PSR-0, and one for "HI I'M A DRUPAL"

msonnabaum’s picture

Which is why we should change the namespaces if we're changing the structure. We dont break autoloaders and we dont introduce a huge WTF inconsistency that no other project will adopt.

webchick’s picture

Yes, one for PSR-0, for core libraries/components, where there's a reasonable amount of re-use possible. And one for modules/themes/profiles that are Drupal-specific and will never, ever be used outside of Drupal, ever. Right tool for the job. PSR-0 ain't it.

Also, that custom code we have to maintain looks like it's about 100 lines, half of which are comments. I'm not that fussed for the *tremendous* DX impact this would bring.

Lack of APC support though does concern me.

quicksketch’s picture

We will now be running two autoloaders, side by side. One for standard PSR-0, and one for "HI I'M A DRUPAL"

Right, but this means that the PSR-0 autoloader no longer needs to deal with hundreds of additional classes provided by Drupal modules. Because we know more about Drupal's module and directory structure than a generic autoloader does, we can eliminate burdening the generic autoloader with hundreds of classes. The new module autoloader on the other hand takes into account the information we already know about modules, skipping the loop and using that module's path that is already known.

So even though this is two autoloaders instead of one, it's not twice as much work. It's shifting the work of the PSR-0 autoloader onto the module-specific autoloader.

Dave Reid’s picture

Hrm, I can see a valid use case for my module wanting to provide an 'implementation' of a PSR-0 plugin, or Drupal component. This seems pretty impossible with the proposed solution. I still think not having a good to way register implementations on behalf of core is something that should be highly considered by the proposed solution, otherwise we're back to requiring you to prefix every class with your module name.

quicksketch’s picture

Hrm, I can see a valid use case for my module wanting to provide an 'implementation' of a PSR-0 plugin, or Drupal component.

Not at all. If you want to register a PSR-0 library, you use the PSR-0 loader:

$class_loader = drupal_classloader();
$class_loader->addPrefix('SomeLibrary', 'path/to/SomeLibrary/lib');

And if you want to register a module's classes, you use the module_classloader():

$class_loader = module_classloader();
$class_loader->addModule('some_module', 'path/to/some_module/lib');

To clarify, the only difference on the surface between our autoloader and a PSR-0 autoloader is that we're namespacing modules under "Drupal\[module_name]" instead of just having "[module_name]" as the top-level namespace. That's why @msonnabaum is saying we should just change the module namespace to be top-level and we could keep using existing PSR-0 autoloaders. However, that means that modules could collide with 3rd party library namespaces. Something that I think is not only likely but would be expected. There are dozens of "integration" modules that do nothing but implement a 3rd party library. If that 3rd party library and the implementing module have the same name (again, very likely), then they'll end up in each other's namespace.

Dave Reid’s picture

I think what I'm saying:

  1. My module provides a new plugin type.
  2. In my module, I want to add a plugin implementation for an optional core module, that should be available only if the core module is enabled.
  3. Right now the information for 'can I load a plugin or not' is based on the module_list() and the module name, which is part of the PSR-0 directories.
  4. This proposal removes that contextual information about 'what module provides this plugin' that we used to use directories for. We currently have 'module' => 'contact' or similar in the plugin annotations but this doesn't do anything.

If this proposal were to be adopted, ensuring that the optional-core plugin implementations are loaded only if the core module is enabled is a blocker to actually using this proposal. We would need to ensure that the plugin managers actually do look at the 'module' key/value in the annotations and check them against module_exists().

Overall it seems like we'd still be using namespaces in the PSR-0 format, but not the actual directories which match the namespaces, which seems really strange to me. :/

quicksketch’s picture

If this proposal were to be adopted, ensuring that the optional-core plugin implementations are loaded only if the core module is enabled is a blocker to actually using this proposal.

That sounds reasonable and something we can write a test for.

We would need to ensure that the plugin managers actually do look at the 'module' key/value in the annotations and check them against module_exists().

I don't think you'd register the namespace for a disabled module at all if it wasn't enabled to begin with. It seems like it would be as simple as this:

if (module_exists('core_module')) {
  $class_loader = module_classloader();
  $class_loader->addModule('core_module', 'path/to/mymodule/modules/core_module/lib');
}

I'm unsure what core does now if you were to include a different, disabling module's plugins inside of an enabled modules /lib directory. I think currently it's get loaded anyway because we only check the status of the parent module, then everything inside of that module's /lib directory is blindly loaded into namespaces. After this patch that problem doesn't happen because /lib contains exclusively the parent module's code.

quicksketch’s picture

This proposal removes that contextual information about 'what module provides this plugin' that we used to use directories for. We currently have 'module' => 'contact' or similar in the plugin annotations but this doesn't do anything.

I'm not clear on how this information is currently provided. Sure the namespace for a plugin currently maps to path/to/module/lib/Drupal/other_module/Path/To/Plugin, but after this patch it would still contain the parent module part of the path: path/to/module/different_directory/other_module/Path/To/Plugin. So as far as detecting "which module provides this plugin based on the include path", it doesn't seem like there's a change.

AmyStephen’s picture

@webchick - in response to your question in #29:

"I can't parse that FIG discussion. How close/far away is that from being approved, and what's an example of what end product would look like? (If the issue summary could be updated to include this information, that would be great.)"

1. Part of the slowdown is this is the first time FIG is "changing" a standard. They are having to deal with rule and process implications. That's going slowly.

2. As far as what could change, we'll see two standards if this passes. PSR-0 will stay the same. The new standard essentially allows you to remove the Drupal/Package-name portion of the path while retaining it in your namespace. This will not impact anything in your Vendor folder.

+++

@tim.plunkett - The spl_autoload -- http://php.net/manual/en/function.spl-autoload.php -- is designed to manage multiple autoloads. Even composer will have to handle that with multliple namespace PSR's.

+++

The other consideration is that Fabien said earlier this week Twig will not be namespaced "anytime soon" - http://fabien.potencier.org/article/68/about-symfony-stability-over-feat... Not sure what that means, but thought I'd point that out.

Not much to do right now except decide between PSR-0 or your own approach for non-shared code and wait and see what comes of this. But, no one can say you're not PSR-0 compliant for code that isn't shared. That's just not what FIG is about.

AmyStephen’s picture

@webchick and @quicksketch -

If I understand the new PSR proposal (Larry correct me if I am wrong) - you should be able to use the new PSR (assuming it's adopted) and get install paths like you pictured http://drupal.org/node/1971198#comment-7312970

That'd be the way to go if you like those paths. That way, if FIG adopts the 2nd standard, you should be okay with the Composer classloader - and if they don't - you're shared code is still compliant - and the non-shared code can be done your own way.

RobLoach’s picture

There has already been so much work put into making class loading easy and extensible in Drupal 8. I understand that it's easy to spend an afternoon to re-implement our own loaders, but that's beside the point of why we are using PSR-0. PSR-0 ensures that we could throw any PSR-0 loader we want at Drupal 8, and it will work. APC, static ClassMaps, Xcache, they're all already supported, tested, working, and will continue to be so in the foreseeable future.

If I understand the new PSR proposal (Larry correct me if I am wrong) - you should be able to use the new PSR (assuming it's adopted) and get install paths like you pictured http://drupal.org/node/1971198#comment-7312970

Correct... The PSR Proposal is aimed to be an extension to the PSR's which allows you to not worry about having the Vendor paths in the PSR-0 directories. Although the namespace is kept intact, and the directory structure doesn't have the extra directory prefixes. Crell resonded to the thread cross-linking this post.

The other consideration is that Fabien said earlier this week Twig will not be namespaced "anytime soon" - http://fabien.potencier.org/article/68/about-symfony-stability-over-feat... Not sure what that means, but thought I'd point that out.

What he means is that if Twig were to switch from PEAR-like namespacing (MyVendor_MyClass) to actual PHP namespaces (MyVendor\MyClass), it would need to be Twig 2.0, which isn't coming anytime soon.

quicksketch’s picture

APC, static ClassMaps, Xcache, they're all already supported, tested, working, and will continue to be so in the foreseeable future.

This is off-topic for this issue, but this isn't true. The way the drupal_classloader() is structured right now you must use the Symfony ClassLoader, with the single option to enable APC or not. See the API for drupal_classloader(). While it takes a parameter, the only value it accepts is the string "apc". Nothing else has any effect.

My version doesn't yet include an APC implementation. In part this is because the selection of our PSR-0 class loader is so bizarre I couldn't bring myself to imitate its implementation for the second custom class loader. But in any case, assuming we fix this and make it so that the class load is truly swappable, you could continue to use any existing off-the-shelf class loader for PSR-0 classes. You would however have to write a custom loader for the Drupal classes. But like I've said, this change is not to make life easier on core developers, it's for the module developers that outnumber core developers by an order of magnitude. It's eliminating unnecessary patterning to make Drupal module development more navigable and learnable.

I understand that it's easy to spend an afternoon to re-implement our own loaders, but that's beside the point of why we are using PSR-0. PSR-0 ensures that we could throw any PSR-0 loader we want at Drupal 8, and it will work.

The point of PSR-0 is interoperability between different projects for things that are reusable. At least for the Drupal 8 cycle, Drupal modules are not reusable. PSR-0 wasn't intended to deal with libraries that could be turned on and off. Nor was it intended to deal with a single project that stores libraries in any number of dynamic locations. We're trying to use a generic tool for a specific problem that doesn't match.

larowlan’s picture

+1 for option 1. Switching now means so much more work, so many broken patches in the queue and would add at least 1 month or more to our ship date surely.
There are plenty of other places in Drupal where convention over configuration exists.
This folder structure at least has a) precedence somewhere else outside Drupal and b) is easily discoverable by browsing the filesystem (ie not hidden inside code).
I should qualify this post by saying, I don't have a CS background. Drupal was not my first experience with php, I started with procedural code in php v3 days but I have found the transition to be relatively straight-forward to the Drupal 8 way.

quicksketch’s picture

PSR-0 wasn't intended to deal with libraries that could be turned on and off.

I'm over-generalizing. I should say that the Symfony ClassLoader was not intended for this purpose (which we could fix upstream). PSR-0 doesn't really have to do enabling/disabling things.

jibran’s picture

a) precedence somewhere else outside Drupal and b) is easily discoverable by browsing the filesystem (ie not hidden inside code).

Well said +1 to everything in #54 and +1 for option 1.

Crell’s picture

Just to put a little perspective in here:

- We spent 6 months over more than a half-dozen issues and on the order of 800 comments (numbers approximate based on my memory from last year, but I don't think I'm exaggerating) settling on PSR-0, leaving numerous "smoking craters" in our wake.

- During that, we chewed through just about every single permutation of pro and con at least 6 times for every conceivable approach, including every single one listed in the OP.

- After all of that, we settled on PSR-0-everywhere. We then put hundreds of hours into converting our code to work with that.

- One of the key themes for Drupal 8 is "fewer Drupalisms".

- Changing the autoloading approach for modules now would mean breaking every single WSCCI conversion patch currently in the queue, plus likely hundreds of other patches. That would severely hurt our velocity, and would mean *fewer important changes make it in by 1 July*.

- It would also mean changing a crapton of documentation we've already written.

- In the WSCCI conversion issues so far, the module PSR-0 standard has yet to cause a problem. No one has complained. Developers that I've never heard of before these issues (so I assume are not core regulars who are used to it by now) seem to be fine with the current structure.

Given all of that... we're talking about reopening ancient wounds, breaking 80% of patches in the queue, reducing our velocity, and getting less important work done by release... for the sake of not clicking two directories in your file browser/IDE. That's the trade-off we're talking about.

Not to put too fine a point on it, but "patch of the century" is not quite the description I would give that trade-off...

quicksketch’s picture

The perspective is that a hundred thousand Drupal developers needing to navigate 2 additional directories 10 times a day, for the next 4 years is more important than 2 weeks of core developer time. Yes.

No one has complained.

You must mean other than the people in this issue, including chx (quite prominently), Damien, webchick, and myself. :P

cweagans’s picture

-1 to changing away from PSR-0 from me as well for the reasons that Larry so articulately describes. PSR-0 is not that bad. We're talking about a couple of extra directories. As mostly a contrib author myself, I don't see this as a problem, and especially not one that warrants such a sweeping change.

RobLoach’s picture

Quick note that there's no reason why these loaders couldn't go in XAutoload. Just like how Libraries isn't quite appropriate for Drupal core, everyone still loves and uses it. No reason why the same thing couldn't apply to XAutoload for Drupal 7/8, hell, even Drupal 6.

The drupal_classloader() function could use some help with clean up and abstraction. Perhaps switch the class_loader variable to a PHP function callback that creates the ClassLoader object for us? Very off topic and should probably be done in a separate issue.

jenlampton’s picture

I would also like to be the voice for everyone who has attended a Twig sprint.

When someone looks at the Drupal 8 file system for the first time the reaction is "why are there all these new empty directories?" and then something like "how deep is my code? ...will contrib have to be like this too?". No one likes it, especially people who are using text editors (which is most PHP developers - Drupal aside).

It's important to keep in mind that as of now now very, very few people have seen the mess we made of the directory structure in D8. Almost every non-core developer I've talked to about it is burdened by it, and they are all eagerly waiting for it to be "fixed".

I don't think saying "we made the decision months ago" is a good enough reason not to re-evaluate the issue. We've been working with this for a while, and though there may be 'good' reasons for it to be the way it is, it's clearly not the right tool for the job we're using it for (as stated several times above) and I agree that putting core's problems on the shoulders of contrib developers isn't going to gain us any fans - quite the opposite.

Also speaking as someone who's patches will all break (and I mean ALL of mine, since none of my stuff is in yet) I think fixing this major headache for contrib is COMPLETELY worth it. It's not too late!

Crell’s picture

I was specifically referring to people actually doing WSCCI conversions, who are not core devs who have been in and around these issues for 2 years: aka, closer to the "great masses of Drupal devs" than chx, Damien, webchick, or myself.

AmyStephen’s picture

@crell - if the new PSR is adopted and you go with that, instead of PSR-0, the same changes would be made that @Webchick/@quicksketch proposed. Right? Not following how everything would break, I guess. What am I missing here?

Crell’s picture

Adopting a PSR-X would eliminate the "Do our own thing" problem, and the double-autoloader problem on the assumption that Composer or ClassLoader end up supporting both (which I think is likely). The "break every patch in the queue" problem would still be there.

quicksketch’s picture

Here's a reroll. It's passing nearly every test on my local. Letting testbot have a crack at it.

An interesting challenge I encountered in fixing all the tests was getting our custom annotation parsers to work (a fairly recent development introduced through #1966246: [meta] Introduce specific annotations for each plugin type). Doctrine has it's own PSR-0 autoloader for loading classes, which we're using to load PSR-0 namespaced annotation parsers. After adding our own autoloader, our module-provided annotation parsing classes were no longer working because they weren't PSR-0. However, then I found that Doctrine has built-in support for *alternative autoloaders*, which I utilized to register our non-PSR-0 classes. Goes to show we're not being entirely unconventional with our desire to use something other than PSR-0.

quicksketch’s picture

Status: Needs work » Needs review
quicksketch’s picture

Here's a diff with just content modifications. Same as #64.

bojanz’s picture

+1 to this.
Having sane paths in a module would lessen the big DX regression/annoyance we introduced.
I've always considered it a necessary evil, but if we can avoid it (FIG argument + "module classes won't be interoperable anyway" argument) that would be amazing.

tim.plunkett’s picture

+++ b/core/lib/Drupal/Component/Plugin/Discovery/AnnotatedClassDiscovery.phpundefined
@@ -57,6 +57,14 @@ class AnnotatedClassDiscovery implements DiscoveryInterface {
+      'modules' => array(),
+    );
+    $annotation_namespaces += array(
+      'psr0' => array(),
+      'modules' => array(),

I pointed this out on IRC, but I'll do so here for everyone else: We can't have module-specific code in a Component. This will need to be moved.

Or won't fixed ;)

msonnabaum’s picture

won't fix so hard

kim.pepper’s picture

won't fix so hard

+1

xtfer’s picture

My position is "if FIG does #7, do that. If FIG can't get that together, stick with PSR-0 (option #1)".

I wholeheartedly agree. Trying to reinvent the wheel on this one is frankly, stupid. We have an adopted standard which we can hang our hat on, and we are thinking of throwing this away because some of us don't like "a couple of extra directories"? I'm stupified.

PSR-0 is "sane". It is standardized, tested in the wild, documented, and commonly understood.

I can use PSR-0 in my Drupal 7 libraries and be sure that it will work in Drupal 8.

Doing anything else would be foolhardy, and will make Drupal look stupid.

quicksketch’s picture

I think there are some incorrect assumptions about what is considered common outside of Drupal. Admittedly, I don't know as much about 3rd-party systems as I should, but upon doing some research, it seems that attempting to use PSR-0 for *everything*, including system-specific code is NOT common in other CMS projects. It IS common in PHP *Frameworks*, where the code is intended to be pulled into another system. But for system-specific code, using PSR-0 is not a popular option.

Joomla

Currently uses its own autoloader (JLoader), which uses a similar concept of starting a 1-to-1 discovery mechanism at the module directory level. There is currently a pull request to add a PSR-0 support to their autoloader, so that it can support *both* module-based class discovery and PSR-0 namespaced libraries.

PyroCMS

From Phil Sturgeon, author of "Catapult into PyroCMS" and who also wrote the official FAQ on PHP-FIG (and has a number of blog posts on PSR-0), just a few days ago, points out that PyroCMS uses an autoloader that supports *both* PSR-0 and a modification that eliminates redundant directories:

The second point allows the autoloader to allow a little extra config to make it system/cms/modules/users/src/User/Model/Group.php, still mapping to Pyro\Module\User\Model\Group.php, which is trivial at best but will make a few of my module developers happier that they don't need to use those two extra folders

Ironically he points to Drupal (and Joomla) as examples of PSR-0 not being forced into situations where it's not applicable. If only we were so fortunate:

That autoloader can try loading all of your custom application classes. Drupal and Joomla use their own autoloaders to load some of their own code using whatever the hell rules they see fit, AND ALSO support PSR-0 to load generic code packages, which makes life easier on the package developers because they can just build to PSR-0 instead of building to work with Zend, or work with Drupal, or work with Symfony - exactly the problem we're trying to avoid.

Elephant CMS

In their autoloader documentation supports *both* PSR-0 for external libraries and a shortened namespace for their own code:

We chose not to follow PSR-0 for core classes since they are not a general purpose library separate from the CMS itself.

Using PSR-0 for everything *is* the Drupalism.

quicksketch’s picture

One more pattern that is apparent from these examples is that the more common approach is only using a single autoloader, which has support for PSR-0 and project-specific autoloading (rather than two separate autoloaders like the current patch). Essentially the same pattern @AmyStephen suggested in #17. There's not much point in being able to use an off-the-shelf autoloader which replaces only half of the autoloading.

msonnabaum’s picture

THis issue has nothing to do with implementation details like how autoloaders do/can work.

quicksketch’s picture

I had an e-mail exchange with Phil Sturgeon to double-check that I was making an accurate statement, but it turns out that he was describing the proposed package manager (i.e. the proposed option #7 in the issue summary) that would allow PyroCMS to shorten its directory structure.

Me:

You mean that PyroCMS is currently using PSR-0 for its module directory structure ("system/cms/modules/users/src/Pyro/Module/Users/Model/Group.php"), but if (or when) "PSR-X" is approved it can be shortened to "system/cms/modules/users/src/Users/Model/Group.php"?

Phil:

Right, that is what I meant. We have PSR-0 folders in each of the modules.

His suggestion is to wait a month and check back in on the situation with the package manager (which he refers to as "PSR-X").

AmyStephen’s picture

@quicksketch -

Without going into too much detail, essentially, you have to think of Joomla as two separate projects now, one is the CMS (which is *not* currently implementing PSR-0) and the other is the Framework (which is).

Each folder in the Framework https://github.com/joomla/joomla-framework/tree/staging/src/Joomla are shared as packages on Packagist https://packagist.org/packages/joomla/database and these packages work great with the standard Composer class loader, no problem.

What is annoying for the Joomla framework is the same thing others are complaining about -- they get those long install paths with doubled Vendor-name/Package-name - the first lower case, the second mixed case:

vendor/joomla/cache/Joomla/Cache/ClassBeginsHere

If Composer searched those first two nodes following vendor when looking for Namespace and allowed mixed case, the Joomla Framework could be installed with paths like:

vendor/Joomla/Cache/ClassBeginsHere

Which is what everyone wants.

Today, I stumbled on a Lithium discussion https://github.com/UnionOfRAD/lithium/pull/917 where they had asked why the composer autoloader isn't working for their perfectly PSR-0 compliant library paths:

/vendor-dir/unionofrad/lithium/

To fix, it has been recommended that they add in a target-dir parameter with another "lithium" folder which will result in a Composer install path of:

/vendor-dir/unionofrad/lithium/lithium

If that class loader was changed so it looks at the first node following vendor Lithium's first non-doubled install path would be fine.

I ran into the same thing -- and I hacked the class loader to start at Node 1 and allow mixed case. For that reason, I don't see the need for another standard -- that one class loader loads all of my classes in the Vendor and outside of the vendor -- it just doesn't "skip" layers now. https://github.com/Molajo/Standard/blob/master/ClassLoader.php#L233

PyroCMS is just starting to explore implementing PSR-0. So, they are also seeing the doubling and considering options.

quicksketch’s picture

Thanks Amy for the explanation. I saw a comment about Joomla using a PSR-0 loader *instead* of JLoader recently and it had me confused, since the CMS still uses JLoader (which is what I linked to earlier).

I'm in complete agreement that using a PSR-0 loader for our *internal* and *non-redistributable* modules doesn't make a lot of sense. We know the namespace of these modules already, so why double-up the directory structure? PSR-0 purists are in the camp that it's for the greatest consistency, which is definitely true, but for most developers like myself, they only see superflous redundancy.

The discussion over in the PHP FIG mailing list seems to have stalled. Everyone seems to like the package loader namespacing idea, but there hasn't been an official proposal put forth yet. I joined the group and queried to see if there was some kind of unmentioned hold-up: https://groups.google.com/forum/?hl=en&fromgroups=#!topic/php-fig/qT7mEy0RIuI.

quicksketch’s picture

Paul Jones has added the Package-oriented autoloader to the PHP-FIG repository. Moving us closer to having a new PSR. It's pretty well summarized in his write-up: allows the root directory of a namespace to be variable (i.e. we can point "Drupal\[module_name]" at an arbitrary directory, without Drupal/[module_name] directories), and removes special underscore handling.

So basically, exactly what we've been suggesting here. While I think it's silly that we're essentially holding off on implementing a good idea until the PHP-FIG group votes that we can use the good idea, I'm going to keep pursuing moving forward the new standard.

quicksketch’s picture

Status: Needs work » Active

Yay, the PHP-FIG group is now officially voting on the new PSR for the "Package-Oriented Autoloader". From feedback in the mailing list, passage seems extremely likely.

I hereby call for votes on the Package-Oriented Autoloader:

The member count at the start of the vote is 27. The quorum requirement is 9 votes. A simple majority of +1 over -1 is required for the measure to pass. The vote started on Fri 10 May 2013 at about 1030am CDT, and closes on Fri 24 May 2013 at about the same time.

quicksketch’s picture

Current votes are 8-to-1 approved (including Drupal's vote). :)

Upon looking at implementing this likely-to-be standard (PSR-4), I'm actually a little bit stumped. It's likely that both Composer will be updated to support PSR-4, but it seems like a possibility that Symfony's autoloader (which we're using now) may end up coming to an early death (or maybe simply not be updated) because Composer solves the autoloading problem more comprehensively. We're considering using Composer's cached autoloader for some of Drupal (but not all of it) in #1818628: Use Composer's optimized ClassLoader for Core/Component classes.

The biggest problem with Symfony's autoloader is that it contains an is_file() check on every single class that is included. This means that a typical Drupal request could end up with hundreds or thousands of filestat() checks to disk. I'm almost certain that we can optimize this somehow, considering Drupal has tools at its disposal for caching this information.

RobLoach’s picture

Some links:

1. Class Loader Adapter
Put this together a while ago when we were switching from the UniversalClassLoader to Symfony's ClassLoader. It allows swapping out class loaders without having to worry about the API around it. Still haven't stuck it on Packagist or anything yet.
2. Composer issue about Package-Oriented Autoloading
Once the PSR is accepted, Composer will adopt it. We can track its progress there.

Would be great to switch completely to Composer's ClassLoader as it does have more features, and the benchmarks proved promising. In the mean time, I'd vote this as postponed, and look at:

  • Working with PHP-FIG to help get the proposal accepted
  • Put together PRs for Symfony and Composer
  • Possibly add to the new class loaders and functionality to Class Loader Adapter, allowing us to easily switch class loaders if that becomes a thing
quicksketch’s picture

Title: Drupal and PSR-0 Class Loading » [meta] Drupal and PSR-0/PSR-4 Class Loading

Since this issue is already a disaster of failed suggestions, I wouldn't mind keeping it as a continued brainstorming issue until we have a solid route to pursue. You're right that we have things we can get started on outside of the Drupal project to broaden our options.

I'm not sure an adapter is really needed for transitional purposes, we actually don't have that much code that really contains calls to the class loader. From the patches above, I'd estimate there's only a 10-20KB patch to update all our calls to the class loader. The larger part is deciding what that class loader should actually be. I don't think our class loader is actually swappable at all at this point though. Making that a possibility would be a good undertaking.

quicksketch’s picture

Sigh.

PSR-4 has had it's voting reset because of a procedural issue (the example code in the proposal was changed after voting had begun). The proposal is going to be revised and put back up for voting, probably in a few weeks time, then we'll wait again for the two-week voting period. Although it's extremely likely that PSR-4 (or 5) will exist in the months to come, we're probably looking at a month or two before it becomes a standard. Then it will be implemented in some libraries in the following months.

From the discussion, I don't think the conceptual ideas within standard will be changed. The only changes will be to the language and example documentation contained within the standard.

The situation makes it more tempting to write an autoloader that implements the will-be standard, as well as supporting PSR-0 for our current external libraries. When a reusable autoloader becomes available (either from Composer or from Symfony, or something else entirely), we switch to it at a later date. The amount of code that truly interacts with the autoloader is very small: a dozen lines in the Drupal Kernel.php. We should consider implementing PSR-4/5 on the proposed standard, then move to a 3rd-party autoloader (possibly) after it is finalized. I think such an approach would have the dual-benefits of allowing us to make an optimized autoloader that utilizes Drupal's caching mechanisms and shortens our directory structure by 2 directories per module sooner rather than later. We already have a project that provides a similar-sounding concept, @donquixote's X Autoload project. A trimmed-down version that only supports PSR-0 and PSR-4 could provide us with a more efficient autoloader in less time, if we're willing to trust PSR-4 will eventually be approved with its basic concept in-tact.

Crell’s picture

*puts on FIG representative hat*

One thing we've discussed recently on the FIG list is the utility of having "trial implementations" of a spec before it's finalized, inspired by IETF's requirement of 2 independent implementations before an RFC can be called complete. Drupal writing a PSR-4-pre-release autoloader would qualify for that. From that perspective, #84 makes sense. (It's probably good PR in the interop world for Drupal, too.)

I do anticipate the "package autoloader" proposal getting approved sooner or later in a form very close to what it is now, give or take nitpicking (which, for a PSR, is important).

*puts on architect hat*

The autoloader MUST be stand-alone. Under no circumstances should we write an autoloader that relies on the cache system. We had one in Drupal 7 that relied on the database, and that caused no end of problems. APC cache is about the only cache we should even consider touching. Otherwise, it should be either compiled PHP (a la composer) or just runtime logic. Anything else is begging for circular dependency caching hell. Let us not create a need for drush rr in Drupal 8.

*puts on pragmatist hat*

I'm still skeptical of the benefits of making this change at this date, given the massive impact on existing code and in-progress patches it would have.

pounard’s picture

Agree with Crell about at least 3 of his statements:
- #84 is indeed a good idea if we want to switch to may-be future PSR-4
- Using any internal caching mecanism in an autoloader is a *very very bad idea*, circular dependency hell on class hierarchy but also the fact that the autoloader is the most low level stuff in the userspace runtime: it cannot rely on anything else than vanilla PHP and optionally low-level extensions such as APC
- It's now very late to change all class path in the full Drupal core, and would be a serious mess, not impossible but still a serious mess
- And I may add that early adopting a may-be future standard is also dangerous, and would make whole core obsolete if the standard is abandoned, not fully adopted, or changed in between our adoption and its release

quicksketch’s picture

- #84 is indeed a good idea if we want to switch to may-be future PSR-4

I said as much in the PHP-FIG discussion, but PSR-4 truly is a replacement (not merely an alternative) for PSR-0. Although it's spec is slightly different, I don't expect anyone will be using PSR-0 a year from now other than those who have locked-in to it for a release cycle. We definitely want to be on this train, not because it's expected to be a widely-used standard, but because removing 112 directories named "Drupal" and 250 total unnecessary nested directories is a big win in DX.

- It's now very late to change all class path in the full Drupal core, and would be a serious mess, not impossible but still a serious mess

For clarity, I have to add that we're not changing the class namespaces at all. We're only changing the directory structure by removing two unnecessary directories. Rerolling a patch means applying the patch with a different base, or a simple find/replace to remove "Drupal/[module_name]" in the patch file.

- It's now very late to change all class path in the full Drupal core, and would be a serious mess, not impossible but still a serious mess

It's better late than forcing tens of thousands of Drupal developers to endure this structure. Rerolling patches comparatively is a short-term problem. In case you haven't noticed, we're already right in the middle of renaming all plugins to remove a nested directory. This effort just started 2 weeks ago: #1987298: Shorten directory structure and PSR-0 namespacing for plugins.

- And I may add that early adopting a may-be future standard is also dangerous, and would make whole core obsolete if the standard is abandoned, not fully adopted, or changed in between our adoption and its release

It's extremely likely to be adopted because it makes sense. We're not the only project to believe that the proliferation of empty directories is dumb. Before voting was rest, it was 8-to-1 approved. It's only been reset because of procedural problems.

quicksketch’s picture

The autoloader MUST be stand-alone. Under no circumstances should we write an autoloader that relies on the cache system. We had one in Drupal 7 that relied on the database, and that caused no end of problems

Which autoloader is this? And what problems did it cause?

The cache system is one of the only systems we specify directly in settings.php so that it can be available extremely early in the bootstrap. Assuming the minimal core paths were a static map, how would depending on Drupal's caching system be a problem? I'd *really* like to avoid the is_file() check on *every* class on *every* page load. Both Symfony's and Composer's autoloaders have this check. While APC alleviates the problem, I haven't figured out why Drupal's caching mechanisms couldn't be used to the same purpose to ensure caching coverage.

pounard’s picture

There are many ways to avoid file_exists() calls, two of them are using an extension such as APC or using a generated classmap. In both case, this can be safely done on any production site as soon as the code doesn't move anymore: file_exists() are not required on those environemtns once we're sure that our code is stable (a file won't disapear out of nowhere). For example using return (bool)include_once $file; statements also avoid file stats when using an opcode cache (anyone of them): it will trigger PHP notices on fail but is safe to use on such map based autoloaders (APC based can be considered as a map based autoloader IMO). A generic failsafe autoloader using the file_exists() calls are good for development sites, on a production enviroment it *must* be replaced by a map based or a fatest one. Using a plain PHP generated classmap will end up in the opcode cache (and be equivalent to a direct APC implementation, but applyable to other opcode caches too) will be *a lot* faster than using a map stored in a remote cache bin. I really don't think that remote bin cache is necessary for autoloaders, and in the contrary, I think it's counter-productive and is probably an anti-optimization, I surely don't want to add remote storage queries where we can avoid them very easily.

quicksketch’s picture

A generic failsafe autoloader using the file_exists() calls are good for development sites, on a production enviroment it *must* be replaced by a map based or a fatest one. Using a plain PHP generated classmap will end up in the opcode cache will be *a lot* faster than using a map stored in a remote cache bin.

I definitely agree that if APC is available, we should be using it for the classmap caching. Unfortunately right now, you must *explicitly* set the class loader to use APC in settings.php. 99/100 websites would not do that if D8 was released today. Although APC is common on production sites, it's not universal. If we can make non-APC sites faster by using some other caching mechanism, that'd still help a whole lot of developers. It's not uncommon for pages to take 1-2 seconds even on my localhost with stock D8 after caches are primed. D8 is hella slow and finding ways of speeding it up in all situations is important. Though I don't fully understand the caveats @Crell referred to, I'm just saying that *some* kind of caching or way to avoid the file_exists() call on every class on every page load must be implemented. If we have access to suitable mechanisms in Drupal that a generic autoloader does not, we should utilize them.

However I don't quite know that we have "suitable mechanisms" for caching, which is why I'm curious to know what problems using Drupal's caching system would cause for us.

pounard’s picture

I think that the easiest fallback would be to generate a classloader map, many frameworks allows this, we probably should too. Proposing two different alternatives (APC and class map generating) is already covering almost every use cases, except the smallest hosting servers. Even thought, we could potentially have an administration screen allowing the user to manually trigger a class map generation, doesn't sound impossible to me, we already generate the container so why wouldn't we be able to generate this? We could reduce the file_exists() calls to 1 only (trying to load the map) in best case scenarios?

[EDIT: Removed off topic and useless stuff]

quicksketch’s picture

we could potentially have an administration screen allowing the user to manually trigger a class map generation, doesn't sound impossible to me, we already generate the container so why wouldn't we be able to generate this?

There are some pretty serious problems regenerating the container actually. There's an issue here for making a dedicated rebuild.php file just to regenerate the container, because the UI for the performance page fails when classes go missing: #1872522: Compiled data in PHP storage is cleared too late in drupal_flush_all_caches()

That could also be used to generate the classmap I suppose, but I hate giving users yet another reason why they have to clear their caches.

pounard’s picture

Ideally this would be an option only for production sites, I hope that nobody will ever remove or add code on a production running site: no cache clear ever would be necessary for this to work.

Crell’s picture

Any code that is in any way shape or form touched by or used by the autoloader MUST be manually included.

Cache implementations may be provided by contrib modules.

How do you find those classes? Oh, right, hard code more lines of code into settings.php. Lame. And what if those classes depend on other classes? Now you have to include those, too. What if one of them is database backed? That's a lot of classes to manually include.

The slippery slope there is very slippery. Screw it.

A typical composer-based project just hard-includes the autoload file on the first line of index.php, and then the problem is solved. The ONLY reason we can't do that is that we can't require regeneration of that file when someone enables a module. The burden of proof for ANY additional complexity on that is on the person proposing it, because everything we require above a single one-line require statement is a self-inflicted wound.

Easier solution: Make it easier to end up using the APC-based loader. Like, say, a dev/prod toggle, like we've needed for years: #1537198: Add a Production/Development Toggle To Core

quicksketch’s picture

Easier solution: Make it easier to end up using the APC-based loader. Like, say, a dev/prod toggle, like we've needed for years: #1537198: Add a Production/Development Toggle To Core

I posted a comment to #1537198: Add a Production/Development Toggle To Core, basically stating there's no need for a toggle for APC caching. If you have APC, you should use it. Maybe a toggle for *disabling* it might be useful, but if APC is available, you'd want to use it by default and not trouble an end-user to make that decision.

However regarding APC in general, I'm not sure we can give up and say "too bad" to those without it. Not a single one of the hosting providers featured at the top of http://drupal.org/hosting enables APC for shared hosting accounts (though BlueHost does allow you to turn it on manually). I'm sure I'm not alone in using shared hosting for plenty of Drupal sites, including my own (I've been using an A2Hosting account for years). Deciding that we don't want to utilize our own caching mechanisms because manually including classes is "lame" is not a very good reason. Assuming we used a static class map for core (as suggested in #1818628: Use Composer's optimized ClassLoader for Core/Component classes), you'd only need to manually include classes in settings.php if you were using an alternative caching mechanism (AND you didn't have APC). What I'm envisioning here would look sort of like this:

Bootstrap -> Load static class map (core) -> Autoload classes and cache lazily using 1) APC if available 2) Drupal caching mechanism otherwise.

Or we could simply not do caching until the caching the system is autoloaded. So basically the caching system is autoloaded without a cache, but after it becomes available, we use the cache for the remaining 1000+ classes needed to serve the request.

Crell’s picture

APC caching by default means anyone writing code on a system with APC needs to restart Apache every time they rename a class. No-go. We went over that when we first added ClassLoader to the system.

Better idea: Don't mess with caching until we have something in place and can benchmark to see if it matters. (Classmap for core I'm +1 on, but for contrib... wait and see. That's a post-July 1 question.)

quicksketch’s picture

APC caching by default means anyone writing code on a system with APC needs to restart Apache every time they rename a class. No-go. We went over that when we first added ClassLoader to the system.

Don't give me this "we decided it already" crap. Drupal is hella slow. We didn't officially decide to make it slow by default.

Don't mess with caching until we have something in place and can benchmark to see if it matters.

That's fine, let's just not use the lack of benchmarks as a shield. There is no way that thousands of is_file() checks per page is going to be fast. Obviously we'd need to prove that adding a caching system (especially an external caching system like memcache or the database) is faster, but we can't eliminate the possibility out of hand because you don't like the suggestion.

quicksketch’s picture

APC caching by default means anyone writing code on a system with APC needs to restart Apache every time they rename a class.

Also, avoiding this problem entirely is trivial. You cache all the known locations of files. When you encounter a new one, you add that location lazily to the cache. No cache clear necessary at all. And of course we'd make it so that clearing all the caches would also clear the APC cache. That definitely wouldn't require a restart to Apache.

quicksketch’s picture

Okay, the situation isn't as bad as I thought, but here are some numbers on calls to file_exists() by Symfony's ClassLoader.php:

                                   | file_exists() hits | file_exists() misses 
Typical page load (a normal node)  | 385                | 14
List of content (a view)           | 312                | 10
Cache clear                        | 430                | 55

Using a simple script to test calls to file_exists() benchmarks the importance of file_exists():

$time_pre = microtime(TRUE);
for ($n = 0; $n < 100000; $n++) {
  is_file($n);
}
$time_post = microtime(TRUE);
print $time_post - $time_pre;

For files that do not exist, 100,000 calls takes an average of 0.55 seconds. For files that do exist, 100,000 calls takes an average of 0.35 seconds after the first call. The first call itself if the files exist take the longest of all, 2.5 seconds. Calling 100,000 calls on the same file is essentially free, taking 0.007 seconds. Running this script with APC turned on or off has no significant impact.

So basically, file_exists() has a small amount overhead, and only slightly more if the files are not found, but at the scale we're currently seeing, it's not enough to worry about. I had expected thousands of calls and a lot more missing files. In total, we're only spending 0.00035 seconds per page load in the Symfony autoloader's findFile() method. So it's not slow in the least bit.

In this situation, caching (especially to the database or even memcache) wouldn't help anything. I'll have to look elsewhere for performance gains in D8. Sorry for loosing my cool.

So performance benefits from our own autoloader are likely to be minimal to non-existent. Adopting the proposed PSR would need to be done on the merits of DX alone, being able to remove 250 nested directories. This is definitely still a worthy cause, just not one which will affect performance.

Crell’s picture

Agreed that this issue should be resolved on DX/process grounds, not on performance. Performance should be a wash.

effulgentsia’s picture

I'm still skeptical of the benefits of making this change at this date, given the massive impact on existing code and in-progress patches it would have.

Correct me if I'm wrong, but I don't see anything in the PSR-4 proposal that prevents you from having an autoloader that mounts both a PSR-4 namespace prefix and a PSR-0 namespace prefix to the same directory. In fact, if I'm reading RobLoach's Composer issue correctly, that approach would allow for exactly that if Composer chooses to implement it that way. So, when we're ready to put this into core, can we start by registering Drupal\$module to /path/to/$module/lib as both PSR-0 and PSR-4 mount points? That would allow each module (for both core and contrib) to decide whether to put Drupal\$module\SomeClass into lib/Drupal/$module/SomeClass.php or lib/SomeClass.php, and we can time the switch for each core module appropriately, to minimize disruption of existing patches in the queue. At some point before release, we'll probably want to drop the double registration, and only autoregister to PSR-4.

quicksketch’s picture

So, when we're ready to put this into core, can we start by registering Drupal\$module to /path/to/$module/lib as both PSR-0 and PSR-4 mount points?

When we put this in, we can switch everything to PSR-4 without moving directories at all (keeping compatibility with PSR-0). PSR-4 is essentially the same thing as PSR-0 but with more flexibility for namespace prefixes. You can easily register a PSR-0 path within PSR-4:

PSR-0:

$loader->addPrefix('Drupal\\system', '/core/modules/system/lib');

Same thing in PSR-4:

$loader->addNamespacePrefix('Drupal\\system', '/core/modules/system/lib/Drupal/system');

The only thing is that PSR-4 allows you to drop the redundancy and map "Drupal\system" to an arbitrary path instead, without the duplicated "Drupal/system":

$loader->addNamespacePrefix('Drupal\\system', '/core/modules/system/lib');

But since the entire point is to shorten the directory structure, we'd probably end up moving everything all at once for modules and core's lib directory. We'd probably stick with using a PSR-0 compatible path (but loaded via PSR-4 like the second example) for our external libraries until Composer is updated.

effulgentsia’s picture

You can easily register a PSR-0 path within PSR-4: $loader->addNamespacePrefix('Drupal\\system', '/core/modules/system/lib/Drupal/system');

D'oh. Of course. And, since the PSR-4 proposal explicitly states that a namespace prefix may be mapped to more than one directory, doing that in addition to the shortened directory will therefore definitely be possible. However, we might not even need that, because...

since the entire point is to shorten the directory structure, we'd probably end up moving everything all at once for modules and core's lib directory.

Depends when we do it. If we do it before code freeze (July 1), then per #56.5, I wouldn't want to force hundreds of patches to need a reroll close to that kind of deadline, so perhaps the code that does the registration could temporarily have a list of modules for which it registers the longer path, and then all other modules get the shorter path registered. That bypasses the need to register a namespace prefix to two paths. It also means we get contrib immediately on to the shorter paths (and ideally, we would do that as soon as possible, and before code freeze). But, we could move core modules over at a time that's more convenient.

quicksketch’s picture

Yeah I suppose we could split the renaming, though I don't think this will have as significant an impact as it might seem. We've already renamed everything once (core directory move) and we're in the middle of doing it again for all plugins. Patches need rerolling all the time, this change is more trivial than most, it just affects more patches.

Anyway, right now we have all namespaces for all modules is registered all at once:

  /**
   * Gets the namespaces of each enabled module.
   */
  protected function getModuleNamespaces($moduleFileNames) {
    $namespaces = array();
    foreach ($moduleFileNames as $module => $filename) {
      $namespaces["Drupal\\$module"] = DRUPAL_ROOT . '/' . dirname($filename) . '/lib';
    }
    return $namespaces;
  }

Which of course could be adjusted, but it'd be my preference as a core developer to eliminate these directories sooner rather than later. There's little guarantee that we'll actually be breaking fewer patches after July 1 than we would if we applied this next week.

Crell’s picture

One clarification, PSR-X/4 is not simply PSR-0 with variable prefixes. It also changes the handling of _ in class names. (In PSR-0, _ maps to a /. In PSR-0, it does not.) We don't have any class names with _ in them anymore, I'm fairly certain, but I wanted to make sure no one got the wrong idea.

Now, if we were using composer.json files for modules, too, then we could let modules use whatever autoloading standard they wanted... But I think that's been punted to Drupal 9, sadly.

chx’s picture

So where's this issue now?

theunraveler’s picture

(In PSR-0, _ maps to a /. In PSR-0, it does not.)

I believe this should read "(In PSR-0, _ maps to a /. In PSR-X/4, it does not.)". Just trying to keep an already confusing issue clear.

Fidelix’s picture

Priority: Normal » Major

If we're aiming this for D8, this needs more visibility.

quicksketch’s picture

Yeah the PHP-FIG group is moving the speed of molasses on the proposal. After the vote was reset, it devolved into a argument around terminology, grammar, and examples.

In this issue I'm (again) tempted to write our own autoloader that implements the new standard. We already know it's going to pass (eventually) and what the content of the standard is. The discussion right now (and for the last month) has been over non-technical aspects of the standard.

donquixote’s picture

#109
I am all for that.
(but there might be others who say it's too late now)

I have been somewhat involved in the discussion on PSR-X. I think the current wording is shaky, but I rather see it pass than having an unproductive discussion.

There was one aspect that was technical, and that was pushed aside as out of scope.
So, it will be possible to implement slightly different flavours of PSR-X (or PSR-4): One that always assumes the loaded file contains the expected class. And another that does not expect that. In the latter case, you need include_once instead of include, and you need to check with class_exists(*, FALSE) after the file inclusion.

The latter can become relevant if we map more than one namespace to the same directory. Whether this is possible, was considered out of scope.
Imo we can be more rigid for Drupal, and assume that our directories are clean, and each file with in lib/ or src/ or whatever contains exactly the file we expect.
There is a small theoretical ambiguity if we mix PSR-0 and PSR-4 in the lib/ folder of a module. But then it would only apply to lib/Drupal/(modulename), and something would have to request a class (with class_exists()) in \Drupal\(modulename)\Drupal\(modulename)\. Not very likely to happen..

Dries’s picture

Priority: Major » Critical

I agree that removing 2 levels of unneeded directories is a big DX improvement, especially if it's likely that this will get adopted as a PSR standard. I'm in favor of #109 and am bumping this to critical. I think this can be committed as soon as it's ready. Other patches in the queue can be rerolled for file location changes.

donquixote’s picture

#109, #111
Ok, so how does Drupal-flavoured PSR-X look like?
We have some degrees of freedom that we need to decide on.

Do we keep (module path)/lib/ as the directory, or do we choose (module path)/src/ instead?

Do we continue to support PSR-0 for a transition phase? If we do that, and we continue with lib/, then for some time we will have two standards mixed in the same folder. Not a big deal, but something we should be aware of.

quicksketch’s picture

(Most of these replies are general, not really for @donquixote, who probably already understands most of this.)

Ok, so how does Drupal-flavoured PSR-X look like?

So our implementation should match the standard of course. Although I do think this is an opportunity to build a semi-static class-map for core classes. Let's keep that as an option if it gets us a significant performance gain. But probably something we'll want to look at that after the initial patch.

Do we keep (module path)/lib/ as the directory, or do we choose (module path)/src/ instead?

We do need to keep *at least* the lib OR switch to a src directory. We can't eliminate it entirely because of namespace conflicts (i.e. the "lib/Tests" directory bumping into "tests" in the module root). I don't really care which one it is. "src" honestly makes more sense to me than "lib" but I think just leaving it lib and focusing on removing the two directories is going to be the approach that requires the least debate.

Do we continue to support PSR-0 for a transition phase?

All vendor libraries will continue to use PSR-0-compatible directory structures, loaded via PSR-X (since the two standards are compatible in that regard). I think we should switch all modules at the same time. We only have one line of code that loads all module code, so from an implementation standpoint it's going to be cleanest doing it all at once.

geerlingguy’s picture

I vote for doing it all at once as well (last point in comment above). At this point, contrib developers are used to having to make these kinds of changes from alpha-to-alpha or even week-to-week, so I really don't think requiring the (few) contrib modules that have D8 branches to make the change ASAP will be any real burden.

And putting files into lib/ instead of lib/Drupal/modulename will immediately save me 5-10 extra clicks/arrow keys per minute while working on the module anyways. The sooner the better!

donquixote’s picture

I am willing to cook something up.

Crell’s picture

Related note: There have been a number of issues trying to move to the Composer autoloader, at least for core and vendor files. We already HAVE the necessary autoload file; we just aren't using it. We've been discussing it recently in the WSCCI channel to as a way to simplify early code. If we're going to move to PSR-X-preview for modules, I would recommend just having 2 autoloaders: Composer's for /core/lib and /core/vendor code, and DrupalPsrXPreview for modules. There's nothing wrong at all with having multiple autoloaders active at once and that's probably going to be the easiest implementation. (And, I would suspect, fastest, since we can compile the composer autoloader to be a classmap. That's an existing feature of Composer.)

Also, let's make sure to follow what the PSR-X rule are likely going to be, which means NOT treating _ in class names as a directory separator. (PSR-0 does that; the new autoloader does not. I don't think this impacts any of our classes at present.)

Dries, since the odds of this happening in the next 48 hours are extraordinarily small are you calling this an exception to API freeze? Because changing the directory location for modules is definitely an API change. (I am OK with such an exception, but it should be stated explicitly.)

effulgentsia’s picture

I think Dries raising this issue to critical was a pretty explicit, "yes, this can happen after API freeze", statement. However, let's try to get it in as early in July as possible, and especially before beta1, or else he may change his mind on whether it's critical enough to keep it prioritized that way once a critical mass of contrib modules have ported.

"src" honestly makes more sense to me than "lib" but I think just leaving it lib and focusing on removing the two directories is going to be the approach that requires the least debate

How about making "lib" always mean PSR-0 style (absolute namespace) layout, and "src" always mean PSR-X style (relative namespace) layout? If there isn't fast consensus on this, happy to punt on it to a follow up or D9 depending on timing, but I think that approach would stay true to the conventional meaning of those abbreviated words.

donquixote’s picture

How about making "lib" always mean PSR-0 style (absolute namespace) layout, and "src" always mean PSR-X style (relative namespace) layout?

I am happy with this.

bojanz’s picture

I like that idea too.

geerlingguy’s picture

+1 on src for PSR-X. Conceptually easier to remember, imo.

jcisio’s picture

I like #117 too, so it's basically just add one more class loader. Follow-up could be prioritizing the usage of src/ over lib/.

donquixote’s picture

I'm having some decent success (locally) with the class loading.
However, we also need to consider plugin discovery (and possibly other scan operations).

I might upload something late Sunday or Monday (Europe time), for now I'm out.
(this should not stop others from rolling their own)

webchick’s picture

Oh, so very happy to see this marked critical. :D +1 for "src" rather than "lib," too. Both makes more sense in terms of a "plugin" developer (you're generally consuming core's/others' libraries, not declaring your own for re-use outside of Drupal), and makes it immediately obvious which directory structure is being used.

Crell’s picture

Alex, do you envision us keeping both /lib and /src for D8 release, or just using that as a transitional state? I don't think having 2 autoload locations for every module is going to work out well. It will just confuse people, and may have a performance impact.

donquixote’s picture

#124
For my taste, lib/ can be sacked before release.
But it is nice to have a transition phase, it is not difficult to do that, and the upcoming patch will do exactly that.
(only for discovery I still need to figure out, but I don't think it's going to be a problem)

quicksketch’s picture

I would recommend just having 2 autoloaders: Composer's for /core/lib and /core/vendor code, and DrupalPsrXPreview for modules.

Haha, that's essentially what #32 was, though not yet following a standard. After having hacked our system to have 2 autoloaders (one for modules and one for PSR-0), I have to say that I think it would be easier if we had one class registration system instead of 2. Our current system is not well-architected to handle multiple autoloaders. PSR-X isn't identical (as has been mentioned several times) because of underscore handling, but in our situation, we don't have any libraries that use that convention. So I'm all for PSR-X for everything, one autoloader.

Berdir’s picture

Both PHPUnit and Twig are not using namespaces but underscores. Not sure if there are more.

donquixote’s picture

I didn't want a big discussion about that before the patch, but.. whatever
The patch I am planning is based on https://github.com/donquixote/krautoload
This basically covers everything we need, with a decent performance. It allows registration of PSR-0 folders and PSR-X and PEAR folders side by side, and an APC cache layer similar to Symfony2.

Using this to replace the Symfony2 loader was pretty easy, and it works.
We might do some changes to the library until D8 release, and I will happily accept any improvements.

This can be discussed, and you are free to explore other solutions, but I am going to follow this direction and see how far I get.

quicksketch’s picture

Both PHPUnit and Twig are not using namespaces but underscores. Not sure if there are more.

Doh... didn't know that. Well too bad for one autoloader then.

EDIT: We can still have one autoloader (like krautoload), but it would need to implement both PSR-0 and PSR-X. Or use a separate autoloader for each.

catch’s picture

Cross posting #2023325: Add interface for classloader so it can be cleanly swapped. This doesn't have to be to be done at the same time but in the process of this let's not further box ourselves into using a specific autoloader implementation.

As a general note, if something is 'critical task' or necessary to resolve a critical task (or bug), then it's not subject to API freeze.

There might be some disagreement/flexibility over what counts as critical or not (especially if something stays unresolved when we're closer towards an RC), but there has to be a balance between getting to a release, and what we release actually being releasable.

donquixote’s picture

Three patches:

  1. Test whether the existing class loader covers or survives all edge cases of PSR-0.
    The answer: No it does not. But neither we nor Symfony have seen this as a problem at any time.
    So, it is not a big deal. But it is nice to know, isn't it?
  2. Adding the Krautoload library, and using it to replace the Symfony class loader for PSR-0.
  3. Register PSR-X module folders side by side of the PSR-0 module folders.
    Add tests to verify that this works.

Notes:

  • The APC cache is not fully available yet, so I skip that part. I don't see it as a problem, it can be added any time.
  • Krautoload can use class maps, so we can skip some stuff with PHPUnit.
  • Krautoload has a RegistrationHub that is separate from the loader and finder.
    This is one step towards #2023325: Add interface for classloader so it can be cleanly swapped.
  • The signature of RegistrationHub is not carved in stone. It will change when PSR-X gets its final number (e.g. PSR-4), and we can try to make it more composer-like. This can happen either upstream, or we code our own Drupal-specific RegistrationHub.
  • #1846376: Namespaces for disabled modules are registered during the first request after a module is disabled. This is something I ran into while trying to understand the module registration code in DrupalKernel.
    From what I can tell, the existing implementation cannot work, because Symfony ClassLoader does not allow removal of namespace registrations.
    At some point there is a $this->prefixes[$prefix] = array_merge($this->prefixes[$prefix], array()), which should not have any effect at all.
    I have an idea how I would fix this, but first we should decide what is the desired behavior.
    I left a @todo in the patch.
  • #1658720: Use ClassLoader instead of UniversalClassLoader. The argument here was

    UniversalClassLoader was adding an artificial distinction between namespaced classes and unnamespaced ones whereas PSR-0 does not. And Symfony 2.1 introduced a ClassLoader which has a simpler API by avoiding this distinction (just like Composer avoids it too)

    Imo, this argument is bogus.
    PSR-0 and PSR-4/-X should only ever be read as defining the external measurable behavior of a class loader.
    Whether there is an internal distinction between namespaced and unnamespaced classes, should be an implementation choice.
    In case of Krautoload, I decided to keep the internal distinction for expected performance benefits.
    However, the RegistrationHub has methods that allow registration of composer-style prefix maps. So the external behavior is designed to be equivalent with the common interpretation of PSR-0.

  • To extend this point:
    There is something weird and possibly unexpected with Symfony2 ClassLoader prefix registration:
    If you register a prefix "MyVendor\Gira", then it will trigger on anything that begins with this prefix, e.g.
    class_exists('MyVendor\Gira\Foo')
    class_exists('MyVendor\Gira_Foo')
    class_exists('MyVendor\Giraffe\Foo')
    class_exists('MyVendor\Giraffe_Foo')
    It would be possible for sure to reproduce this behavior with a Krautoload plugin. But is this even desirable?
  • Performance:
    I saw someone doing benchmarks in another issue, will check. But for now I simply want to get it to work.
donquixote’s picture

Status: Active » Needs review

as always..

donquixote’s picture

same as #131, but without the first patch.
Locally all the failed tests from #131 pass now.

Some changes applied to krautoload library. These will go upstream when things have stabilized.

donquixote’s picture

Status: Needs work » Needs review
donquixote’s picture

Benchmarks:
Autoload benchmark (sandbox) now has a Drupal 8 branch.

The module generates 300 namespaces, each with plenty of classes. It then lets different loaders load those classes.
It's complicated..

Disclaimer: I ported the benchmark tool to D8 in a rush. There might be some issues, and the code is not very pleasant atm.
The results are similar to previous findings on D7 / xautoload, so I assume I am not that far off.

300 namespaces:
Disclaimer: Symfony ClassLoader is sometimes faster, sometimes slower.

loadClass loadClass.nested loadClass.nonexisting
Symfony ClassLoader (not paranoid)
732.756
738.290
993.868
Krautoload, paranoid
287.020
336.903
85.313
Krautoload, no API, paranoid
266.617
318.037
70.527
Krautoload, not paranoid
252.890
283.633
79.297
Krautoload, no API, not paranoid
235.987
272.269
69.618
# number of operations
300 ops
300 ops
600 ops

30 namespaces:
Disclaimer: These results were different each time. Sometimes the Symfony ClassLoader was the fastest, sometimes it was the slowest.

loadClass loadClass.nested loadClass.nonexisting
Krautoload, paranoid
194.982
220.929
49.930
Symfony ClassLoader (not paranoid)
191.213
201.366
91.484
Krautoload, no API, paranoid
185.466
213.896
43.661
Krautoload, not paranoid
168.658
191.456
50.149
Krautoload, no API, not paranoid
163.862
183.929
43.004
# number of operations
450 ops
450 ops
510 ops

Notes

"paranoid" / "not paranoid".
As we have seen in #131, there are some edge cases that can crash the Symfony ClassLoader.
Krautoload has one plugin that avoids this kind of crash ("paranoid"), and another that does not ("not paranoid"). For a fair comparison, we can only compare the non-paranoid plugin with Symfony ClassLoader.

"no API":
Krautoload uses a mechanic that makes it easier to unit-test, but adds a tiny overhead.
I crafted a version without this overhead, to see how it compares.

"loadClass.nonexisting"
This benchmark case is about attempting to request classes (e.g. with file_exists()) where the respective file does not exist.
The case shows performance of the class loader without the actual file inclusion, which often tempers the benchmark results.

Conclusion

The difference between loading existing and non-existing classes shows that the include() eats a significant portion of the time.

Symfony ClassLoader does not scale nicely with too many registered namespaces. In this case the class finding eats more time than the file inclusion.

Krautoload "not paranoid" and "no API" add a bit of performance, but not that much.

If we don't register too many namespaces, both class loaders have an acceptable performance.

APC cache and class map are yet to be explored.

donquixote’s picture

Left to do:
- Code cleanup (ouch, ouch, ouch)
- Finalize registration interface
- Discovery of plugins and other stuff based on PSR-X/-4 instead of PSR-0.
- (Optional: Explore alternatives to Krautoload, if you guys don't like it.)
- Activate cache layer. (APC and/or class map)

donquixote’s picture

Re "class map" / "cache":
The ideal solution might be to create a dynamically updated class map at the cache layer.

In xautoload I even managed to skip all the namespace registration (modules, other stuff) until the first cache miss..
We can try this when the rest has stabilized..

msonnabaum’s picture

Now we're talking about lib and src again? We sure have a knack for adding complexity to something simple.

donquixote’s picture

#140
We discussed shortly on IRC.
I personally don't have a strong opinion about lib/ vs src/. I think I was quite passionate about it back then (in favour of "lib"), but now not so much anymore..
I hope an agreement can be found, and I hope this can happen on IRC instead of here.

This being said, for the time being I want to continue working with src/, until the class discovery has stabilized.
Two separate directories are easier to work with.

donquixote’s picture

A note about class discovery.
There are many places where we juggle with module namespaces and plugin namespaces. Each plugin manager gets them injected.
ViewsHandlerDiscovery extends AnnotatedClassDiscovery, and has its own namespace handling.

And aside of the plugin namespaces, we also have the annotation namespaces, which are registered in Doctrine's AnnotationRegistry (that is a final class with only static methods, used by a number of Doctrine annotation-related classes). (*)
There is a good number of modules which cook up their own annotation namespaces.

The problem is, each of these namespace lists has associated directories. And these need to change for PSR-X/-4.

It is possible to do this, but it will be messy.
This will be a typical thing to do:

    foreach ($this->rootNamespacesIterator as $namespace => $dirs) {
      $namespace = "$namespace\\Plugin\\views\\{$this->type}";
      foreach ((array) $dirs as $dir) {
        $plugin_namespaces[$namespace][] = $dir . '/Plugin/views/' . $this->type;
      }
    }

So, whenever we want to pass around a sub-namespace, we also need to determine the associated subfolder within the psr-x directories. We can no longer be passing around the root directory, because that will not exist in the future.

I think this is going to be very unpleasant to work with.
So, my plan is to refactor the discovery logic, so that directory lists for directories are encapsulated, and no longer need to be passed around. Modules can still juggle with the namespaces, but don't need to worry about directories.
I think this will be a positive change overall. But it will be a big patch.

EDIT: (*) I should mention, the Doctrine AnnotationRegistry::loadAnnotationClass() is hard-coded on PSR-0, we would need to hack/modify it to support PSR-4. Subclassing is not an option, because of too much static/final.

EDIT II: Subclassing the DocParser might be the solution.

donquixote’s picture

Summary:
- major refactoring of the Krautoload library (will go upstream when stabilized)
- added class discovery capabilities to Krautoload (will go upstream when stabilized)
- Let AnnotatedClassDiscovery use the Krautoload discovery logic.

As a result, AnnotatedClassDiscovery no longer cares about the directories on the injected plugin namespaces. It only cares about the namespaces themselves (that is, the array keys).

The discovery thingie extends the class loader, and has the same interface to register namespace plugins.
It is wrapped in a RegistrationHub for more convenient registration methods.

Left to do:
- Doctrine's AnnotationRegistry still needs PSR-0 directories on the namespace list.
- The discovery engine is not properly injected in AnnotatedClassDiscovery. This is to avoid big noise in every plugin manager that creates its own AnnotatedClassDiscovery. Imo, we should have a factory service that can create AnnotatedClassDiscovery objects, so plugin managers don't need to mess around with namespace lists.

donquixote’s picture

Not sure if a reroll is necessary. Line numbers may have changed.

RobLoach’s picture

This is a meta-discussion of different options available to us. We should probably add Krautoload in as a solution #8 in the OP and open up a new issue for it so that it's easier to track. Would also love to add an interface for it over at #2023325: Add interface for classloader so it can be cleanly swapped and Class Loader Adapter so that it's easy to switch in.

donquixote’s picture

This is a meta-discussion of different options available to us. We should probably add Krautoload in as a solution #8 in the OP and open up a new issue for it so that it's easier to track.

Seems like a good idea.

donquixote’s picture

donquixote’s picture

/src/ vs /lib/:
msonnabaum and others pointed out that there were good reasons we chose /lib/ over /src/.
See #1400748: Proposal for unified namespace organization ff.

I think most of these arguments apply to PSR-4 just as much as they applied to PSR-0.
So, for my taste, we can stick with /lib/.
(we could still have an intermediate phase where we play with /src/ vs /lib/ as proof-of-concept, but only for strictly technical reasons)

Pancho’s picture

/src/ vs /lib/:
msonnabaum and others pointed out that there were good reasons we chose /lib/ over /src/.

I'm putting together the arguments raised in #1400748: Proposal for unified namespace organization, so we know what we're referring to and don't draw more circles than necessary. Hope I caught all relevant arguments appropriately:

Pro /src/:
#161: "Also, lib vs. libraries is only an issue because we went with lib instead of src above."

Contra /src/:
#30: "I'd find core/src and [module]/src highly misleading. As if the stuff outside src was not source code too."
#49: "I'm much more reluctant to see a 'src' folder with core source code *outside* of it."
#55: "src - that is going to be strange compared to the amount of source code that'd be outside of it"

Pro /lib/:
#33: "isn't this essentially an equivalent of Libraries module we're putting in core, focused on the management of PHP libraries..?"
#55: "lib - doesn't really mean anything, but is also the least offensive and not inaccurate."

Contra /lib/:
#26: "'library' makes me think of libraries.module."
#159: "concerns about the mental model conflict between the lib directory and the contributed module Libraries API's concept of libraries."
#167: "One more thing I don't like about a /lib/ folder name is that not everything (maybe not even most things) that will go into it is a library, in any conventional sense of the term. [...] I don't want to rule out the possibility that we'll want to move, say, the vertical-tabs js and css files into a core/libraries/vertical-tabs folder."

Pro /classes/:
#47: "consider 'js', 'css', 'includes', and possibly more directories next to it -- 'src' or 'library' would be very ambiguous, since JS, CSS, and whatnot is also source code, or potentially also contains other kinds of libraries."
#51: "Naming this stuff 'classes' indicates 'you know, not everything is classes, we also have procedural'".

Contra /classes/:
#48: "So classes folder would also contain interfaces and may be traits in the future? Classes seems broken to me and refers to a really old time now where PHP did only support classes, which is not the case anymore."
#53: "'Other projects are doing it' *is* a valid argument, for the simple reason that we want to be able to absorb (read, steal) developers from those other projects."

See also http://www.garfieldtech.com/blog/php-project-structure

[edit:] linkfix

Xano’s picture

Regarding the arguments against /src/: IIRC we are working towards a situation in which all PHP code lives in autoloadable classes, which means that in the end there will be no more code outside /src/ or /lib/ or /hazcode/ or whatever option we decide on.

Pancho’s picture

Re #152:
Yes, I agree. And in the meantime we've gone quite far, with more of our code base moving to OOP every single month until release, at least regarding modules.

Ideally, most contrib modules won't have much more than the info.yml, routing.yml, plus minimal .module and .install files in their main folder.
However, a PSR-4 folder called 'lib' might suggest this was made for super-duper reusable code, so might slightly lead contrib devs into keeping more procedural code.
Whereas a PSR-4 folder called 'src' suggests that ideally all source code should live there, leading contrib devs to converting more of their code to OOP.
So finally this would be about running a bit ahead of the status quo in our naming, in order to indicate our direction.

This might be an additional argument for effulgentias proposal in #117:

How about making "lib" always mean PSR-0 style (absolute namespace) layout, and "src" always mean PSR-X style (relative namespace) layout?

So, weighing all arguments in, to me it really sounds like the best idea to stick with /core/lib for core's PSR-0-code, and to use /core/modules/src and /modules/src for all PSR-4-organized code.

[edit:] Meaning that we should simply stick with what #2033501: Explore Krautoload as a solution for PSR-4 class loading defines at least as temporary solution.

donquixote’s picture

#151
Another interesting study would be how other projects name their code folders.

Pancho’s picture

donquixote’s picture

#156 more of that :)

chx’s picture

Kraut is a German word recorded in English from 1918 onwards as a derogatory term for a German

Kraut Offensive Slang Used as a disparaging term for a person of German birth or descent.

Sigh.

Pancho’s picture

Re #158: Yeah, but it's not like anyone in Germany would feel offended by that (anymore). It's sometimes used here in a self-ironic of chuckling way... :)

tstoeckler’s picture

That's what I think of when someone talks about "Kraut". (I'm German.)
A picture of a bowl of cabbage, which is what the Germans call 'Kraut'
I do not really care for naming, so I wouldn't be strictly against a rename, but I have a very hard time imagining that you could find someone who would be offended by the name.

donquixote’s picture

The "Music" part of the same WP article quoted in #158 has some nice advice for everyone to listen to :)

To draw any further questions away from this issue, I opened a separate issue on github.

pwolanin’s picture

This is truly among the least important DX issues currently in D8 core. I propose it be postponed to 9.x

catch’s picture

Yes Dries marked it critical so I'm not going to play status pong, but it doesn't feel very important to me either. I didn't like the number of directories when they went in, don't like them much now, but there's much, much worse things to worry about at the moment.

Pancho’s picture

Not so sure. While objectively there are much more important DX issues, subjectively the deep directory structure makes developing for D8 feel like you're dealing with a very complex architecture. Finally, DX is not just about the most experienced developers but also about admins becoming developers. In the end, this would have to be determined by a usability study. But the mere fact that quite many of us deem this aspect important, means we should care.

It seems very favorable to early adopt the forthcoming PSR-4 standard. If Krautoload turns out to be a good solution for us, this could be easily solved. So keeping this one critical doesn't really do much harm, either. It doesn't look like it would unduly draw much more time and effort from possibly more important issues.

chx’s picture

This whole discussion only happens because we are so very theoretical.

The document root for Drupal\node should be modules/node and poof! most directories are gone. No lib. No src. Nothing. Just put that interface beside node.module, mkay? The violation of PSR-0 would be absolutely minimal, we just skipped the top "Drupal" prefix but given that we are inside Drupal modules, themes, profiles etc that's not a biggie. Easy to do. Easy to understand. Easy to find. Subnames could follow the PSR-0 pattern, little choice now. This proposal deletes three levels of directories as they are completely unncessary.

The implementation is also quite easy to do, just add an autoloader for these that skips one level. Heck, I could probably write an autoloader that implements both PSR-0 and this Drupal-standard with absolutely minimal overhead.

Edit.: there's a lot of mentions of PSR-4 / PSR-X but it's not yet a standard and I can not care less about FIG. All that talk about getting off the island shouldn't mean this issue needs hundreds of followups -- this is our own code, do what is easiest with it, end.

fgm’s picture

A recurring theme in the whole PSR-0/PSR-X (4) discussions on FIG has precisely been that these, and especially PSR-0, have never been meant to be used for "internal" loading, the expectation being that bases projects like Drupal are able to make use of alternate mechanisms of their own for their internal code, and there is no very good reason to force PSR-0 loading for it. IIRC Drupal was the first major project to use PSR-0 even for its own code, but other have followed suit since, and this behaviour is actually a big part of the push for PSR-X.

We could just as well stop using it for core/*.

rudiedirkx’s picture

I vote for #165:

This proposal deletes three levels of directories as they are completely unncessary.

That seems like a good deal.

Apparently this is an issue, because people are still discussing it.

Back in the day, if I'd known this, I'd have voted for top level namespaces for every module: node\entity\Controller (if that exists) would refer to core/modules/node/entity/Controller.php. I guess it's too late for that.

donquixote’s picture

I think we should update the issue summary, since we (Dries in #111) pretty much decided to use the upcoming PSR-4, and we are only talking about the details. (e.g. src vs lib)

chx’s picture

Re #168 or nothing. There's no point in having src, lib or whatnot.

Crell’s picture

chx: We went over that well over a year ago. We don't want to mix namespaced classes, procedural code, and CSS/JS all in together. That would be *more* confusing. I don't give a crap between src and lib, but we should have something there. If one single directory is such a problem... well, you're not going to like PHP as a language anymore. Sorry.

donquixote’s picture

what Larry says..

#169
The "nothing" option was discussed to death when we first talked about PSR-0.
The motivation to have a src or lib folder is that modules can contain plenty of other stuff: css, js, (procedural) includes, tests, 3rd party code, etc.
The arguments in favor of lib/ instead of nothing have not changed since then. So I think we should stick with this decision.

quicksketch in #113:

We do need to keep *at least* the lib OR switch to a src directory. We can't eliminate it entirely because of namespace conflicts (i.e. the "lib/Tests" directory bumping into "tests" in the module root). I don't really care which one it is. "src" honestly makes more sense to me than "lib" but I think just leaving it lib and focusing on removing the two directories is going to be the approach that requires the least debate.

-------

One could argue that the arguments in favor of lib/ vs src/ have not changed since then either.
See #113, #140, #150, etc.

However, there are some arguments that can be considered as "new" since then:

  1. other (new) projects in the PHP world that have chosen either src/ or lib/ (or nothing) since then.
  2. being able to distinguish between PSR-4 directories and PSR-0 directories.
    • PSR-0 will be eliminated in modules, but it will still be around in core, so it might be useful being able to distinguish.
      See effulgentsia in #117.
    • Reduce confusion in D7 contrib, which already has some modules using PSR-0, and will get other modules using PSR-4.
    • Reduce confusion during the transition from PSR-0 to PSR-4.
  3. The idea that future Drupal will have most stuff in classes, with almost no procedural code remaining.
    (We are far away from that in D8)

My current perception is that those who defend lib/ are quite passionate about it, whereas those who tend to vote for src/ don't really care that much.

donquixote’s picture

About different options for the PSR-4 loading.
(e.g. for Crell)
I added this in the issue summary over at the Krautoload issue, but this might be the better place.

I am a bit opinionated about this, so maybe some of it needs to be tuned to be more neutral.
However, the main arguments remain.

Alternatives

It was suggested to use a hand-crafted PSR-4 loader, and do all the rest with Composer's class loader.
The composer loader would have a compiled class map for core classes.

However,
- Krautoload can handle compiled class maps just as good as Composer, and it plays nicely with the composer vendor directory.
- Krautoload can have a cache layer on top of everything, instead of "just" for modules.
- By having two class loaders on the stack, we totally eat up any gain from micro-optimization. Having two loaders means that for one half (or whatever percent) of classes we fire a loader that does nothing.

E.g. if the composer loader is first in the chain, only for core and vendor, and autoload fires for a module-provided class, then the following will happen:
- Composer will look in the class map and find nothing.
- Composer will look into registered prefixes, which is exactly what we want to avoid. And again, finds nothing.
- Krautoload will find the class (e.g. in APC cache).

On the other hand, if Krautoload is first, and autoload fires with a core- or vendor-provided class, then
- Krautoload will check the APC cache, and find nothing.
- Krautoload will check the registered prefixes, which is exactly what we want to avoid. And again, finds nothing.
- Composer will do the class loading (e.g. via class map)

Besides, Composer does not help us with class discovery.

If this alternative is still being considered, it should be explored in a separate issue!

Pancho’s picture

C'mon @chx, it's not like one side wins a battle against the other side.
Some want to stay at PSR-0, others wanted to break out of both of them, and now there is the forthcoming PSR-4.
Good arguments for all positions have been brought up. So we need a good compromise, and IMHO PSR-4 exactly looks like that: Looking forward to become a standard, so yes, it's a compromise by definition. Yet it is flexible enough for our special needs that you correctly described. And being an early-adopter of a forthcoming and most probably successful standard is nice, too.

MrHaroldA’s picture

If PSR-0 is forced upon all Drupal contrib devs for all contributed modules, my guess is that there the adaption of D8 will be very, very slow. I will be one of those who will probably keeps using D7 because I just don't see the point why Drupal-specific code should comply with a non-Drupal standard. It's not that Drupal modules are usable on other frameworks or something like that.

"Sad Harold"

Xano’s picture

@MrHaroldA: if you want to know why using a standard for this is good, please find the change record that describes this change. This issue is about improving what we already have.

xtfer’s picture

For reference, I assume we are thus considering adoption of what is currently known as PSR-X, not PSR-4:

https://github.com/php-fig/fig-standards/blob/master/proposed/autoloader.md

Is it wise to attempt to adopt a standard that hasn't actually been approved?

Can the two live side-by-side? If so, use semver, and add PSR-4, WHEN IT IS PSR-4, not when its just a proposal, since if we miss the implementation slightly, because it changes or whathaveyou, Drupal is suddenly doing its own thing again, like an independent kid on Sesame Street.

quicksketch’s picture

For reference, I assume we are thus considering adoption of what is currently known as PSR-X, not PSR-4:

All new PSRs can generally be called "PSR-X" until they are finalized. Voting *had* already begun at one point, ensuring that the new standard would have been PSR-4 (assuming it passed, which it was 9-to-1 until the vote was cancelled). So we were calling it PSR-4 with some confidence. The new standard *will* pass. It's just a matter of time before the PHP-FIG actually takes a vote again after the bickering stops.

Can the two live side-by-side? If so, use semver, and add PSR-4, WHEN IT IS PSR-4

semver has nothing to do with autoloading. Perhaps you mean PSR-0? The current plan moving forward is using #2033501: Explore Krautoload as a solution for PSR-4 class loading, which supports both PSR-0 and PSR-X, so yes they can live side-by-side and that's the plan currently.

donquixote’s picture

so yes they can live side-by-side and that's the plan currently.

That's the plan for a "transition phase", but I think most of us agree that we will remove PSR-0 for modules, once the dust has settled. Having both around for too long will be confusing.

msonnabaum’s picture

Honestly, if we're not going to have a 1:1 mapping, I'd prefer what chx proposes. What's the point in bothering with a "standard" if there are two?

jcisio’s picture

#180 +1, with OOP files camel cased, it's easy to distinguish from other files.

donquixote’s picture

@msonnabaum (#180), jcisio (#181):
This has nothing to do with the standard.
Putting everything in the module root would still comply with PSR-4/-X (*), but it would be unpractical for us. And I should say, impossible.

We just have too much stuff floating around in modules other than class files. Yes, class files will be distinguishable by their CamelCase, but they will show mixed up with the rest in an alphabetic directory listing.

And now the "impossible":
(probably exactly what Crell has in mind in #170 quicksketch has in mind in #113)
Having both "$module_dir/tests" (for tests-related stuff) and "$module_dir/Tests" (for classes in the Tests namespace) is not just wonky, but it will clash on Windows.

Besides, every *.php file in the PSR-4 folders which does not belong there may (in theory) result in false positives in class loading.

As for the DX penalty: I think part of the current problem is not just the depth of the directories, but the repetition of the module name, which feels confusing. A directory level of "lib/" or "src/" is a lesser problem, because it will be unique in the directory trail.

I am going to update the issue summary, this discussion needs to be more focused.

(*) The current proposal for PSR-4 does not mention whether the directories can contain anything other than the expected class files. I think to remember that this (among other things) was considered out of scope when I brought it up.

jcisio’s picture

About the false positive, if there is one loader, I guess it is not a problem. Good point on "Tests". So it's impossible then.

Pancho’s picture

I'd say the other way around: "Tests" vs. "tests" problem isn't the big problem. We wouldn't necessarily need a "tests" folder for tests-related stuff.

But class discovery would be a real problem, even with a single loader, and even if the loader is smart enough to avoid false-positives.
As a developer, I don't want an incredibly smart autoloader - I want an easily predictable autoloader!
At every point it needs to be absolutely clear which php files would be autodiscovered and -loaded, and which ones won't.
For a number of reasons I might want to place a .php file into the module root, and I definitely don't expect it to be autodiscovered.
For a number of reasons I might want to place a whole subfolder with .php files or whatever into the module root module, and I definitely don't want any of that to be autoloaded.
So it's just for DX reasons that we do want a '/lib' or '/src' folder, making sure it easily predictable what will be autodiscovered (the contents of that folder) and what will be left alone (whatever else is in the module root).

donquixote’s picture

Everyone, it is review time!
#2033501-32: Explore Krautoload as a solution for PSR-4 class loading

And +1 what Pancho says about DX.

donquixote’s picture

Krautoload, it has been nice knowing you!
Things have changed, and I have changed my mind (if that matters), so here is an update:

  • Composer has an open PR that will add PSR-4 class loading.
  • Drupal class loading is switching to Composer's autoload.php, which very soon will have a class loader with PSR-4 support.
  • Krautoload has performance benefits over Composer *if used in Drupal*, BUT it is by far not the best we can get, performance-wise (aside of class maps and APC, which should work equally well in any loader).
    Instead, we should stick with Composer for now, and discuss possible performance improvements in follow-ups.
  • The Krautoload patch did also introduce some class loading discovery stuff.
    However, the patch did introduce one object that would act as both a class loader and a discovery thingie. Which did look attractive in the first place, but it introduces nasty hard coupling of systems that should rather be independent.
    Instead, focus on #2033611: Port AnnotatedClassDiscovery to PSR-4, and make it agnostic of directory information., and attempt a Drupal-native solution.

So the new plan would be:

  1. Use a Composer class loader that supports PSR-4. (on the way)
  2. Register all module namespaces to PSR-0 *and* PSR-4 alongside.
  3. Support PSR-0 and PSR-4 in AnnotatedClassDiscovery, using an abstraction such as SearchableNamespaces.
  4. Support PSR-0 and PSR-4 in simpletest class discovery
  5. Make a final decision on src vs lib.
  6. Move all module class files.
  7. Unsupport PSR-0 for modules.

Sounds reasonable?

quicksketch’s picture

@donquixote: Well since you're the one moving things forward, I think if you've decided against Krautoload then that's probably the direction we'll head also. I'm encouraged by the positive response you've been getting in the Composer Github queue, it seems like they're very willing to incorporate the changes and that could probably happen before PSR-4 is even passed.

As above, my only concern with this approach is the timeframe. Anything we can manage now would be great.

donquixote’s picture

The real important issue is AnnotatedClassDiscovery, I really need some more feedback over there.
This affects the plugin system, I can't decide this stuff by myself.
Even if we move away from Krautoload, some of the ideas proposed there may still make sense. I just don't think anymore that they need to necessarily live in a separate 3rd party package.

RobLoach’s picture

As an added benefit from #2038135: Use the Composer autoloader to make everything simpler, it's possible to wrap the ClassLoader with a decorator class. This means that one could use a contributed module to add PSR-4 awesomeness to the class loader if so desired:

Have the module set the class_loader variable:

  variable_set('class_loader', 'MyLoader');

MyLoader.php

class MyLoader extends ClassLoader {
  public function findFile() {
    // Do PSR-4 things or parent::findFile() otherwise.
  }
}

Of course, implementation details would be quite different from what I showed here, but I hope you get my point. This could happen whether or not the Composer ClassLoader gets PSR-4, the timeline does not matter since it allows that kind of flexibility.

DamienMcKenna’s picture

Just to mention it, the *vast* majority of OSX users, i.e. those who have not reformatted their system drives to be case-sensitive (and who didn't want to run a large amount of software compatibility problems), would also run afoul of module_dir/Tests vs module_dir/tests as the default filing system (HFS+ / Mac OS Extended) is case-insensitive by default. So it wouldn't just be Windows users who'd potentially run into problems.

quicksketch’s picture

This could happen whether or not the Composer ClassLoader gets PSR-4, the timeline does not matter since it allows that kind of flexibility.

The timeline matters if all modules are going to be switched to using PSR-4 before D8 is released, and all contrib modules will use PSR-4 too. No body is going to be happy if a separate contrib module is required to use PSR-4. People will just suck it up and deal with PSR-0 for module name spaces, which is what we're trying to avoid.

donquixote’s picture

And again, we have stuff to review.

#2033611-26: Port AnnotatedClassDiscovery to PSR-4, and make it agnostic of directory information.
gets us very close to porting AnnotatedClassDiscovery.
Module namespaces are passed along in PSR-4 style, without actually being moved to a new directory.

#2058867-9: Consider introducing a native ClassLoader for performance and PSR-4
is an option if we don't want to wait for Composer to introduce PSR-4.
It attempts to be very similar to Composer, so we don't have to worry that much about being different from the rest of the world.

Both issues contain intermediate patches and interdiffs, so we can discuss which of the proposed changes we want and which we don't.

webchick’s picture

I'm very sad this issue hasn't moved in close to a month. :( The sooner it makes it in, the faster we can start updating our module upgrading docs and probably turn down the dial of rage from D7 developers towards D8 from an 11 to more like an 8. :P~

The PSR-4 spec was just re-proposed to the PHP-FIG mailing list and is currently at RFC for 2 weeks.

https://github.com/composer/composer/pull/2121 hasn't gone anywhere in 25 days, though it may pick up again if PSR-4 is adopted, I suppose. Not sure that we should wait on Composer; we might get to a PSR-4 solution faster by introducing our own custom autoloader and then swapping it out for Composer once it's updated upstream.

Given we do not have consensus around src/ vs. lib/, let's just leave it at lib and be done.

#193 is a little confusing to me and doesn't quite jibe with the issue summary. What's the simplest thing that could possibly work to get PSR-4 support in Drupal 8 so that we can start updating docs/presentations/change notices/etc.?

donquixote’s picture

Issue tags: -Performance, -DX (Developer Experience), -naming convention, -PSR-0, -sad chx, -PSR-4, -classloader

@webchick,

I'm very sad this issue hasn't moved in close to a month.

me too!

#193 is a little confusing to me and doesn't quite jibe with the issue summary.

What is the problem with #193?
Could you chat me up in IRC to resolve the confusion?

Regarding the issue summary - this can only ever represent the state at the time that the issue summary was last updated ..
I can try to improve that, but maybe we should have a chat first?

What's the simplest thing that could possibly work to get PSR-4 support in Drupal 8

No matter which technical solution we choose, the tasks are the same:
- Class loading
- Class discovery for plugins and test classes.

None of these can be "simplified away".

I am willing to work more on this, if I get some feedback and direction.

Crell’s picture

The path of least effort, IMO, is:

1) Commit #2038135: Use the Composer autoloader to make everything simpler
2) Wait for PSR-4 to be officially approved.
3) Wait for Composer to add PSR-4 support, for which there's already a PR and I doubt it will take very long after step 2.
4) Make a branch that just moves modules up a level and adjusts the wiring accordingly. This would be 99% moving files around without changes so doing it via patch makes no sense. This is a textbook case for doing a proper merge.

The caveat is that I cannot guarantee the turnaround time for step 2. That it is going to happen is, at this point, a done-deal. As an option, we could ask Jordi if he'd be willing to push PSR-4 support into Composer before it's officially voted on (since the logic for it hasn't changed in months).

Given the amount of work that's already gone into moving TO the Composer autoloader, it doesn't make much sense to me to do something else that we know is going to be temporary. The exception would be a completely separate autoloader that does a brain-dead simple PSR-4 implementation (there's sample code already available) and register it as a *second* autoloader (PHP allows for this),. which we can just rip out entirely later. That's going to show up in benchmarks as a performance regression though, so we'd have to agree to ignore that fact since we know it's temporary.

donquixote’s picture

4) Make a branch that just moves modules up a level and adjusts the wiring accordingly. This would be 99% moving files around without changes so doing it via patch makes no sense. This is a textbook case for doing a proper merge.

The moving around of files can be done with a script.
This has already been shown to work with the Krautoload approach (which is discarded for reasons other than that), and it means we can continue to work with patches.
Or even if we work with branches, having just a few changes + a script will be much easier to review.

donquixote’s picture

it doesn't make much sense to me to do something else that we know is going to be temporary. The exception would be a completely separate autoloader that does a brain-dead simple PSR-4 implementation

The alternative that I am suggesting (if we don't want to wait) is

  • A custom core/autoload.php, to be used instead of core/vendor/autoload.php. (after step 1 of #198 is committed)
    This is something we can keep even if we switch back to the (then updated) Composer class loader in the end.
  • A copy of the class loader proposed for Composer PSR-4, e.g. in Drupal\Core\Autoload\ClassLoader.
    Besides PSR-4 support, this can also have major performance improvements tailored for the Drupal use case.
    When Composer finally supports PSR-4, we will have the choice to switch back or to keep our performance improvements.
  • A copy of the AutoloaderInit class generated by Composer, e.g. in Drupal\Core\Autoload\AutoloaderInit.

Compared to the Krautoload stuff I proposed earlier, this is going to be very small, it is very similar to what we natively get with Composer, and it will be very easy to switch back to the generated Composer class loader.

@Crell, IMO this will be simpler and easier to switch back from than the second loader on the stack that you are proposing.
Since the classloader is also a service that some components depend on, it would be confusing if temporarily we have two of them.

The performance concern is a thing, but as you rightfully say, it is acceptable because it is temporary.

drifter’s picture

I think we don't want to wait, module directory structures should be simplified as soon as possible. The relevant Composer PSR-4 pull request is here:

https://github.com/composer/composer/pull/2121

Also authored by @donquixote. The approach outlined above in #200 seems fine to me, and it will be easy to switch to the official Composer PSR-4 if needed. Can we agree to move forward with this approach?

webchick’s picture

I tend to agree that sooner is better. We have a unique opportunity coming up with DrupalCon to train 1600+ developers on how to write D8 modules, and without some form of this support in core they're going to learn a method of structuring classes that's deprecated from the start.

I posted to https://github.com/composer/composer/pull/2121#issuecomment-23884358 asking if the Composer maintainers would be willing to expedite this. Given it's only our folks (specifically, donquixote) driving this, I'm not sure if this will result in anything, though.

donquixote’s picture

Great..

#2033611: Port AnnotatedClassDiscovery to PSR-4, and make it agnostic of directory information.
is still waiting for a review.
Esp, some feedback from plugin people would be welcome.
effulgentsia already posted a very useful comment there, but we need to move on.

#2058867: Consider introducing a native ClassLoader for performance and PSR-4
needs a re-roll now that the composer autoload patch has landed. I can do that myself.
But I think the patches and interdiffs already there are good enough to ask for some general feedback.

Still missing
- something for discovery of test classes. can go into a new issue.
- a script to move the class files around automatically. this already exists in the Krautoload patch, so we can simply take it from there.

donquixote’s picture

donquixote’s picture

For now I'd like to get forward with the existing issues.
When these are "approved", we can see how we put it all together.

donquixote’s picture

donquixote’s picture

donquixote’s picture

#2083547: PSR-4: Putting it all together is green and finished and needs reviews.

Dries’s picture

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.

donquixote’s picture

Can someone start a stub doc page somewhere?
I will then help flesh it out.

@webchick:
The main argument has always been DX pain caused by the redundant top-level directories. Maybe we can make this more specific, e.g. how this eats extra brain cells on the commandline, in a filesystem explorer, in an IDE, in the git repository browser on drupal.org, or any place where people interact with these directories.

@Dries:
I don't see the learning curve issue, but probably others do, so this should also go on the doc page.

We should also shed more light into whether or how this affects interoperability. And what "interoperability" really means for us.
Sure this depends on the progress with PSR-4 in the php-fig group, and in Composer.
But personally I think it does not make a huge difference either way. Reusing Drupal core or a Drupal module outside of Drupal is NOT the kind of interoperability that I consider realistic.

What I consider realistic future scenarios of code reuse Drupal vs the rest of PHP:

  1. Stuff from core and contrib being outsourced into independent Composer packages, which could be reused by the larger PHP community. PSR-0 vs PSR-4 in modules does not make a big difference for that.
  2. Composer packages that *contain* Drupal modules. E.g. the composer.json could have a key that says "Drupal", and that would point out directories that are to be scanned for Drupal modules. Maybe the same package could even have multiple Drupal modules - one for 7.x, one for 8.x, another for 9.x..
    Again, PSR-0 vs PSR-4 in module namespaces does not make a huge difference for that, because the reusable code will live outside of that.

This might not be easily possible right now, but I imagine we will get there either in post-release 8.x core, and/or in contrib with e.g. Composer manager.

tim.plunkett’s picture

"Why does the Node class have a namespace of Drupal\node\Entity if it's in core/modules/node/lib/Entity/Node.php"?

That is the biggest problem here.

In HEAD, you have core/modules/node/lib/Drupal/node/Entity/Node.php
The difference between Drupal\node\Entity\Node and Drupal/node/Entity/Node is an easy one to understand.

But with PSR-4, nothing explains the Drupal, it and instead of node/Entity/Node you have lib/Entity/Node.

If our namespaces were Node\Entity instead of Drupal\node\Entity, this would make sense.

-1, won't fix.

donquixote’s picture

@tim.plunkett (#211):
With PSR-4, the "Drupal\node\" tells you in which module dir to look. Then the remaining part "Entity\Node" tells you the relative path within ("node" module dir)/lib/.

With PSR-0, the "Drupal\node\" tells you in which module dir to look, and then the full "Drupal\node\Entity\Node" tells you the relative path within ("node" module dir)/lib/.

I don't see either of these as "does not make sense". They are both quite logical.

Which of them is more difficult to learn, that's a good question. The participants of this thread seem to disagree, and I heard from webchick that she and xjm have different observations from trainings. So this is really hard to determine.

The other question is, once you learned how it works, which of those is more pleasant and productive to work with.
I heard and read a number of "I don't care", but so far I heard noone say that the PSR-0 way is better for productivity.
There are those who are bothered by empty directories in the commandline, the filesystem explorer, the IDE, and other places. And there are those who don't care.
How heavy the productivity penalty can be, is again a good question. How would we measure it?

drifter’s picture

As I see it: if we would have invented our own autoloading directory scheme, I don't think it would have resembled PSR-0, it would have resembled PSR-4, or something even simpler. Yes, the directories don't map one-to-one, but that's something you need to understand once. After that, I'm just imagining the cumulative developer hours saved by not having 2-3 extra clicks in the IDE/editor/FTP client while browsing directories. It would help a lot.

A guesstimate: 10.000 developers saving 5 seconds a day over 220 working days in a year would amount to 3055 working hours in a year saved.

+1 for PSR-4

tim.plunkett’s picture

If 10000 developers are actually clicking through directories, it's already over.

joachim’s picture

> If 10000 developers are actually clicking through directories, it's already over.

So how do you open files?

If I open a module as a project in TextMate, then I have to click open down the chain of subfolders to get to the classes.

Maybe I am missing a better way to do it?

catch’s picture

I open files from the command line, and the couple of extra 'tab tab' to get to the right file annoyed me for the first couple of months after PSR-0 was committed. However:

1. Finding classes in 7.x annoys me more, NFI where they are without grep.

2. After a couple of months I got used to it.

3. If I really cared about stuff like that I probably would have installed an IDE by now or figured out some other way to reduce the tabbing (I realise that doesn't help people who don't know what an IDE is), but I still haven't.

It boils down to two choices:

1. Consistent to the point of annoyance (but I don't think the problem goes past annoyance/taste).

2. Less consistent and less annoying, until you get caught out by the inconsistency.

I would've gone for PSR-4 when we first adopted PSR-0, but at best neutral on it now.

If anything I find the Core and Component directories more irritating at this point, especially since we've been less than strict about what goes where.

ParisLiakos’s picture

i agree with catch..and i also agree with this

@joachim i dont know about textmate but any half-decent IDE lets you CTRL+click on the class name and it will automatically open it. or to navigate to a class by entering its name..sitting around and clicking directories is a PITA, PSR4 or not

-1, won't fix.

geerlingguy’s picture

Just judging from my experience (anecdotal, but still...), most developers I know seem to click through directories, even if they use IDEs or more advanced editors.

I'd say maybe 1/5 developers would be neutral on PSR-4 (like catch), and the rest would probably benefit by not getting RSI from clicking on lib/Drupal a thousand times. I think the 1/5 developers are also the ones who develop in other languages where class-based work is the norm (Java, Obj-C, C++...), so they're more used to the structure/shortcuts already.

Just something to consider.

I'm more neutral on this now, too, but I do think that those who have worked strictly in PHP/Drupal to this point would be helped more by PSR-4 than more 'seasoned' (is that the right term?) developers.

MrHaroldA’s picture

Both PSR-0 and PSR-4 are unfit for humans, they are only fit for parsers.

Drupal 7 will be the last version of Drupal which doesn't require a degree to understand the folder/file-structure anyway, so why bother making it a little easier with PSR-4?

According to all -1's above: PSR-4 won't make it anyway. I'd better start learning how many times my module name should be in a path, and how I can navigate quickly, without having to open 6(!) folders to get to my code.

$module_name/lib/$vendor/$module_name/Rockets/Apollo/Apollo_15.php

yched’s picture

I rarely click or cd through folders manually since the code navigation tools in PhpStorm are really handy (if you make the effort to find the good keystrokes)
But not everybody uses PhpStorm (not free), and even with other IDEs, I'm regularly baffled at how many people don't use navigations and open files manually instead...

Also, using PSR-0 in a case (drupal modules) where it makes no sense *and* a more adapted standard exists seems just misleading. Drupal modules are not interoperable libraries.

Regarding the confusion between the different patterns:

Drupal\[module]\*  -> [module_folder]/lib/*
Drupal\[other]\*   -> core/lib/Drupal/*

Would it help if the PSR-4 namespaces we register for drupal extensions (modules/themes/profiles) more clearly indicated that they follow different pattern ?
DrupalModule\[module]\* -> [module_folder]/lib/* ?

"If it starts with DrupalModule, it points to the lib folder of the module, else it points to core/lib/Drupal" seems easy to grasp ?

Crell’s picture

*sigh*

MrHaroldA: Please skip the hyperbole. You don't need a degree in anything to understand the file/folder structure. If anything it's more predictable than any previous version of Drupal, and more consistent with every other modern PHP project.

We had this discussion months ago, and the overall lean was strongly toward shortening the directory structure. I was against that until PSR-4 gained steam, because my stance is "no custom autoloaders". PSR-4 is not a custom autoloader now, so we're good. Dries approved it back in comment #111. Andreas has been working on the patch to make it happen for months now.

Why are we now suddenly seeing a surge in people opposed? Really? This is what makes people rage-quit...

MrHaroldA’s picture

MrHaroldA: Please skip the hyperbole. You don't need a degree in anything to understand the file/folder structure. If anything it's more predictable than any previous version of Drupal, and more consistent with every other modern PHP project.

Yeah, sorry 'bout that ... I'm a Drupal/PHP dev since the 4.7/5.x-era, and I'm quite frustrated I don't understand anything in Drupal 8, except for that "correctness" always prevails over "UX" or "ease of use", which is a major fail imho.

I'll unfollow this issue, simply because I don't care anymore.

donquixote’s picture

Issue summary: View changes

Issue summary cleaned up.
Noone is asking for src/ anymore, we stick with lib/.
Open questions in the "technical plan" have been resolved.

More detail about the DX impact.
This is obviously written from a personal point of view, so maybe others can put more emphasis on the supposed problems of PSR-4.

donquixote’s picture

Just to add my 2cents about DX.
A lot of this is already in the issue summary I just updated.

I use a mix:
- Command line (local and remote) for drush, grep/ack/find, git, looking into apache error.log, etc.
- Nemo / Nautilus to navigate through folders in ssh/sftp and open remote or local files in gedit.
(I never made friends with commandline editors, and it is hard to open gedit in a remote commandline)
- Gedit Ctrl+O file open dialog.
- PhpStorm for local projects, whenever I feel like it.
- Clicking around in github or drupalcode.org web interfaces.

I am aware of tab-tab in the command line, but I still would prefer not to do that.
I also find myself writing "cd ../../../../.." with +2 levels of "/../.." too often.
And to check "does this module have any production classes or just tests?" I need "ls lib TAB TAB" instead of just "ls lib".
And the redundant directories add visual clutter in error logs, stack traces, the command prompt, the command itself (e.g. "git add ../lib/Drupal/.."), git diff/status, patches, and whatnot.

In PhpStorm filesystem explorer the redundant directory levels open up automatically, which is nice. But still they add visual clutter, and they show up in search results, etc.

And for all other graphical filesystem browsing I think it is pretty obvious that the additional directories suck. Not everyone uses them, but enough people do.

----------------

Btw, the Composer people like PSR-4 too, since it allows to get rid of the confusing "target-dir" setting in composer.json, and still shorten the directory structure e.g. for Symfony packages.

--------

Besides that, as Crell says (#224), this has been discussed long ago and people opposed did not take the time to really argue for their point of view.

yched’s picture

And to check "does this module have any production classes or just tests?" I need "ls lib TAB TAB" instead of just "ls lib".

Indeed, one important UX aspect of the folder depths is: "I'm evaluating a module (either browsing the code in git.drupal.org or even locally in an IDE), I want to discover what it's doing and how - i.e: what does it have in its lib/ folder". As a site builder, it's something you do on a daily basis.
And this is not only about contrib modules, but also about understanding custom code written by someone else than yourself.

No IDEs can mitigate the "2 empty dirs" effect here. You can't auto-navigate into what you don't know, you need to open those folders in the tree view...

tim.plunkett’s picture

No IDEs can mitigate the "2 empty dirs" effect here.

Untrue (for PHPStorm). If you are in a .module, and you use the top bar to navigate, it skips empty dirs for you: http://www.youtube.com/watch?v=Bkonsrx5m0c

joelpittet’s picture

I may not have pull either way although... PSR-4++. I like PSR-0 as well as it is very consistent, helps promote good organization of classes, and both have this going for them. I use Sublime and PHPStorm and both allow me to fuzzy search for what I need without drilling around too much. And though those tools are great, I wouldn't want to make the decision based on which fancy features my tool provides me to get around the code base. We are not always in an ideal environments. So great work getting PSR-0 in D8 and here's hoping for PSR-4 to make things a little bit sweeter!

effulgentsia’s picture

"Why does the Node class have a namespace of Drupal\node\Entity if it's in core/modules/node/lib/Entity/Node.php"?

My opinion is similar to @donquixote's response in #215. In terms of learnability, I think the problem with this question is that regardless of PSR-0 or PSR-4, it hinges on first asking and answering the first part of the question in isolation: "Why does the Node class have a namespace of Drupal\node\Entity?"

The problem is that as far as PHP is concerned, that namespace is opaque: no parts of it have any special meaning apart from the others. But as far as Drupal is concerned, the "Drupal\node" part is a different thing than the "Entity" part. The distinction is similar to the difference between a surname and a given name. The class simply being in the node module means it has a "Drupal\node" surname, whereas "Entity\Node" is the given name chosen by the module author. If you're working with Drupal module code, you need to know and understand the 2 different parts of the namespace. And once you do, then I don't think there's any learnability difference between "directory = module's lib / full name" and "directory = module's lib / given name".

donquixote’s picture

@effulgentsia (#231)
To follow up on this:
When PSR-0 was created, I think the idea that a namespace should be "mapped" to a directory was not in the focus. It was a second thought, if anything.
Then (*) Composer came along, now mapping a namespace or prefix to a directory became the normal thing.
But this mapping does not play a role for the relative path of a file within the PSR-0 base directory.

With PSR-4, the mapping gets a much stronger role. Now we could even say a PSR-4 base directory is identified with a base namespace.
It is still officially allowed to map multiple base namespaces to the same PSR-4 base directory, but it is not really a good idea to do that.

As such, PSR-4 is following what has become the most common practice in Composer. Instead of big shared PSR-0 base directories, projects are split into packages, and each has a directory for classes that is identified with the package namespace. And the package namespace is identified with the package.

EDIT: So yes, the "last name" metaphor is a good one!

(*) With "Then" I am not talking about the order in which PSR-0 and Composer were introduced (too lazy to look that up), but more about a learning process in the PHP community.

webchick’s picture

In reference to a vocabulary for discussing DX issues, PSR-0 definitely falls under the verbosity category. No one is arguing that two extra directories in every module is too complex for people to handle. They are arguing that the extra TAB-TAB- or CLICK-CLICK- is a constant annoyance.

There are people here who don't get the uproar about this annoyance. Almost without exception, they are people who are core/module developers who work on Drupal code with a huge chunk of their time (most of whom use proprietary IDEs), and have the luxury of "getting used to this after a couple of months." (Though, since when was this a good bar..? Sheesh.)

But who we are not hearing from in this issue, and almost certainly won't due to the collective karma of people they'd be up against, is a huge audience who fall into the boat of only touching Drupal code when something is broken and/or when their site needs a new feature developed, and these instances might be weeks or months apart. The extra nested directories obfuscate how their modules work, and create constant, nagging irritation in every single thing they do, adding extra annoyance to performing even simple tasks. They will not be exposed to it enough to "get over it."

Further, the point of PSR-0 is for frameworks to share interoperable code. We are screwing up the semantics using this for module-specific code with Drupal-specific stuff in it like FAPI and which will never, ever, ever be shared outside of Drupal. PSR-4 is being proposed to PHP-FIG for this very reason, and it's only a matter of time before it's passed, and starts being used in other frameworks/CMSes for their plugins. Are we suggesting we suddenly stop following standards bodies because we've decided we like consistency? That seems inconsistent. :)

I am trying, but I honestly do not understand the concerns around the namespaces and directories not matching. Core and modules having different rules is absolutely nothing new, and "Module stuff starts with Drupal\module and core stuff starts with Drupal\Core|Component, but regardless it's in a folder called lib/" is not exactly rocket science to figure out.

The benefit of PSR-4 is that the code that the vast majority of our 30,000 developers are touching (aka, not Drupal core) is easier to grok and less annoying to work with every day. Given that Drupal 8 can't be successful unless a substantial portion of those developers make the leap, and given that this is something that comes up every. single. time. I've seen a new developer looking at D8, I really, really don't understand why this is not being seen across the board as a colossal win.

catch’s picture

No one is arguing that two extra directories in every module is too complex for people to handle. They are arguing that the extra TAB-TAB- or CLICK-CLICK- is a constant annoyance.

That's exactly what #22 argued. Most people only argue that it's an annoyance but there is a lot of exaggeration about exactly what the problem is (or how small it is too).

and have the luxury of "getting used to this after a couple of months." (Though, since when was this a good bar..? Sheesh.)

I didn't say it's a good bar, however it's what happened after an initially strong negative reaction (and I use vim rather than phpstorm). I never, ever got used to files[] or hook_hook_info() or registry fatals all of which have been considerably more prone to errors and taken up dozens if not hundreds of hours of my time over the past 4-5 years.

We are screwing up the semantics using this for module-specific code with Drupal-specific stuff in it like FAPI and which will never, ever, ever be shared outside of Drupal.

That's not entirely true. We currently force people to define modules in order to provide any service, for example cache backends - see #2044137: No longer possible to configure cache backends (and similar) without enabling a module.

So alternative implementations of Drupal core/component interfaces which might very well be possible to use outside Drupal, will end up with PSR-4 namespaces as well. This is a result of us conflating 'project' and 'module' in so many cases. That's its own issue that likely goes away if we sort out the site-specific container namespaces, and a small minority of classes, but it is a real problem and we might have to bump that issue to critical once we start enforcing PSR-4 for modules.

donquixote’s picture

So alternative implementations of Drupal core/component interfaces which might very well be possible to use outside Drupal, will end up with PSR-4 namespaces as well.

As in #210, I think the recommended way of code reuse would be a Composer package in combination with a Drupal module, where the module is only an adapter. Like with composer_manager.

But even without that: The vote is looking nice, lots of +1.

I never, ever got used to files[]

I agree that PSR-0 is a big improvement to files[]. Especially since it allows to determine the class file path from the (namespaced) class name, whereas with files[] it was all arbitrary.

I personally refused to do any files[] stuff in contrib modules, so I made a module which now supports both PSR-0 and a custom scheme without the deep directories (but based on underscores). So I could say I use 50% PSR-0 and 50% this custom scheme.

I never want to turn back to files[], but I also realize that I clearly prefer the less deep directories.

So:
PSR-4 = bathroom directly in your apartment.
PSR-0 = bathroom on your floor but outside your apartment. You get used to it, especially if this is all you know.
files[] = you go outside and dig a hole in the ground to do your business.
(pardon my example)

We are going to live with this for the next 3, 6 or 15 years, so we should do the right thing now. I don't think we want to switch again in D9.

msonnabaum’s picture

@webchick

In reference to a vocabulary for discussing DX issues, PSR-0 definitely falls under the verbosity category.

No, it doesn't. You could possibly argue that the namespaces we chose were verbose, but there is nothing verbose about PSR-0 itself. Similarly, if we required everyone to prefix their functions with drupal_modulename_ in D7, you could argue that's verbose.

There are people here who don't get the uproar about this annoyance. Almost without exception, they are people who are core/module developers who work on Drupal code with a huge chunk of their time (most of whom use proprietary IDEs), and have the luxury of "getting used to this after a couple of months." (Though, since when was this a good bar..? Sheesh.)

I do not spend the majority of my time on Drupal, I don't use an IDE, and I didn't have to get used to anything. I was already familiar with the pattern from every other programming environment I've used, because they all use a 1:1 mapping of namespaces to directory/files.

Getting used to it after a while isn't a bad bar at all. It's a sign that something is unfamiliar, but after living with it you realize it's not nearly as bad as you'd thought.

Also, It's unwise to dismiss the opinion of people who have lived with it. If this was a constant annoyance, they'd be the ones feeling the most pain, and you'd be seeing their support.

The extra nested directories obfuscate how their modules work, and create constant, nagging irritation in every single thing they do, adding extra annoyance to performing even simple tasks. They will not be exposed to it enough to "get over it."

How in the world does it obfuscate anything? That's quite an overstatement. Also, it's not about getting over it, it's about getting used to it. We're not giving into something that is universally annoying, we're using it enough to come to the conclusion that there's really no annoyance.

Further, the point of PSR-0 is for frameworks to share interoperable code. We are screwing up the semantics using this for module-specific code with Drupal-specific stuff in it like FAPI and which will never, ever, ever be shared outside of Drupal.

You're conflating PSR-0 with a direct mapping of namespaces to files. PSR-0 requires a vendor namespace, to prevent namespace conflicts between libraries. Using a 1:1 mapping isnt about interoperability, it's just using the simplest, most common directory layout convention for classes with namespaces.

If we want to not care about interoperability (I think that's a good idea), the argument is to get rid of \Drupal in module namespaces, not PSR-4.

PSR-4 is being proposed to PHP-FIG for this very reason, and it's only a matter of time before it's passed, and starts being used in other frameworks/CMSes for their plugins. Are we suggesting we suddenly stop following standards bodies because we've decided we like consistency? That seems inconsistent. :)

It's hardly guaranteed that it will be widely adopted. I'd actually be surprised if it was considering it's inconsistent with the rest of the programming world and more complex than PSR-0.

And yes, we shouldn't follow every standard they put out because some of them are dumb (PSR-2).

I am trying, but I honestly do not understand the concerns around the namespaces and directories not matching.

It's concerning because it's confusing.

Core and modules having different rules is absolutely nothing new, and "Module stuff starts with Drupal\module and core stuff starts with Drupal\Core|Component, but regardless it's in a folder called lib/" is not exactly rocket science to figure out.

I think that sentence illustrates the problem quite well. That really doesn't make sense at all.

The benefit of PSR-4 is that the code that the vast majority of our 30,000 developers are touching (aka, not Drupal core) is easier to grok and less annoying to work with every day.

Totally untrue that it's easier to grok, and subjective at best that it's less annoying.

Mark Trapp’s picture

Can the issue summary be updated to include the technical reasons why both PSR-4 and PSR-0 can't be supported, allowing contrib/custom to decide which they want to use? The only mention I found was #179 where it seemed like removing PSR-0 support was already decided and there was general agreement ("most of us"), but I couldn't find any mention of a discussion where that decision happened. There were a number of comments before #179 that talked about support for both and most (all?) of the third-party autoloaders all seem to be keen on supporting both.

One other point I wanted to make was that an end user developer (end developer?), I found it strange that there is a strong sentiment here that Drupal modules do not (should not?) contain interoperable code. I was under the impression that most of these big changes were to foster interoperability between Drupal and the larger PHP world, and having a solid OOP base usually encourages interoperability.

I'm sorry if that second part is somewhat off-topic, but this issue is the first one I've seen where the sentiment that Drupal modules don't contain interoperable code was expressed, and I don't think that's common knowledge, particularly given how people have been blogging about why Drupal 8 is using so much third-party code. It'd be helpful to either have in the issue summary or change notice or documentation somewhere in all of this what those of us who do want to incorporate interoperable code (notably for this topic, PSR-0 compliant interoperable code) in Drupal modules are supposed to do. Or is the assumption—going into Drupal 8—that if you aren't writing 100%-Drupal specific code, you shouldn't be using Drupal? If that is the case, I think that needs to be made much more clear.

dsnopek’s picture

@Mark Trapp:

One other point I wanted to make was that an end user developer (end developer?), I found it strange that there is a strong sentiment here that Drupal modules do not (should not?) contain interoperable code.

I think you're misunderstanding. The idea is that if you are writing interoperable code, you'd put that into a seperate project and pull it into Drupal via composer. So, your module code would only contain the Drupal-specific code, and the interoperable bits would be in the seperate project - but you as the Drupal developer would still be writing both parts.

The Drupal 7 equivalent, would be making the generic code as a seperate project and then including it via the Libraries API.

I hope that makes sense (and that I'm correctly understanding what everyone else is saying above)!

donquixote’s picture

Issue summary: View changes

@dsnopek, Mark Trapp:
Updated issue summary, "Interoperability" section.
While I generally agree with dsnopek, it also should be mentioned that modules with PSR-4 are not going to be any less interoperable than those with PSR-0. Both php-fig and Composer are approving it. This is the future.

Mark Trapp’s picture

@dsnopek, @donquixote: thanks, I think that clears it up for me: as long as there's still some mechanism for autoloading PSR-0 compliant code, that allays my concerns.

donquixote’s picture

As mentioned in the other issue, the vote for PSR-4 has finished.
https://groups.google.com/forum/#!msg/php-fig/L8oCDQCzDcs/Z-6_9U0hpJIJ (philsturgeon)

Out of 30 current members 29 voted one way or the other, with only 1 forgetting to vote.

For: 26
Against: 2
Abstained: 0
No Vote: 1

This clearly passes quorum and holds a strong majority, so the vote has passed!

PSR-4 is now a thing. The Composer team are on the case and have a PR for PSR-4 support that will be hopefully worked in within the week.

effulgentsia’s picture

Great news about PSR-4 now being an official standard.

In terms of us moving to it for modules, I've pretty much always been and still am +1 to that.

However, I'm curious what people think about the following potential snag:

What we do for core modules and recommend for contrib modules is to have a 1:1 relationship between a namespace prefix and a module's "lib" directory. Specifically, if the module name is "foo", the namespace automatically mapped to modules/foo/lib is Drupal\foo. And I think this makes a lot of sense for modules that are on drupal.org. IMO, by virtue of being on drupal.org, they are "owned" by the Drupal community, regardless who the original author or current maintainer is, so "Drupal" is the proper root namespace, and the module name is a logical next part.

But, what about custom modules that aren't intended for sharing on drupal.org? I can imagine a team working on a custom site deciding on some other namespace convention: for example, if the team consists of multiple distinct companies/freelancers, perhaps they'd want a namespace convention of company name. So, module "siteX_custom_code" might have classes like CompanyX\SomeClass and CompanyY\SomeOtherClass.

I'm assuming such a thing would be possible via code in the .module file that explicitly registers these namespaces, either as PSR-0 or PSR-4, depending on what that team wishes to do, but my question is:
1) Do we think that something like this is a legitimate/likely use case?
2) Does the presence of this use case present a DX/consistency problem (people now have to know that *some* modules have their "lib" directory start at a known namespace (Drupal\$module), while *other* modules have their "lib" directories containing something other than that)?

donquixote’s picture

In response to #242:

I think it makes sense to have code that directly interacts with Drupal be under the Drupal namespace. This might be a bit exotic compared to the rest of the PHP world, but it would be consistent with how things currently work and have been designed until now. There are some mechanics in core that build on the assumption that the namespace is always Drupal + "\\" + module name, and I think it is too late to change that now.

The main purpose of namespaces, to prevent name clashes, would not work anyway: If you have CompanyX\mymodule, and CompanyY\mymodule, then the two module names would nameclash, even if the namespaces do not nameclash. Drupal does not allow two modules with the same name.

I think the best is to do it like in the old days, where you simply put an underscore prefix in front of the module name. So, Drupal\acme_foo\SomeClass.

This being said, you can still have your module interact with a library that lives in a non-Drupal namespace. This library could be pulled in with composer_manager, or it could be shipped with the module, in a subfolder. So you would have Drupal\acme_foo\Plugin\Block interact with Acme\FancyPants. The library code, if shipped with the module, could live in $module_dir/acme/.., using either PSR-0 or PSR-4 or whatever you want.

You will want a mechanic to register the Acme\ namespace in the Drupal classloader. But this is independent of whether Drupal uses PSR-0 or PSR-4 for modules.

The one thing that the switch to PSR-4 would prevent is that you have the Acme\ namespace stuff in the same lib/ folder as the Drupal stuff. But that would be a bad idea anyway, IMO.

donquixote’s picture

Btw, the following issue has not received enough attention yet:
#2094843: Port PHPUnit test classes in (module dir)/tests/ to PSR-4

Crell’s picture

Feels weird saying this but... "What donquixote said". :-) As a practical matter, D8 can't not use Drupal as the definitive universal namespace. Ask again in D9 and we may be able to do something else.

webchick’s picture

I agree. It would never occur to me to put Acquia\ as the name space for any Spark modules, whether they lived on d.o or not. They are for Drupal, hence Drupal\.

Dustin@PI’s picture

"There are some mechanics in core that build on the assumption that the namespace is always Drupal + "\\" + module name, and I think it is too late to change that now."

Would Sub-modules and Distribution modules be a level deeper?

So if modules/foo/lib is Drupal\foo--Would commons/modules/commons/commons_blog/lib be Drupal\commons\commons_blog And modules/foo/foo_sub/lib be Drupal\foo\foo_sub?

webchick’s picture

Nope. They're all Drupal\foo, because there can only be one foo module in all of your Drupal install.

Dustin@PI’s picture

Thanks @webchick ... I just want to double check my understanding: According to psr-4-autoloader.md you can have a "SubNamespaceNames" (\<NamespaceName>(\<SubNamespaceNames>)*\<ClassName>).

So Drupal 8 won't be-able to support sub modules with SubNamespaceNames? i.e. if foo has a sub module foo_sub then foo_sub's namespace would be Drupal\foo_sub and not Drupal\foo\sub.

I think that being able to support SubNamespaces for sub-modules and distributions would be good functionality. I understand that it's probably too late for D8--I'll open up a postponed issue.

tim.plunkett’s picture

By SubNamespaceNames, it means \Drupal\node\Entity\Node, where Drupal is the NamespaceName, node and Entity are the SubNamespaceNames, and Node is the ClassName.

I understand your suggestion, and it does sound interesting for submodules, but I can't imagine how that would work. A separate issue is a good idea though.

Dries’s picture

It sounds like there are still some legitimate concerns with PSR-4, despite the DX benefit that it brings in terms of reducing verbosity. For example, that namespaces no longer map cleanly to class directory structures, and that it requires new module developers to learn two different ways of interacting with OO code.

I spent a lot of time reviewing this and have a possible counter-proposal, which would still allow for the DX improvements, while still benefitting from all of the goodness we get from PSR-0. Let's call it PSR-Dries. :-) Here's a comparison:

PSR-0 (across the board - what we currently do)

drupal-8/core/lib/Drupal/Core/Entity/Entity.php
drupal-8/modules/mymodule/lib/Drupal/mymodule/Form/MyForm.php

The DX concerns being raised around PSR-0 are that there is a lot of needless repetition and verbosity in the folder structure for modules. Since I'm already in a directory called "modules/mymodule" in a directory called "drupal-8" it is superfluous to have to re-state those directories once again in my project root directory. This repetition is both a one-time "ahhh! scary!" when new developers encounter Drupal 8, making it appear more complex than it actually is, as well as an ongoing annoyance when having to double-click, , or "../" two additional directories for every file edit.

PSR-4 (for modules only - what is being proposed)

drupal-8/core/lib/Drupal/Core/Entity/Entity.php
drupal-8/modules/mymodule/lib/Form/MyForm.php

This is slightly better, in that the extra two directory noise is gone, but it is still not ideal. For example, while the namespace of the MyForm.php class will be Drupal\mymodule\Form\MyForm, and unlike with PSR-0 (and with namespaced code found in other languages, e.g. Ruby), this does not map to the directory structure on the filesystem. This therefore presents a learnability challenge in that sometimes, e.g. Drupal\Core\Entity maps to lib/Drupal/Core/Entity/, but other times not. Module developers need to learn this difference in the way core-provided libraries operate vs. the way module-provided libraries operate.

PSR-Dries (PSR-0, but with different directory layout in Drupal)

drupal-8/core/Drupal/Core/Entity/Entity.php
drupal-8/modules/Drupal/mymodule/Form/MyForm.php

By adjusting the directory structure of PSR-0 so that "Drupal" is included, we solve several issues:

  1. Within a module root, module authors have free reign to organize and structure their OO code however they'd like. No need to submit to a strange folder convention that they would not necessarily use if the code were written outside of Drupal (also makes forward-porting potentially easier).
  2. A namespace like Drupal\Core\Entity now maps directly to its underlying directory structure, e.g. Drupal/Core/Entity; no need for the "special" knowledge that this structure is actually under a folder called "lib" (which, while not a Drupalism per se, is also not exactly widespread either). This improves the learnability of Drupal 8 code.
  3. Unlike with PSR-4, there is only one set of rules for both core and module developers to learn in terms of how class files are organized.
  4. Least amount of nested directories, yet it follows PSR-0, which is most widely understood and widely used standard.

To me it looks like PSR-Dries offers the best DX.

Introducing this extra directory would obviously break all of the patches in the queue, which is exceptionally problematic given that we are actively trying to get beta1 out the door. On the other hand, I think that shooting for a solution to this critical DX problem that everyone can live with in D8 is vastly preferable to introducing a sub-optimal change in D8 that we then have to re-do again in Drupal 9, as this will make backporting fixes harder (as the core/ directory move did in D8 => D7) and will also invalidate tons of existing documentation.

What are your thoughts?

chx’s picture

1. So node would live under drupal-8/core/modules/Drupal/node then? I think the opposition against such an arrangement was that then classes would live in the top directory, so drupal-8/core/modules/Drupal/node/NodeViewBuilder.php and for some reason this was undesirable.

As for me, I obviously love it :)

2. May I suggest:

  • contrib: drupal-8/modules/Drupal/mymodule/Form/MyForm.php as Dries suggested
  • core module: drupal-8/Drupal/modules/node/NodeViewBuilder.php where we rename core to Drupal instead of adding it. This would cause the following supershort paths:
  • Core lib: drupal-8/Drupal/Core/Entity/Entity.php
  • Component lib: drupal-8/Drupal/Component/Utility/String.php
Crell’s picture

What are your thoughts?

My thoughts can best be summed up by curling up in a little ball and crying that we're still bikeshedding this issue after two years, and repeating the same debates we had two years ago.

1) Using a non-PSR autoloader means we cannot use Composer for our autoloading, which allows us to have only one autoloader active plus having all of core's classes and some functions available from the very first line in index.php. Not using a PSR autoloader means we are forced to have 2 autoloaders, or to entirely reimplement Composers's for our composed code. We only just recently finally got this all working, so that would be a regression.

2) Using a non-PSR autoloader is a Drupalism. While the rest of the PHP world has landed on PSR-0, and PSR-4 is its inheritor, we're going to go from PHP-standard PSR-0 to custom-Drupal-thing-that-no-one-else-understands, which would serve only to dissuade the broader PHP community from working on Drupal? "Following the same convention as every other major project" is good DX. That's a regression.

3) Where does non-OOP code go? We hashed through that at length 2 years ago, and concluded that the lib directory for OOPy code and "what we're already doing" for the rest made the most sense. (lib vs. src landed on lib because "non-OO code is source code, too". I personally don't give a damn which of those we use.) This proposal has nowhere to put .module, *.yml, template, or config files. That's a regression.

4) What about the fact that we have 4 different places that modules can live? It's not immediately obvious to me what happens with that. That's a regression.

5) Modules are no longer self-contained. They can only be autoloaded if they're placed in the right place with respect to some other magic directory outside of themselves. That is completely non-obvious. That's a regression.

At the end of the day, as I see it we have 3 options:
1) Use PSR-0.
2) Use PSR4.
3) Be isolationist idiots.

I really don't care which of the first two options we take, as long as we take it and commit to it and stop waffling. (Although given that Andreas is writing the PSR-4 support for Composer it would seem logical to use it.) Taking option 3, well, I think my opinion of that is abundantly clear.

aspilicious’s picture

Not really an opinion...
I'm not against it...
But I see some troubles

The main folder probably will look messy with a lot of OOP code mixed with procedural stuff.
And putting it inside a folder (to clean it up) will force an additional layer inside the namespace.

And than we have this issue:
==> A "Tests" directory containing the test classes
==> A "tests" directory containing test modules

And another issue, submodules will break everything. As you have a nested namespace now :)

xjm’s picture

We already decided not to support submodules awhile ago:
https://drupal.org/node/1532250

Mark Trapp’s picture

@Dries

+1 from me, a lowly custom developer. Coming from other languages, PSR-0 (minus the underscore interpretation thing) always made a lot more sense to me and matched my expectations more.

@Crell

PSR-Dries is PSR-0 compliant, is it not?

A fully-qualified namespace and class must have the following structure \<Vendor Name>\(<Namespace>\)*<Class Name>

Still holds: \Drupal\mymodule\Class

Each namespace must have a top-level namespace ("Vendor Name").

Still holds: \Drupal

Each namespace can have as many sub-namespaces as it wishes.
Each namespace separator is converted to a DIRECTORY_SEPARATOR when loading from the file system.

Still hold.

Each _ character in the CLASS NAME is converted to a DIRECTORY_SEPARATOR. The _ character has no special meaning in the namespace.

Legacy rule that was dropped in PSR-4, but nothing about this new structure would indicate a fundamental incompatibility with it

The fully-qualified namespace and class is suffixed with .php when loading from the file system.
Alphabetic characters in vendor names, namespaces, and class names may be of any combination of lower case and upper case.

Still hold.

aspilicious’s picture

I'm exteremly -1 to the propasal after some questions in IRC.
This proposale would mean I have to subdir my module one level if I have "module packages".
Tkae for example display suite. It's a collection of several modules

ds
ds_ui
ds_extras

This proposal forces the directory structure to be

Drupal/
--modules/
----ds/
------ds/
------ds_ui/
....

This means my namespace for the main module always have to contain 2!!! ds parts.
I don't want that... And it makes the namespace of each submodule longer to. :( :( :(

This applies to every "packaged module".

jbrown’s picture

So this is really PSR-0 for both core and modules, except that now the "lib" dir is gone.

One point I am not clear on - are you proposing that there be only one PSR-0 directory for all the modules in core and only one PSR-0 directory for each project from drupal.org ?

If a project from drupal.org contained multiple modules, would there would only be one "Drupal" PSR-0 directory for the whole project?

For example:

drupal-8/modules/rules/rules_admin/rules_admin.info.yml
drupal-8/modules/rules/rules_i18n/rules_i18n.info.yml
drupal-8/modules/rules/Drupal/rules_admin/Form/MyForm.php
drupal-8/modules/rules/Drupal/rules_i18n/Form/MyForm.php

Or would it be:

drupal-8/modules/rules/rules_admin/rules_admin.info.yml
drupal-8/modules/rules/rules_admin/Drupal/rules_admin/Form/MyForm.php
drupal-8/modules/rules/rules_i18n/rules_i18n.info.yml
drupal-8/modules/rules/rules_i18n/Drupal/rules_i18n/Form/MyForm.php

At the very least, getting rid of the "lib" directory looks attractive - even for PSR-4!

donquixote’s picture

@Dries, everyone:

  1. The proposal by Dries in #252 is very much like PSR-0 with Composer's target-dir setting. The Composer guys already do not like this anymore, and want to deprecate it in favor of PSR-4.
  2. It interferes with modules in subdirectories, or makes them less pleasant.
    E.g. modules/Drupal/views/Drupal/views_ui/Form/ViewsUiForm.php
  3. It will muddle all non-OOP files together with the OOP files, as Crell already said above.
    E.g. modules/Drupal/views/Drupal/views_ui/views_ui.api.php

------------------

As sad as it sounds, I don't think there is going to be a "magic 3rd way" to solve all our problems. Or someone would have come up with it in the long time this has been on the table.

The "namespaces no longer map cleanly to class directory structures" is a sacrifice that was made intentionally in the design of PSR-4.
The expectation that directory = *complete* namespaced class name is coming from PSR-0. If we choose PSR-4, we are officially giving up on this expectation.

I think the best way to decide is to browse through and explore a version of Drupal where the transformation has already taken place, like here: https://github.com/donquixote/drupal/tree/D8-psr4-moved-2083547-75/core/...

xjm’s picture

For what it's worth, I find the benefits of both PSR-4 and "PSR-Dries" to represent negligible DX improvements. To me, they're just a slight reduction in the abuse of my tab key, and I know I would find that even less of a benefit if I actually used an IDE. Maybe I'd be glad to have that tab key abuse gone once the change were made. However, to me, it's debatable whether or not the improvements outweigh the drawbacks and the technical challenges, and some of it is going to boil down to personal preference. I do recognize that others find the directory structure with our current PSR-0 implementation far more annoying than I personally do.

However, from my perspective, the entire discussion is a huge distraction from the 595 other criticals and majors that deserve our attention.

Specifically, because of this:

Introducing this extra directory would obviously break all of the patches in the queue, which is exceptionally problematic given that we are actively trying to get beta1 out the door.

A disruption of that nature at this point in the release cycle should IMO require a clear, incontrovertible, and significant improvement to Drupal. This is not that. Therefore, I'm opposed to both proposals. I don't like PSR-4 for a number of reasons discussed elsewhere, but I am even more opposed to "PSR-Dries" for Drupal 8 because (unlike the PSR-4 implementation which has a working patch) it has unresolved technical challenges, and it would break every patch everywhere (instead of merely requiring gradual, rolling rerolls on patches against core modules as the PSR-4 change would).

API freeze was more than five months ago. Let's focus on what brings Drupal 8 closer to release.

chx’s picture

So I believe we just need to make sure that submodules are covered. So because Dries' didn't cover core modules I proposed in #253 renaming core to Drupal and thus keeping Dries' contrib path but even more shortening his core example. This is totally PSR-4 compatible:

Fully Qualified Class Name Namespace Prefix Base Directory Resulting File Path
Drupal\node\NodeViewBuilder Drupal\node drupal-8/Drupal/modules/node drupal-8/Drupal/modules/node/NodeViewBuilder.php
Drupal\Core\Entity\Entity.php Drupal\Core drupal-8/Drupal/Core drupal-8/Drupal/Core/Entity/Entity.php
Drupal\foo\Some\Class.php Drupal\foo drupal-8/modules/Drupal/foo drupal-8/modules/Drupal/foo/some/class.php

The last one could be a PSR-0 root for all contribs set to drupal-8/modules but I am not so sure that's a good idea because of not installed modules lurking there which the classloader would know nothing about, so PSR-4 rather. Same reason I would rather do a prefix per core module.

Yes procedural and class would mix but I have no idea how's that a problem when you have the file extension to tell them apart.

jbrown’s picture

We need PSR-0 because the directory structure has to match the namespace declaration in the PHP file.

We only need one PSR-0 "lib" directory per downloadable project.

Core can have all it's PSR-0 code in /core/lib, for example:

drupal-8/core/lib/Drupal/Core/Controller/AjaxController.php
drupal-8/core/lib/Drupal/user/Entity/User.php

and each downloadable module project can have all the PSR-0 code for all the modules in the project in a single "lib" directory, for example:

drupal-8/modules/myproject/lib/Drupal/mymodule1/Form/MyForm.php
drupal-8/modules/myproject/lib/Drupal/mymodule2/Form/MyForm.php
drupal-8/modules/myproject/mymodule1/mymodule1.module
drupal-8/modules/myproject/mymodule2/mymodule2.module

Every module doesn't need to have its own personal "lib" directory.

yched’s picture

Yes procedural and class would mix but I have no idea how's that a problem when you have the file extension to tell them apart

No, no, no, please no :-)
Very much -1 to a [mymodule] folder with a sad mess of .module, .info.yml, .inc, classes.php, folders for classes in subnamespaces, folders for css, folders for js...
Human discoverability of what a random module does (whether some contrib you're evaluating on git.drupal.org or some unknown custom module on a given site) is critical. a lib/ or src/ folder for autoloaded classes is key to that.

Still +1 on PSR-4... As stated above, we can name our auto-registered PSR-4 prefixes DrupalModule/[module_name] instead of Drupal/[module_name] if we're worried about the confusion between "what's PSR-0 and what's PSR-4".

dsnopek’s picture

I agree with Crell - either PSR-0 (as we have it now) or PSR-4 would be fine - but not this new proposal (no offense to Dries!).

donquixote’s picture

Issue summary: View changes

I cleaned up the issue summary a bit, and added a link to the browsable version of Drupal with PSR-4.

I am tempted to remove a lot of the older cruft under "Originally proposed alternatives", but then the starting comments would get out of context.

DamienMcKenna’s picture

There's no +1 for comments on d.o unlike gdo, but +1 for what xjm said in comment #261.

IMHO one vs the other vs the other makes such a negligible difference in comparison to everything else, once people play with $directory_structure for a little which, and once it's PROPERLY DOCUMENTED, nobody will care. Meanwhile we're wasting tons of time debating it.

DamienMcKenna’s picture

Put another way, how many lost hours of productivity will it take for (possibly) every single patch and contrib module having to be restructured again if either PSR-4 or PSR-Dries were implemented? What other work could be done with the time this would take?

donquixote’s picture

I don't know if this could change your mind, but:
The patch contains a script. You run the script, and it will move the files in your locally modified Drupal.
You could even combine this with git filter-branch, if you have a local branch and want to keep a useful history. (I did not try this combination yet, but I assume it works)

The thing I have to counter is that the choice we make here will stick around for years or a decade, and will not be so easy to change later.

Think of the "<?php" you put on top of each PHP file. Sure we can all live with that. But if you had the choice, you'd probably rather get rid of it.

clemens.tolboom’s picture

I'm for PSR-x standards and don't mind opening subdirs from an IDE.

I really like the lib/ dir to know all OO stuff is in there.

My DX is a no brainer:

  1. Check out what devel module is doing then copycat it
  2. Check for core modules.
  3. Check online for PRS-0/4 related puzzles
  4. Check Symfony documentation

So please no PSR-Dries as that requires to check Drupal documentation instead of great(er) online places.

webchick’s picture

So, a few questions, based on the above:

- Crell says this PSR-Dries is a Drupalism. Mark Trapp says it's PSR-0 (which is also my understanding). Which is it, and why?

- Why exactly is it bad if both procedural and OO code get mixed into the same folder? Isn't it merely an implementation detail whether something is coded in a class or a hook or a YAML file? Isn't what's most important is that people coming into a module fresh (including yourself coming to your own module, a year from now :D) can grok what it does quickly, without having to know that, e.g. blocks are implemented as classes and therefore will be found at lib/.../SomeBlock.php? (That was certainly possible in D7, at any rate. Just pop open the module and scan the function list, there's your table of contents... we also threw .test files in root along with .module files and no one died. :))

- Assuming it actually is bad, though, why would we end *up* with procedural and OO code mooshed together in the same folder anyway, given that in almost any module's lib/Drupal/xxx/ folder, classes are further sub-divided there into e.g. Form, Controller, Tests, and so on? This seems no different conceptually to the fact that we keep templates in a "templates" folder or js/css in their respective folders.

Sub-modules still seem to be a big issue, and the concerns about unknown implementation details (if it is in fact a Drupalism and can't be done with standard PSR-0/PSR-4 autoloaders) are valid.

I do just want to point out that "two years" what we agreed to was this: #1240138: Adopt PSR-0 namespace convention for core framework classes

"Adopt PSR-0 namespace convention for core framework classes" (emphasis mine)

...

"This applies only to core framework code at this time, and does not include modules, hooks, theme keys, or anything else other than core classes. Procedural framework code in Drupal core is unaffected." (emphasis yours)

And while yes, it's certainly late, this was declared a critical DX task before API freeze. It's not Drupal's fault that PHP-FIG took so long to approve the standard.

pounard’s picture

Problem with the proposal is that once you switch your modules in a site specific folder, it doesn't work anymore. Or am I wrong?

Crell’s picture

webchick: It's possible that PSR-Dries or something like it could be implemented through a very clever hack of a PSR-0 or PSR-4 class loader, by doing screwy things with the way a given path is registered. I haven't tried or verified if it could be. However, it would be a hack; if you tell a random PHP developer who has been paying attention for the past 3 years "yeah we're using PSR-0, but your directory is actually not the full PSR-0 path but this other fraction of it, so it doesn't look like PSR-0 even though we're saying it is, no really we're following standards, honest!", they'll first look at you in shock, type "..." a lot, then hang their head and walk away. As a barely-relevant analogy, it's like saying "sure we have hot and cold running water; hot during the day, cold at night. What's the problem?"

Submodules work fine when every module is its own "starting point", from which a module's classes descend. PSR-0 or PSR-4 "as intended and as people expect them" should work fine for that. PSR-Dries or such hacks, no, that doesn't work.

I recall the original PSR-0 debate well; I also remember that we were talking about modules the entire time anyway, and what is in Dries' comment above is, actually, something that we talked about, in person, at the Symfony Decision Meeting in February 2012 at the OCTO office. I distinctly recall sun writing it on the whiteboard on Sunday. And it was rejected as being too oddball and having too many issues, and "it's a directory, get over yourself."

To be honest, the supposed DX issues of PSR-0 are horribly overblown, as are the would-be issues of PSR-4. It's a directory. They don't bite.

My position still stands:

1) PSR-0 (for realsies)
2) PSR-4 (for realsies)
3) Be stupid

There is no option 4.

chx’s picture

*knock, knock* is this on? Did anyone see #262? PSR-4, yes, no hack?

joachim’s picture

One problem I see with PSR-Dries even before we get to what's inside a module folder:

> drupal-8/modules/Drupal/mymodule

We had a major upheaval to take modules out of sites/all/modules and into a top-level folder. To me, the proposal appears to be a partial regression of that.

It could also quite easily look like a DrupalWTF to newbies, who would be quite justified in asking 'What on earth ELSE would I put in my modules folder OTHER than Drupal modules???'

donquixote’s picture

@webchick:
(see also what I said in #260)

Crell says this PSR-Dries is a Drupalism. Mark Trapp says it's PSR-0 (which is also my understanding). Which is it, and why?

Mark Trapp is right, it *is* PSR-0 (or it can be, if we want to), but there are a few IFs attached.

  • Modules in subdirectories and custom/contrib need extra subfolders so this is still all PSR-0 compliant.
    (sites/*/)modules/contrib/Drupal/views/Drupal/views_ui/Foo/Bar.php
    We'd have a number of these additional Drupal folders all over the place.
  • We have to choose either PSR-0 OR PSR-4, we can't have both. Some contrib module WILL have an underscore in the classname, and then this decision will matter.
    Of course what we CAN do is register the stuff as PSR-4 but still attempt to be PSR-0 compliant. (which means we need to avoid underscores after the last namespace separator like the plague).
  • What *would be* a Drupalism is that we (as site builders) can't just download our modules to any folder we want, but have to carefully design and maintain this folder structure. As mentioned in #260, this would be very much like the Composer "target-dir" setting, which results in PSR-0 compliant code, but is very much a "Composer-ism". And our situation would be even worse, because we would have to manually babysit this awkward directory structure. E.g. if you decide that some modules should go into modules/features instead of modules/custom, then you'd have to pay attention to create the modules/features/Drupal/ subfolder and put the modules there instead.

Why exactly is it bad if both procedural and OO code get mixed into the same folder?

Again, I think it helps to have some showcase to browse through, instead of being so theoretical.

PSR-4, moved, eliminated lib/Drupal/$modulename/ (showcase, won't run):
https://github.com/donquixote/drupal/tree/D8-psr4-root-showcase-1971198-...
This does NOT contain the additional "Drupal/" subfolders I mentioned above.

PSR-4, moved, eliminated only Drupal/$modulename/:
https://github.com/donquixote/drupal/tree/D8-psr4-replace-autoload-moved...

Please note the "tests" vs "Tests" directories, which is one problem we would have to solve (e.g. on Windows systems where directories are case insensitive). This has been mentioned above.

That aside, for me personally this directory feels too crowded.
And: We had the discussion about lib/ vs not lib/ ages ago, and it does NOT help to reiterate decisions all over, UNLESS we have new information available that we did not have before.
(such as, PSR-4 being an official standard, which was not the case when we decided to implement PSR-0 for modules.)

catch’s picture

For history of the extra directory discussions, [#5420864] had some, also #1290658: Move all module-provided classes to PHP namespaces (PSR-0 or similar), and autoload them without the registry [policy, no patch] and [#5621836]. I feel like there was another issue where the lib/Drupal/ModuleName issue and alternatives came up but couldn't find it so it maybe it was just these.

I'm pretty sure the PSR-Dries idea came up in those issues, maybe from Damien Tournoud, and there was a similar split of opinion between people who liked not having sub-directories to find classes in, vs. people concerned about having classes lumped in with .module assets and .info.yml, which is obviously incompatible at least in terms of /lib/ (and you don't want lib in a namespace).

When that came up, I both thought it'd be possible with a PSR-0 autoloader (if we had DrupalModules/ModuleName as the module directory), and that mixing the stuff together wasn't that bad - certainly not a regression from 7.x.

However I agree with xjm on this issue in general, just putting this in for background.

donquixote’s picture

Back to the original PSR-0 vs PSR-4:
I'd say there are two different dimensions to this discussion that we should distinguish:

  1. "I worked with both PSR-0 and PSR-4 for some time, and see / don't see a major DX difference."
  2. "I worked with PSR-0 for a some time, and I absolutely love it / don't see a DX problem / got used to it / absolutely hate it."

My personal motivation comes from the first dimension. I spent time working with a PSR-4-like directory structure and with PSR-0, and this made me really really prefer PSR-4.

I am not saying that we will all die with PSR-0. I am saying that PSR-4 is significantly more pleasant to work with, and this for me makes it worth the effort to switch. But I see it this way because I actually did spend some time with either option.

(I am tempted to make a "Stockholm Syndrome" reference regarding PSR-0, but I realize this is unfair.)

------------

And btw, there is more to it than just "more buttons to click" or "more times to hit tab" or "longer paths to type". There is also the stuff that goes into my brain, e.g.
"Tests? Isn't that the stuff in lib/Drupal/$modulename/Tests or tests/Drupal/$modulename/$tests? Yeah I remember."
as opposed to
"Tests? Isn't that the stuff in lib/Tests or tests/lib? Yeah I remember."
Whenever I think of tests or plugins or other stuff in these folders, I find myself juggling with these ugly bulky pieces of information in my head, when I know they could be so much smaller and nicer.

pounard’s picture

I don't have a strong opinion to debate here PSR-0 vs PSR-4; I don't like PSR-4 for a lot of reasons but they are not revelant to this issue. Something that is bothers me would be using a non PSR-* standard which would be getting away of tools such as composer or loosing a lot of flexibility.

Trying to find solutions in moving directories here and there are even worst IMO because it's like trying to bend everything just in the name of DX, loosing a lot of other flexibility. It's like trying to plug mecano and legos together, at some point it will explode.

I'm not against merging core modules into a single huge code tree, why the heck not? Core is core, and those are not meant to be dissociated (I mean at least not for packaging purposes) as long as they live in core, so why not: it would indeed make things less painful to browse and structure will even be clearer while navigating in the code; But for for contrib it's not even feasible: I think that integrators or sysadmins need to be able to put their module's code at the place they want to, no matter where it is within the PHP include path, the only pre-requesite for this to work is to have a good autoloader: which makes us wanting even more composer for generating the good autoloader; Which makes us fallback to PSR-0 and PSR-4.

I think a good solution would just be to support both PSR-0 and PSR-4 (we basically have nothing to do to support them gracefully than telling composer which to use for which module) which basically is just as hard as writing composer.json files. That wouldn't bother me and I'd have the choice between PSR-4 or PSR-0 or even a classmap (as composer supports it) and leave that battle for core developers that want to bikeshed all day long on how the core classes should be written in the filesystem for core modules.

sun’s picture

@Crell is right on all points in #254. I did not expect that part of the epic original PSR-0/namespacing discussion to get re-opened either. We collectively identified various problems with that idea in the past already.

(Unfortunately, that painful discussion happened on 10+ different issues with ~300 comments each and I can't find the one that has all the counter-arguments for that proposal.)

PSR-4 is the successor of PSR-0 for package based autoloading, with two discrete benefits over PSR-0:

  1. DX/UX: Direct access to files without unnecessary repetition of vendor/package subdirectories.
  2. Performance: Direct mapping of \Package\Space\Name to Package/Space/Name.php without any special cases (PEAR).

An important aspect: A package retains full ownership and freedom within the package directory. A package may ship with other files, but the package is still self-contained. Object-oriented PHP code is cleanly separated from other data. Sub-modules still work like they do now. To completely remove a (project) package, you delete the package directory.

The concern of DX does not only apply to your local development working directory, for which you might (not) use an IDE. It also applies to browsable online code repository viewers (extra clicks), quick introspection of module tarballs, file paths in documentation, URLs, etc.pp.

It is also a major selling point for Drupal 8: Legacy developers are needlessly scared by the perceived complexity of all the nested subdirectories. By removing them, the complexity goes away and what remains is a very simple change towards more structure: "Most of what has been in your monolithic .module file before is moved into topical files in the /lib directory, simple!" — Give or take some better wording and you can easily turn the topic of OOP that was previously perceived as a problem into a happy unique selling point.

As someone who took a break from Drupal development for a longer time-frame (and also, who's still not using an IDE), the unnecessary nested subdirectories in every module do present plain ugliness, a big annoyance, and make things much harder to understand. The switch to PSR-4 not only resolves discovery, usability, and learnability — it additionally takes the wind out of Backdrop's sails; the perceived complexity is vastly decreased.

PSR-4 was designed for modular applications that are composed of add-on packages. In other words (note: stretched interpretation), it was explicitly designed for applications like Drupal, Joomla, and others, who share the architectural design of application specific extensions that plug into a larger base framework.

Several PHP web application stakeholders (including Drupal) discussed the need for a successor of PSR-0 and agreed on the new PSR-4 standard together. As a consequence, PSR-4 will become a common way for naming, placing, and autoloading object-oriented PHP code in the future. Externally authored tools will emerge, which will automate and simplify the management of PSR-4 compliant application stacks.

That is, not to mention the existing major use-case of Composer of course. Architecturally, the Composer toolset has the potential to replace many of our own, homegrown Drupal specific tools and features. By sticking to the intended package architecture of PSR-4, Drupal will be able to leverage these tools in the future. We should adopt more of Composer and the wider PHP community, not less.

It is in Drupal's best interest to adopt PSR-4. And it is not like we're riskily betting on some fancy new technology with PSR-4 — no, we're adopting a new interoperability standard for PHP applications.

Now, usage of PSR-4 autoloading does not imply that there can only be PSR-4-autoloaded files within a particular package. It merely describes a means for how to load OO code of a certain namespace. In fact, it may be a mistake to limit autoloading capabilities within a package to PSR-4 only.

To my knowledge, the question of "How to load code in another namespace that is shipped with a module" hasn't been addressed for D8 yet — for example, when the Mollom module ships with Mollom's generic PHP library, or when a contributed module acts as a backport/bridge/adapter/connector for an external PHP library. The intention of such module authors is to continue to provide plug'n'play solutions; i.e., solutions for end-users. Solutions that do not require command line access or any other complex installation instructions to get a feature up and running.

All considerations combined, the most sensible architectural solution I would recommend is to support both PSR-4 and PSR-0 for modules, but only register PSR-4 namespaces by default:

  • ./src/ → PSR-4
  • ./lib/Vendor/Package/ → PSR-0
  • ./templates/stuff.twig.html
  • ./other.stuff

As an extra benefit, "src" and "lib" would actually have a concrete, self-explanatory meaning, as "lib" would contain namespaced PHP code of (external) libraries that are outside of the package namespace, as opposed to "src", which would contain the primary source code of a module.

And as yet another possible benefit, you wouldn't necessarily have to break all patches at once, but instead, modules could be individually transitioned when the time is right. — In general though, the argument of "breaking all patches" is rather irrelevant here, as history shows sufficient evidence that Drupal core development survived very well and recovered very fast from similar incidents (e.g., the /core rename). The argument is based on fear, but we should stick to technical arguments instead. The somewhat technical argument of potentially delaying the release is non-existent for aforementioned reasons: Renaming the filepaths in existing patches is a matter of seconds and can be scripted even.

Conclusion:

It is best for Drupal as an application, technology, and product to adopt PSR-4 for modules by default. There are only huge gains to take away and no risks or downsides involved.

PSR-4 presents a significant improvement for developer experience, both for programmers with object-oriented background as well as legacy developers who are transitioning to a new world order. It reduces complexity and vastly improves learnability.

It retains interoperability and commonly shared programming patterns with the rest of the PHP world, now and even more so in the future. Other architectural aspects (like sub-modules) continue to function. A package is still an independent and self-contained package, just organized more human-friendly.

As a bystander, it feels strange to see this debate in the first place. Historically, PSR-4 is the standards-compliant reincarnation of a custom DX-focused proposal in a former PSR-0 discussion for Drupal core, originally coined "PSR-NG" (or ExtendedUniversalClassloader), which was only rejected because it diverged from PSR-0. I've to admit that I have no idea how that proposal secretly paved its way to php-fig, or whether it isn't the result of a major historic coincidence and achievement in the PHP landscape; in any case, bright minds of various projects thinking alike.

Seed the standard. Grow with PSR-4.

Crell’s picture

I never expected to say this, but "What sun said".

Historical note: There's no secret. Multiple people in Drupal brought PSR-0-variants up for discussion in Drupal, and it was shot down because we want to follow standards. Multiple people, seeing the same directory-count issue when PSR-0 is combined with Composer, brought up a PSR-0 variant for discussion in FIG. FIG was the appropriate venue, and always was. It was successful there, and Drupal was one of the sponsor projects for PSR-4, precisely because there was clear interest in it from Drupal developers.

Constructive collaboration wins over emotional isolationism. Welcome to modern PHP.

Dries’s picture

I was under the impression that PSR-Dries would be PSR-0, no hack required. I'll have to dig in a bit more to understand.

I don't understand what is wrong with mixing OOP and non-OOP files in one directory. It's quite common in other projects to put resources in your classpath. I guess this comes down to personal taste. Personally I think it is a bit more future-proof, as I can see Drupal 9 using OOP only.

Crell’s picture

Dries: If the directory root of the namespace tree is outside of the root of the downloadable package, I would consider that a hack.

Dries’s picture

Crell: I'm confused by your comment in #283. The package would be Drupal\mymodule\Form, which is inside the downloadable package.

tim.plunkett’s picture

Dries, you said drupal-8/modules/Drupal/mymodule/Form/MyForm.php
Therefore all modules would be putting code in drupal-8/modules/Drupal, and ls -l drupal-8/modules/Drupal would be a list of modules.

So mymodule would only have Form/MyForm.php in its package, if you were to view it on something like drupalcode.org. Therefore, "the directory root of the namespace tree is outside of the root of the downloadable package"

msonnabaum’s picture

pwolanin’s picture

+1 for sun in #280, standards, and integrating with Composer.

clemens.tolboom’s picture

+1 for sun in #280, standards, and integrating with Composer.

dsnopek’s picture

If PSR-4 brings a performance improvement (per #280) that seems like an objective, non-bike-shedding advantage.

@sun: Thanks for explaining this so completely and eloquantly!

donquixote’s picture

Would it help to do a survey, to get an overview of who is in favor or against, and how strongly so?
The idea would not be a simple Yes / No poll, but something that also tells us about the "why", so the reasons for either decision can be understood and documented.

EDIT: I am saying this because the main question "how much does it hurt atm" and "how big would be the DX benefit" is at least to some degree a matter of taste.
The details should be discussed on IRC, to not generate unnecessary comments in this issue.

webchick’s picture

No, I don't think so. I think Dries needs to come back in and make a decision based on the arguments that have been presented.

pounard’s picture

If PSR-4 brings a performance improvement (per #280) that seems like an objective, non-bike-shedding advantage.

I'm not sure performance would be really relevant here. Especially if one day we can use the composer autoloader optimisation.

sun’s picture

@Dries mentioned that it would be common in Java to mix other resource files into the class path.

While that is possible, a properly set up e.g. Maven project package that is following best practices consists of this directory structure:

./pom.xml  ↔ composer.json ↔ mymodule.info.yml
./src/
./src/main/  → Package root directory
./src/main/java/
./src/main/java/org/drupal/mymodule/  → Java class path
./src/main/resources/  ← Declarations, outside class path
./src/main/setup/      ← Installer, outside class path
./src/main/webapp/     ← Public front-end, outside class path
./src/tests/
./src/tests/java/org/drupal/mymodule/ → JUnit Java class path

I don't think that the structure of Java packages is really relevant here, but I wanted to point out that Java code and non-Java code is properly separated in modern Java packages, too.

Again, I'm not saying that it isn't possible in Java, but instead, I'm pointing out that modern/best Java programming practices are following a common standard for structuring a Java package.

And that's exactly what we want to do here: Follow the intended best practice for structuring a package that uses PSR-4 autoloading.


@msonnabaum pointed out that e.g. the Symfony FOSRestBundle registers the package root directory as the PSR-0 namespace root directory of the bundle. However:

  1. Aside from a few declarative meta-data files in the package's root directory, the bundle consists of PHP files only. There are no CSS, JS, image, or other asset files, no theme template files, or any other files that would be mixed into the PHP class path.

  2. Tests are part of the regular runtime code. IIRC, wasn't it also @msonnabaum who vehemently disagreed with mixing tests into the regular runtime class path some time ago?

One of the main objections for mixing autoloaded PHP and other files in a module directory was and is the possibility of unforeseen name clashes, especially on case-insensitive filesystems (Windows).

Consider the entity system architecture: As a module developer, you put your entity type plugins into the ./Entity/ directory and they are discovered and registered automatically.

Now, as a quickly made up example, just consider that the Templates module author loves the (awesome) DX of entity types and wants to do the same.

Without a clear and clean separation of autoloaded PHP and non-PHP files, that is not possible:

./Templates/Foo.php
./templates/foo.twig.xml

The concrete example is to be disregarded. What matters are unforeseen consequences caused by a lack of clean separation.

Conclusion

While other approaches are possible, the most simple and straightforward solution that is guaranteed to not result in bad surprises is to place PSR-4 autoloaded PHP files into a ./src/ subdirectory of each module/package.

The additional separation of ./src/ for PSR-4 and ./lib/ for PSR-0 (cf. #280) might help to resolve some additional architectural challenges that haven't been addressed in D8 yet.

webchick’s picture

"The additional separation of ./src/ for PSR-4 and ./lib/ for PSR-0 (cf. #280) might help to resolve some additional architectural challenges that haven't been addressed in D8 yet."

Hm. Could you explain this a bit further? I don't quite follow. Do you mean the Mollom case, or..?

msonnabaum’s picture

Please, no more talk of PSR-4 being a performance improvement. It's no where near enough (if it even exists) to consider here.

donquixote’s picture

I think we should focus on DX here, and handle the performance gain as a side effect.

(performance side-track)
Indeed, the fastest possible implementation of PSR-4 would be faster than the same for PSR-0.

But, neither the loader proposed for Drupal nor that proposed for Composer uses the "fastest possible implementation" that I think sun has in mind. This has a number of reasons, which I am happy to discuss on IRC.

This said, there is indeed an actual performance benefit if we register our namespaces as PSR-4 instead of PSR-0 with the future Composer loader, and even more so with our proposed custom loader.

EDIT: One thing to mention is that the loader proposed (and mostly approved) for Composer does give priority to PSR-4 over PSR-0.

Some numbers: #2058867-7: Consider introducing a native ClassLoader for performance and PSR-4 - happy to explain on IRC.
(/performance side-track)

donquixote’s picture

dasjo’s picture

I support sun's reasoning and personally would find the /src /lib directory distinction more intuitive. That might of course be influenced by my former java experiences :)

As stated, mixing the namespace folders of autoloaded php classes with other procedural code and assets is very likely to confuse developers & even operating systems.

sun’s picture

re: #294 @webchick:

Yes, the ./src/ vs. ./lib/ separation referred to included libraries.

As a module/package author, I expect to have authority over (1) what code I provide and (2) how the code that I'm supplying ought to be loaded.

The application framework that my package integrates with can supply reasonable defaults; e.g., by auto-registering the ./src/ directory of my module/package as the PSR-4 namespace root for my extension.

However, my extension may supply more PHP code than just the Drupal module code itself, which needs to be autoloaded, too. This code may originate from an external vendor, and as such, presents a dependency for my extension.

As a package author, I would normally specify such a dependency in the composer.json file of my package, instructing Composer to automatically download and supply the dependency in a centrally managed /vendor directory of the application.

However, for users of my Drupal extension, (1) I cannot expect all users to have Composer, (2) I cannot expect that users have command line access to the server, and (3) I cannot expect that users understand how to use a command line in the first place. Drupal is a product, not a PHP framework. (And that is good.)

Drupal is a product, and my Drupal extension is a product, too. Installing my product must not be a barrier for users who are less technically versed. Consequently, there are two options:

  1. Is the dependent library potentially useful or used by other modules? → Depend on another module that supplies the library
  2. Is the dependent library exclusive to my module? → Ship with the library

In both cases, the module that supplies the library needs to be able to register the additionally provided code to the classloader, so it can be loaded.

For the concrete example of the Mollom module (disclaimer: which I maintained up until recently), the second case of above would apply. To get even more concrete, here is how I expect the module's file structure to look like in D8:

./src/  ← PSR-4-autoloaded files for the Drupal module (auto-registered).
./src/Entity/MollomForm

./lib/  ← PSR-N-autoloaded files for included libraries.
./lib/Mollom/Client/Client.php

./mollom.info.yml

Plus a mandatory custom hunk in mollom.info.yml (modeled after composer.json):

autoload:
  psr-0:
    Mollom\\Client: lib
# Alternatively:
# psr-4:
#   Mollom\\Client: lib/Mollom/Client

As before, please disregard the concrete example. What matters is the autoloading of PHP code that is shipped with a module but which is outside of the module's namespace.

To be clear, I intentionally do not suggest to make the 'autoload' declaration in .info files mandatory for all modules/packages (like it would be in a pure Composer-driven world), since the use-case of included external libraries is rare and would thus harm DX.

In other words, Drupal module developers should NOT have to manually specify the following in their .info files in order to get their main module integration files loaded:

autoload:
  psr-4:
    Drupal\\mymodule: src

That is certainly a reasonable default that the Drupal application framework is able to apply for all extensions, and there's no point in asking Drupal developers to specify it manually.

Conclusion

As the author of a package, I am in control of how the code I'm supplying needs to be loaded.

Multiple approaches are possible, and the application framework I'm integrating with may choose to default to PSR-4 for the main application integration files of my extension.

My package is self-contained and I am in charge of what code I supply and how that shall be loaded — sans the reasonable default being applied by the application framework.

As a result, I want the main integration files of my Drupal extension to be placed in ./src/ and be automatically registered and autoloaded by Drupal core via standards-compliant PSR-4, to avoid unnecessarily nested subdirectories and perceived complexity, to support sub-modules in packages like now, to avoid mixing PHP with non-PHP files and potential name-clashes, and because all of that makes perfect sense.

At the same time, I'm still in charge, and when I supply additional code beyond my module, I'm going to tell you, Drupal, how to load it.

jdillick’s picture

+1 @sun flexible implementation from #300

Dries’s picture

It looks like the current situation is roughly as follows:

  1. Just leave it alone, use PSR-0 everywhere: Mark Sonnabaum, Tim Plunkett, xjm, DamienMcKenna, ParisLiakos, jcisio
  2. Please use PSR-4: sun, yched, webchick, quicksketch, donquixote, Moshe, chx, effulgentsia, drifter
  3. I'm indifferent, just pick something: Crell, alexpott, catch, pounard, dsnopek, Dries

The participants in this issue appear to be split evenly on this so we have to make a decision. Given that 3 out of the 4 core committers are indifferent (including myself, I'd prefer PSR-Dries), I'll leave it up to Angie to make the final decision. This effectively means we will go with PSR-4.

Sorry it took so long but in the end we did the right thing waiting for PSR-4 to be officially adopted and for development of composer support for PSR-4 to begin.

xjm’s picture

Title: [meta] Drupal and PSR-0/PSR-4 Class Loading » [policy] Drupal and PSR-0/PSR-4 Class Loading
Status: Needs work » Fixed

Marking this issue fixed as the policy has been finalized (and re-scoping specifically as a policy issue). Discussion on the implementation should continue in #2083547: PSR-4: Putting it all together and its followups.

DamienMcKenna’s picture

@Dries: Thank you for making a decision on this.

gdd’s picture

I'm not sure how my name ended up in that list since I intentionally never commented on this issue, and was actually in the "Leave PSR-0 be" camp. Not that it would have changed anything.

msonnabaum’s picture

............................................________
....................................,.-'"...................``~.,
.............................,.-"..................................."-.,
.........................,/...............................................":,
.....................,?......................................................,
.................../...........................................................,}
................./......................................................,:`^`..}
.............../...................................................,:"........./
..............?.....__.........................................:`.........../
............./__.(....."~-,_..............................,:`........../
.........../(_...."~,_........"~,_....................,:`........_/
..........{.._$;_......"=,_......."-,_.......,.-~-,},.~";/....}
...........((.....*~_......."=-._......";,,./`..../"............../
...,,,___.`~,......"~.,....................`.....}............../
............(....`=-,,.......`........................(......;_,,-"
............/.`~,......`-...................................../
.............`~.*-,.....................................|,./.....,__
,,_..........}.>-._...................................|..............`=~-,
.....`=~-,__......`,.................................
...................`=~-,,.,...............................
................................`:,,...........................`..............__
.....................................`=-,...................,%`>--==``
........................................_..........._,-%.......`
...................................,

xjm’s picture

@heyrocker, that's my fault -- we were taking notes for an internal discussion and then it became an issue comment. :) Updated Dries' post to reflect what was actually on-issue.

Dries’s picture

Correction: I may have misrepresented catch and Alex Pott in comment #302. When I spoke to them this week, the 3 of us felt that this issue was 'major' instead of 'critical'. The comment in #302 is incorrect in that it doesn't imply they are 'indifferent'.

However, I now remember/believe both catch and Alex lean towards keeping the status quo as porting every core module to PSR-4 is going to break dozens of core patches at a time where we have to be focused on beta 1.

On that phone call we also discussed doing a survey to try and get the feedback from the larger community. We felt this was a good idea. However, other core developers I talked to after that phone call, including Crell, felt that we can't afford to wait making a decision as we're holding up module porting, documentation writing, etc.

So I'm revoking my previous comment, and will circle back with Alex, catch and Angie on this. I want to make sure to be on the same page with them.

Dries’s picture

Priority: Critical » Major
Status: Fixed » Needs work

Stay tuned!

catch’s picture

Yeah just to be clear (or not clear, but more verbose at least), I'm mostly indifferent in the abstract on PSR-0 vs. PSR-4 at this point. This is after changing my mind at least a couple of times in the past year or so.

However in terms of whether to make the change, I'd prefer to have left things as is, because I'm mostly indifferent. I feel it's a cosmetic change to DX, being made at a point where we're having lots and lots of discussions about what is and isn't exactly in and out of scope for beta/release elsewhere, which are pretty much all higher priority for me than this.

On the other hand, I'd rather we move to PSR-4, break the dozens/hundreds of patches that this breaks, and then don't have to talk about this any more, compared to another 300 comments on here. So unless there's a tonne of people who strongly object to the decision, it's OK with me that one got made - even if I wasn't completely in favour of it.

tim.plunkett’s picture

a survey to try and get the feedback from the larger community

Only 11 of the top 50 contributors were mentioned in #302. Those 39 other opinions, and the opinions of people outside of this issue, would certainly be more informative than the few here actually pushing for the change.

dsnopek’s picture

Not that it matters, but I'd consider myself in the "let's just pick something standard, but if it were solely up to me, I'd lean towards PSR-4" camp. :-)

jibran’s picture

Oh I am in top 50 contributors so PSR-0 it is.

bzrudi71’s picture

When starting to port all our modules for a handful of D7 sites in June (in the hope of an D8 API freeze in July) I was not to happy with all the PSR-0 stuff in the beginning. Now it's business as usual while hacking and I don't see any problem with that. I think as mentioned above, the PSR-4 change will have a big impact in terms of a D8 release.

I follow the issue queue for months to keep my modules compatible with the latest core API changes, but as even simple issues like "Remove unused variable X from..." sometimes need re-roll because they don't get committed "just in time" I wonder what will happen with all the 90% RTBC patches in queue after the PSR-4 switch ;-)

To be true, broken PostgreSQL install and other "little" annoying things seem much more important for me as "just a Drupal Developer" in terms of DX.

Best Regards
bzrudi71

amateescu’s picture

I see this got down to counting, so I'll say that I'm fine with PSR-4 and rerolling all my patches.

webchick’s picture

I think it was less about counting directly, more to reflect that opinions are pretty divided, and there are top contributors on all 3 sides.

webchick’s picture

Also, just want to point out that though I'm violently in support of PSR-4, I'm also violently in support of fixing DX issues across the board. I'm more than happy to work with folks who feel other, more important DX issues are not being looked at and discussing whether we want to make them beta blockers.

pounard’s picture

I don't think that PSR-4 is really a DX improvement, while fixing PostgreSQL sure is! :) A lot of stuff are real DX improvements, improving and simplifying API's are, while just moving files arround really isn't. Switching to such complex OOP design most people will probably use a modern IDE, in which case opening files will never happen again for them (or almost never happen). I did some years of java a few years ago, and folder structure is oftenly way stricter and deeper than that (in my memories) and it never hurted me, as it didn't seem to hurt anyone studying it beside me, neither hurting anyone working with me after that, then I did work a long time with ZF1, full PSR-0, never got hurt too, neither none of my colleagues; Then I started Drupal which was 6 years ago, and now I'm still working a lot with PSR-0, ZF2 compoennts, Symfony components, a lot of people are, and sometime still also writing code using old style PSR-0 (PEAR convention) and some other PSR-0 libraries, and still doesn't hurt me as it doesn't really hurt millions of PHP developers. What hurts me more is when the API design is broken and I can't use the library for what I really want to do.

EDIT: My conclusion is, PSR-0 is good enough and I'm kinda glad it stays a bit longer. There is absolutely no reason on earth to "violently" support switching such amount of code to a new standard while so many really important things needs to be fixed. New doesn't mean cool, using a new standard not even completely implemented in composer seems both a too fashionable choice to be pragmatic and too dangerous while approaching a beta release of a many-years-of-development-software such as Drupal 8. Switching to the new cool stuff while reaching the end of a development phase is taking an risk and it doesn't really sound like a good idea, just because it brings "DX" where really, it is really not.

dawehner’s picture

Once you have written more than one module in Drupal 8 PSR-4 vs PSR-0 really does not matter anymore. You create your two directories right from the start and you are good with it. There is, at least from my point of view, no long term DX difference between the two opportunities, so from there on, it is more about the initial learning curve. Creating two folders/press two times more tab/press two times your mouse don't make a big difference.

For people which are familiar with OOP from other places, this seems to be a real small detail especially if many other systems already have the concept of mapping folders to namespaces.
The remaining question for me is: What do people coming from a pure drupal background think about it. For them the PSR-0 mapping seems to be more scarifying though more self-explaining at the same time. Given that PSR-0 seems to be a few advantages.

donquixote’s picture

@dawehner:
The issue summary, "How you are affected depends on the tools you are using", has a list of various motoric and mental overhead caused by the deeper directories.

Obviously these affect some people more than others. They are certainly all survivable, and you can develop habits to minimize the effect.

They all apply on a "micro" level: The individual extra action required is small, but it adds up through repetition. Like keyboard shortcuts in an RTS game. If you have 300 APM (actions per minute), and 100 of them are wasted on meaningless key strokes, your effective APM drops by 1/3.
Of course it is not as bad with PSR-0, because you usually spend far more time working in a specific file than navigating around.

An action where this is most perceivable is if I browse e.g. plugin classes of "node" on github, and then want to browse to the sibling plugin folder in the "user" module. With PSR-0 this means 2 more steps up, and 2 more steps down, that makes +4 page loads.
Or you could hack the url, but that would still mean 2 places in the url to replace "node" with "user", instead of just 1 place.

especially if many other systems already have the concept of mapping folders to namespaces.

msonnabaum mentioned the same on IRC. Do these other developers not suffer?
Imo, this "suffering" is all relative.
Generations before us had to dip bird feathers in ink pots for writing. Did they suffer? Probably not, if you ask them.
The PHP community seem to have discovered the ink pen with to replace the feather. Maybe this time, as rare as it happens, we are actually ahead of the others?

Btw I did had a look at a few projects in Python and Ruby and Java on github, which had the filesystem path = complete namespace path equivalent to PSR-0. Some of them looked ok, but their namespace hierarchy was not as deep anyway. Some of them (Java) really did have a deep directory structure, and - surprise - I found them just as disgusting as in PHP. And these were only single packages, no project trees with vendor subfolders.

donquixote’s picture

I just drafted a documentation page to explain (proposed) PSR-4 in Drupal.

Regarding the supposed conflict of PSR-0 for core vs PSR-4 for modules:
Imo, the narrative should be "PSR-4 everywhere", just with the base directories adjusted to reflect the current folder structure in core, so the files in core/lib/ can all remain where they are.

The article is still a bit lengthy due to the examples.
Further improvement of this page should rather be discussed on IRC.

(And btw, a lot of this could actually be reused to improve the doc page about PSR-0)

Damien Tournoud’s picture

For the record, I'm for PSR-4 everywhere.

If we want people to adopt Drupal 8 (and not move to any of the number of cooler alternatives), we need to reduce its perceived complexity.

Reducing the directory structure is a cheap way of doing that. Let's just go for it.

drifter’s picture

Look, I'm no core developer. But every time I see things named like (Drupal 7):

class="views-field views-field-field-project"

or

field_data_field_body.field_body_value

I feel Drupal has always went for verbosity, "correctness", to avoid namespace collisions, and not so much for readability or understandability. Just looking at these make me feel that this is a system designed for machines, not for me as a human being. I believe a developer's time is very valuable. Every step we can do to make our system look less foreign, less "machine like" is a win. I feel the same way about PSR-4: removing those two empty levels of directories in cases such as core/modules/field/lib/Drupal/field/Entity/Field.php just feels more sane and friendly to me as a non-core developer.

donquixote’s picture

To anyone who is scared of rerolling patches:
git rebase origin/8.x; php core/scripts/switch-psr4.sh
works pretty ok!
#2083547-139: PSR-4: Putting it all together
(I just changed the switch-psr4.sh script to make this possible)

donquixote’s picture

Issue summary: View changes

Issue summary update:

  • Link to new doc pages for PSR-0 and PSR-4, to make the comparison easier.
  • Link to updated tag on github.
  • Removed old stuff, still available in the revisions. I think this was absolutely necessary.

The pros/cons section still needs some work.

donquixote’s picture

Issue summary: View changes

Issue summary: Improve the pros/cons section. Distinguish entrance level, casual development, and advanced development.
I hope this helps to understand what we are actually talking about.

Imo the most relevant differences are in the "casual" category, where PSR-4 has some productivity benefits (how significant, can be argued).
These productivity benefits might fade away, if you become better with your tools and shortcuts.

To really evaluate this, we would need to ask some of these "casual" developer people, who I suspect are underrepresented in this thread.
(I would count myself for some things I do, but I am certainly not a representative section)

cosmicdreams’s picture

To me this decision is all about the direction of the larger PHP community. I consider the following questions:

When I hire new developers, what knowledge will they come with?
When I use external tools, what standard will they use?
When I activate PHP developers on a project, what will they expect to find?

In answering those questions myself having a standard such as PSR-0 or PSR-4 is a huge step forward toward providing non-Drupal specialist PHP developers something they recognize. Perhaps PSR-4 wins out in this test because Composer prefers it.

pounard’s picture

As of now PSR-4 is just new, not prefered. For what it worth it has absolute no use to just talk about PSR-4 since core/lib is still a huge tree in which PSR-4 makes no sense at all, so both PSR-0 and PSR-4 will always live even if modules switch to PSR-4.

Crell’s picture

Actually using the PSR-4 flavored autoloader for /core/lib is entirely possible, and perhaps preferable; just specify a prefix of ''. That would have no significant effect other than being consistent in how underscores in class names are handled. (PSR-0, they mean directory. PSR-4, they mean nothing.)

Still, having a decision and sticking to it is the most important thing right now so that people can start migrating modules to D8 over their Christmas holiday when they want to get away from annoying relatives.

xjm’s picture

In answering those questions myself having a standard such as PSR-0 or PSR-4 is a huge step forward toward providing non-Drupal specialist PHP developers something they recognize. Perhaps PSR-4 wins out in this test because Composer prefers it.

Um, what?

  • Composer supports PSR-0.
  • Composer does not yet support PSR-4, but will soon.
  • Composer does not "prefer" anything; it is a tool. If the developers behind it have an interest in PSR-4 or even a preference for it, that certainly doesn't mean they're going to remove support for PSR-0 which is used in thousands of projects.
  • Reiterating: PSR-0 is already used in thousands of projects. I've looked in PSR-0 projects myself, browsing randomly. I opened the lid and recognized exactly what I see inside a Drupal 8 module. (It was pretty great, actually.)
  • PSR-4 is not used in anything yet, but folks hope it will be because developers from numerous projects voted in favor of creating the standard. Creating that standard does not remove PSR-0 as a standard.

Also, a common misconception came up in IRC last night, which I think it's worth correcting. PSR-4 does not shorten your namespaces. Our PSR-4 implementation only reduces the number of folders you create and navigate (if you navigate by folders...) in modules using PSR-4, without actually affecting the verbosity of the code at all.

FWIW, if you're looking for pros and cons, webchick and I drafted this last week before the maintainer... misunderstanding. It should be pretty NPOV since webchick and I coauthored it, and it is as clear, concise, and rhetoric-free as we could manage.
https://docs.google.com/a/acquia.com/document/d/1ctohNXmFKvTd7dNSo67K0LD...

I still think the biggest con is that we're discussing this at all or considering a disruptive change like this almost six months after API freeze. I think it's irresponsible.

Damien Tournoud’s picture

API freeze is a process, that goes all the way to beta.

What would be irresponsible would be not to discuss this very cheap way of reducing the perceived complexity of Drupal 8.

dasjo’s picture

I'm a casual code contributor. Often I check out other modules by opening the web repository viewer. Especially this use case gets frustrating when you have to navigate into several folders before reaching actual code. I'm in favor of PSR-4 as it easens this process of navigating through the code base without having an IDE at hand. This might also count for most command line based operations.

dasjo’s picture

Edit: removed duplicate posting

xmacinfo’s picture

For any new developer coming to Drupal 8, or a Drupal 7 jumping to Drupal 8 development, would PSR-4 be easier to grasp, filewise? Would it be easier to read and understand code?

I hope a decision will be taken and applied to Drupal beta 1.

This issue is severely blocking Drupal 8 book authors!

webchick’s picture

joelpittet’s picture

core/modules/node/tests/Drupal/node/Tests/Plugin/views/field/NodeBulkFormTest.php
vs
core/modules/node/tests/lib/Plugin/views/field/NodeBulkFormTest.php

tim.plunkett’s picture

Er, I hope #337 is a mistake. There is no way the tests should be in the plugin directories. If so, that's a HUGE blocker.

To answer the other half of #335: PSR-4 does not affect the code at all. Literally no change will be made to the files themselves.

EDIT: The bolding of only the lib part in the second path threw me off. I did not see the /tests/ right before the lib.

donquixote’s picture

@timplunkett (#338)

Er, I hope #337 is a mistake. There is no way the tests should be in the plugin directories. If so, that's a HUGE blocker.

The idea (or what the recent patch does) is that Drupal\node\Tests\ would be mapped to tests/lib/ for PHPUnit (that is, anything that is currently in tests/Drupal/node/Tests/). So class Drupal\node\Tests\FooTest goes to tests/lib/FooTest.php.

The example in #337 is a test class in Drupal\node\Tests\Plugin\views\field\NodeBulkFormTest. So this is not "the plugin directories" but the "/Tests/Plugin/" directories.

If there is a valid technical concern in flattening the tests/ PSR-0 directories for PHPUnit (besides just "I don't like PSR-4"), then I really want to hear about it.
I think it would be stupid to flatten the main lib/ directories as PSR-4, but keep the tests/Drupal/.. as PSR-0.

EDIT: see timplunkett's own edit in #338.

donquixote’s picture

Issue summary: View changes

Update issue summary:

  • Adding a section "cost of the conversion" under "Arguments".
  • Also giving a link to back up the "Composer autoloader will give priority to PSR-4".
    (this will only temporarily hurt existing projects like Symfony, because they can easily switch to PSR-4 in composer.json without even moving any files around)
xmacinfo’s picture

Having done some comparison, I now prefer PSR-4. But the real difference between PSR-0 and PSR-4 is much smaller than I would have expected.

For that reason, I am fine with either PSR-0 or PSR-4. Any new Drupal 8 developer, coming from Drupal 7, or completely new, will learn either one quickly.

I would just love that a decision be made quite early before we hit beta 1.

sammys’s picture

My 2 cents... I feel that PSR-0 for core libraries and PSR-4 for modules/themes is a good separation.

I only have one concern with the proposal as I read it. This concern may have been raised already. If so, please understand my reluctance to sift through the mountain of posts to find it.

The concern: The current proposal does not support a module and theme having the same name. The whole point of namespacing is to allow classes to have the same name.

Even DrupalM/DrupalT prefixes would be adequate.

tim.plunkett’s picture

@sammys modules and themes cannot have the same name in D6 or D7 or D8, and that has nothing to do with namespaces.

That is unrelated to this discussion.

sammys’s picture

Thanks for responding. However, you disrespectfully skipped past the whole point of my post. It's not about what existed in previous versions of Drupal. Sure, I'm late to the party but it doesn't make what I'm writing irrelevant.

What I'm suggesting has everything to do with namespaces. I am well aware that themes and modules couldn't have the same name pre-Drupal 8 and the ONLY reason for that is to avoid a confusing contrib space. If you can give me one other decent reason for disallowing it I'll shut up about this. "Not enough time" is a shitty reason since the implementation of it is trivial.

I'm not suggesting that we allow similar names in the contrib space. That would be terrible. However, in simple deployment scenarios it does make sense to have a theme and module with the same name. The introduction of namespaces makes this possible. Using a common Drupal namespace prefix for both modules and themes makes this impossible. Using a different module and theme namespace prefix makes it flexible enough to support both causes.

This discussion is about PSR-0/PSR-4 policy is it not? Somewhere within that discussion is the issue about PSR-4 namespaces and do we have them. Next step down is defining the namespace prefix. Makes my post very related to this discussion.

dsnopek’s picture

@sammys: It's unrelated in that the change you describe could be made with or without deciding this issue. PHP namespaces are already used throughout Drupal 8, so no matter what we decide as far as PSR-0/PSR-4 (which isn't about the namespaces, just loading the actual PHP files from disk), the feature you want could be implemented. To me that means it should be discussed in a seperate issue, since both things can be decided independently of each other. I hope that helps! :-)

penyaskito’s picture

@sammys: It has nothing to do with this issue because they cannot have the same name because of definition files (.info in D6/D7, .info.yml in D8).

plach’s picture

I purposedly stayed out of this discussion until now, as I too think we have far more important stuff to focus on. Anyway, since it seems core contributors feedback is what is needed to close this issue, here is my POV:

  • I use an IDE so I don't care how many physical folders a PHP file is nested into, as usually I just type CTRL+R and its name.
  • I would be totally fine if we just sticked with PSR-0, but I understand the concerns behind the push for PSR-4, so I'd be fine with it too.
  • I realize that pushing for a standard and then not adopting it might harm our credibility in the broader PHP community.

That said, personally I think it's too late to adopt PSR-4 but I won't shed warm tears if do it (now!) :)

Berdir’s picture

What @plach said. Personally I'm fine with PSR-0, I think the improvements PSR-4 would bring are overrated and it seems to be a very large change at this point (measuring the effect it would have on other issues, number of re-rolls etc.). But I don't care enough either way to fight for or against this.

I guess it's the same for everyone that uses PhpStorm, PSR-0/4 is really irrelevant for us, it even automatically expands empty folders, so not even the two-additional-arguments matters :)

DamienMcKenna’s picture

It has been ten says since Dries first made a decision and then retracted it. It'll be 2014 in another ten days. Could we please set a deadline that if there isn't a new decision before January 1st that we with stick with PSR-0? In short, we need a decision pronto so the community can move on.

[updated to remove the sub-decision]

donquixote’s picture

FileSize
12.43 KB
9.19 KB

These are two screenshots from PhpStorm.
psr-0 in phpstorm
psr-4 in phpstorm
I find the PSR-4 one more pleasant to look at and work with.

(yes this scenario is a bit artificial, but it is not unusual that I want to browse from one psr-0 directory to another)

joachim’s picture

So we have a lot of people saying that PSR-0 makes working with the OO code far too painful.
And we have people saying it's hard to make this change from PSR-0 to PSR-4.

The change is a one-off thing. Once it's done it's over.
The pain of PSR-0 is the gift that keeps on giving.

Crell’s picture

The pain of this issue is the gift that gives on giving. It's now passed into "smoking crater" status, IMO. As before, we need a decision, it has to come from Dries, and we need it about, oh, 3 months ago.

I marginally prefer PSR-4, largely for diplomatic reasons. I can live with PSR-0 and won't be too disappointed, though.

What I am very disappointed in is the process in this issue. Dries has twice said "let's do PSR-4". Yet we're still talking about it, because he's then waffling. We already know issue threads make crappy survey tools. All of the arguments that can be made have been made, on every possible side. Three times.

I will move from "disappointed" to "utterly disgusted" if we end up on PSR-0 on the grounds of "well we should have done PSR-4, but we hemmed and hawed too long so now we can't do it anymore". (Remember, Dries originally said to go PSR-4 in June.) Not because I dislike PSR-0, but because it will be a complete and utter failure of our development process by the core team, in particular core leadership.

Dries: As a Christmas gift to Drupal pick a PSR, stick to it, mark this issue Fixed, and disable comments on it. This thread must die.

If your name is not Dries, please stop posting in this thread. You are not saying anything new.

msonnabaum’s picture

I guess it's the same for everyone that uses PhpStorm, PSR-0/4 is really irrelevant for us

Just wanted to point out that this isn't an IDE argument at all. Any decent editor should support fuzzy-file-finding. I use it in vim all the time.

Anonymous’s picture

If PSR-0 will stay I swear a kitten will die! (I'm a dog person so it won't be that hard for me)

pounard’s picture

There's too many kitten in the world anyway.

chx’s picture

I have written a PSR-0 checker in awk and reworking it in PHP and adding it to devel and drush so people can easily find their non-PSR-0 compliant PHP files. This will help somewhat. I still think a) in theory PSR-4 is better b) we have better things to do. So, in anticipation of sanity prevailing and PSR-4 postponed , I am working on making the DX better.

fgm’s picture

Issue summary: View changes

Updated issue summary now that PSR-4 support has been merged into Composer.

effulgentsia’s picture

Issue summary: View changes

Thanks. I just changed it from Dec 2 to Jan 2 :)

Does this mean we need the latest patch here to be rerolled to have less Drupal custom stuff?

fgm’s picture

One thing I think has not been mentioned has been an experience I had this weekend, after converting a (small-ish) project from PSR-0 to PSR-4:

  • yes, it's nice to see the two levels of directories go away, but then it becomes kind of awkward to find namespaced code in folders not directly related to the namespace in question. This may not be much of a problem with D8, though, because the name of the module is still there one level up from the classes sub-hierarchy
  • it is also possible to use PSR-4 and keep the 2 levels of folders from PSR-0, as long as the package base directory is declared to be there, and not at the module/package level.

(tl;dr: more details on my blog http://blog.riff.org/2014_01_04_psr4_really )

DamienMcKenna’s picture

Issue tags: +beta target

Lets make this a beta blocker - if this isn't decided and fixed before beta1 lets just stick with what we have today in PSR-0. Can everyone at least agree to this?

pounard’s picture

@DamienMcKenna I agree.

pounard’s picture

There's downside in forcing PSR-4 in module: we then can't include a PSR-0 library non-Drupal namespaced in it and expect Drupal to be able to autoload it.

joachim’s picture

> if this isn't decided

That means we can just sleepwalk into a decision.

I'm with #352 -- we need a actual decision.

Dries’s picture

Status: Needs work » Fixed

Hi everyone. Back now from winter break.

I had an opportunity to sync back up with Alex, Catch, and Angie this week. We once again found ourselves in the position where while one core maintainer (Angie) felt extremely strong about doing this change, the others of us felt pretty "meh" about it, including myself. Because none of the core maintainers felt like blocking this from going in, we decided to defer to Angie on this one. Therefore, the decision is to go ahead and do PSR-4 for modules, especially since PSR-4 is now natively supported by Composer.

However, all of us are very concerned about destabilizing beta at this point, especially when we have had such good traction on committing beta blockers in the previous weeks. Therefore, we decided to postpone making the change to the actual module directories themselves until we are only within a couple of beta blockers before beta 1. (Ideally, this would be by mid-March so that the Drupal Dev Days Szeged sprinters could be working against a stable API.)

I hope I'm properly representing the core committers this time around. :-)

xmacinfo’s picture

I am glad that a decision as been made. Can we locked this issue?

Obviously, when ready, a new issue should be created to make the actual change to PSR-4.

quicksketch’s picture

Yay! Glad to see a decision made here as well. Props to @webchick for your determination on this one. The implementation issue for PSR-4 is currently at #2083547: PSR-4: Putting it all together.

I've locked this issue from further comments as requested.

Status: Fixed » Closed (fixed)

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

drupalshrek’s picture

Issue summary: View changes

Made headings match paragraph contents.

donquixote’s picture

Issue summary: View changes

Adding a link to the doc page, https://drupal.org/node/2156625.