Ship a PSR-0 compatible autoloader (for both PEAR code style and newest PHP 5.3 namespaced code). This autoloader needs to be configurable (either at runtime, or in settings.php file), and be up before the cache system (and before database).

Motivation

Today Drupal 7 core does not follow the PSR-0 standard for loading OOP code, nevertheless:

  • A lot of external dependencies, libraries do (e.g. Solr PHP client, any piece of PEAR code, components from other frameworks)
  • Some contrib modules do (even if core does not, it's not a violation of the coding standards to do it)
  • All of contrib modules that do, or need an external libraries are forced to either (oftenly both):
    • Force a dependency over another contrib module that ships a PSR-0 autoloader
    • Implement an ugly fallback wih a custom autoloader, reducing drastically autoloading performances due to the autoloader bloat in SPL autoloader stack.
  • Some dependencies (external libraries such as Predis) must exist before the cache system which forces us to either dictate ugly patches in the settings.php file to users, either taint the module code with fallbacks loaders hurting performances.
  • The number of autoload modules starts to grow insanely, giving way too much alternatives. When using multiple modules having each one one of those as a dependency, you need to enable all them and duplicate functionnality (and hurt performances by loading too much code at runtime):
  • Core autoloader does not support namespace parsing when parsing files, and fails blatlantly loading any PHP 5.3 code.
  • Core autoloader is not tied to any standard, most libraries out there now does.
  • Even if Drupal 8 is coming Drupal 7 support and lifetime is going to be long, and this feature would give great benefits to a lot of contrib modules.

Proposed solution

  • Ship core with a PSR-0 autoloader
  • Ship core with an APC based autoloader and activate it per default when APC is enabled, in order to ensure best performances
  • Write it custom, because most of packaged ones out there are PHP 5.3, but Drupal 7 is PHP 5.2
  • Regarding the PSR-0 standard, and PHP 5.2 compatibility: a PHP 5.2 compatible autoloader can support PHP 5.3 code, it does only string operations, so no questions asked about compatibility there is none to ask.
  • Provide a settings.php variable to disable APC explicitely
  • Provide a settings.php variable for registering namespaces and associated include paths
  • Provide a singleton accessor to fetch the autoloader instance, allowing modules to register or unregister namespaces at runtime
  • Chain it with the core loader, possibly prepend it, so it can be used safely before cache or database ar up
  • Add its initialization in DRUPAL_BOOTSTRAP_CONFIGURATION phase, soon enough so that everything else can use it

Current patch exemple usage

Registering a namespace in settings.php file:

// Registering Solr PHP client library (PEAR style).
$conf['classloader_namespaces']['Apache_Solr'] = '/some/include/path/solr-php-client';

// Alternative (when in webroot).
$conf['classloader_namespaces']['Apache_Solr'] = 'sites/all/libraries/solr-php-client';

// Registering Predis (PHP 5.3 newer PSR-0 style).
$conf['classloader_namespaces']['Predis'] = 'sites/all/libraries/predis/lib';

(De)Registering a namespace at runtime:

// Single namespace.
drupal_classloader_get()->registerNamespace('Apache_Solr', '/some/include/path/solr-php-client')

// Multiple namespaces.
drupal_classloader_get()->registerNamespaces(array(
  'Apache_Solr' => 'sites/all/libraries/solr-php-client',
  'Predis' => 'sites/all/libraries/predis/lib',
));

// Unregistering a namespace.
drupal_classloader_get()->unregisterNamespace('Apache_Solr', '/some/include/path/solr-php-client')

Explicit APC autoloader unregister in settings.php file:

$conf['classloader_apc_enable'] = FALSE;

Alternatives proposed in issue comments

Let contrib do the job (as it is now)

And have a minimal hardcoded solution in core for PSR-0 test classes

Pros

- Works quite OK so far
- Can be PHP 5.2 compatible (e.g., xautoload is completely PHP 5.2)
- Nothing to maintain in core

Cons

- Forces contrib modules to declare one of those as dependency
- Because of numerous fallback autoloaders proposed by all modules this cause a real performance problem
- Modules need to declare a dependency
- Possibility to be forced to use more than one of them of some sites, depending on other modules dependencies
- Some existing modules ship their own custom class loader, to avoid a dependency: spagetti code in contrib modules
- Can't be available before modules are included in bootstrap (unless you modify settings.php)
- We currently have 3 such modules, so module developers need to make a choice
- Implemeting a minimal hardcoded implementation for tests and not using it for the rest feelds like loosing a lot of time and not considering the real need

Additional notes

- Simpletest works fine without as of today and core probably will never ever use it itself

Backport from D8

Pros

- Consistency with Drupal 8
- Symfony's mature code reuse
- Ships various implementations, such as APC based one for performances

Cons

- Only works on PHP 5.3+. It won't break sites with smaller PHP, but it won't help either
- Modules that want to be PHP 5.2 compatible can't use the core class loader
- We need to decide where to put it, because D7 has a different directory structure than D8

Write a custom class loader (original suggestion)

Pros

- Autoloader will be compatible 5.2 and able to load 5.3 code (everyone's happy)
- Autoloader will be documented in settings.php file
- Easier to maintain than backporting the Symfony's class loader
- Allow optimizations to be shipped with (APC implementation for example)
- Good by to the contrib spagetti dependency and ugly fallback behaviors we can see everywhere

Cons

- Need to be maintained
- Adds it to core, needs to go throught the review process

CommentFileSizeAuthor
#3 1902894-3-psr0-d7.patch14.62 KBpounard
#1 1902894-1-psr0-d7.patch12.96 KBpounard

Comments

pounard’s picture

pounard
Primary language French
commented
Status: Active » Needs review
StatusFileSize
new 1902894-1-psr0-d7.patch12.96 KB

Patch attached. Work that needs to be done:

  • Commented defaults and documentation in setting.php
  • Find out why test bot can't install the site (local install is working here)

Status: Needs review » Needs work

The last submitted patch, 1902894-1-psr0-d7.patch, failed testing.

pounard’s picture

pounard
Primary language French
commented
Status: Needs work » Needs review
StatusFileSize
new 1902894-3-psr0-d7.patch14.62 KB

Hopfully working patch?

Added settings.php documentation.
Unified all variables names as classloader_*.
Fixed fatal error at install (wrongly named file).

pounard’s picture

pounard
Primary language French
commented
Issue summary: View changes

Added current patch sample usage.

pounard’s picture

pounard
Primary language French
commented
Issue summary: View changes

Updated code samples to latest patch names.

donquixote’s picture

donquixote commented

Write it custom, because most of packaged ones out there are PHP 5.3, but Drupal 7 is PHP 5.2

We could backport the D8 class loader and only include it if PHP version is 5.3+.
The only case where this would suck is PEAR-based libraries, which we could theoretically use on PHP 5.2, but will then only work with 5.3+.

  public function findFile($classname) {
    foreach ($this->namespaces as $namespace => $include_path) {
      if (($filename = $this->resolveFileName($classname, $namespace, $include_path))) {

This loop is what I've always criticized in D8 / Symfony2 class loader.
Imagine you register one namespace per module, per theme, or in case of tests, per disabled module!

Yes you put an APC cache on top of it, but the loop is still so unnecessary.

In xautoload I do an array lookup instead.
This does the trick, but I doubt that anyone wants to have xautoload in core. (that's just a feeling i get from core discussions)

pounard’s picture

pounard
Primary language French
commented

Hum, interresting idea, this might worth the shot testing that. Minor note, the resolveFileName() method skips at the very first instruction (if(strpos())) if the namespace does not match, which makes it being almost unsignificant. In all cases, it is necessary to be strictly compatible with the autoloader from the PSR-0 folks, because they actually attempt loading files in the global namespace with it (but we might not target this specific point because we don't care about global namespace here).

have to admit I strongly target APC enabled environments, leaving the unperformant fallback (yet still working gracefully, and that's the most important point) for others. It leaves a fully functionnal autoloader in all cases.

Note that today most modules will bring their own autoloader, which makes the loop still being existent, but in the SPL stack instead of this code in today's sites, so this loop won't do worst.

donquixote’s picture

donquixote commented

Note that today most modules will bring their own autoloader, which makes the loop still being existent

Not really. These are only the modules that actually do have PSR-0 classes. (e.g. 10 modules on a site)
I assume you want to register namespaces for all modules (and themes, would be nice), which might be e.g. 100 modules on your site.

In my own tests with classloader vs xautoload on what I consider a typical site I came to sth around 40 milliseconds (*) lost to the loop, if APC was enabled.
http://drupal.org/sandbox/donquixote/1762858
With APC enabled, the results were a bit contradictory, that part would need to be done again.

(*) the number is obviously quite arbitrary, as it depends on so many things.
Of course 40ms won't kill your site, they are perfectly survivable. It just feels stupid wasting them for no reason. Maybe I should tell that to the Symfony guys.

have to admit I strongly target APC enabled environments, leaving the unperformant fallback (yet still working gracefully, and that's the most important point) for others. It leaves a fully functionnal autoloader in all cases.

The APC cache needs to get hot before it can help. So if you have two visitors on a day, each of them gets the full 40ms.

donquixote’s picture

donquixote commented

This being said, I suspect that the community (core developers) will only consider a backported D8 class loader if anything, so the rest of this discussion is kinda obsolete.

The only question would be where we put this classloader, because D7 has a different directory structure than D8, and we don't have a dedicated place for Symfony stuff.

pounard’s picture

pounard
Primary language French
commented

The APC cache needs to get hot before it can help. So if you have two visitors on a day, each of them gets the full 40ms.

No one will optimise anything for two users a day! Aside of that, APC cache is persistent until you kill either your apache or fpm or cgi, so the second one will hit it even two days after!

In my own tests with classloader vs xautoload on what I consider a typical site I came to sth around 40 milliseconds (*) lost to the loop, if APC was enabled.

40ms for calling 100 functions seems quite impossible to me. Most namespaces will never do I/O considering the first strpos() will return if the namespace don't match.

Not really. These are only the modules that actually do have PSR-0 classes. (e.g. 10 modules on a site) I assume you want to register namespaces for all modules (and themes, would be nice), which might be e.g. 100 modules on your site.

This is actually not the goal, the goal is to register easily external dependencies and give something for modules using PSR-0, but we know that most modules don't care about this and won't use it. Most sites will care about this because you almost always need at least something such as the Solr PHP client. We're speaking about Drupal 7 not Drupal 8 here.

This being said, I suspect that the community (core developers) will only consider a backported D8 class loader if anything, so the rest of this discussion is kinda obsolete.

Symfony's class loader does the same kind of loops, I'm not sure it will be faster than this one! Even the APC implementation is the same here. I propose here the most simple patch I can for a very simple goal.

donquixote’s picture

donquixote commented

40ms for calling 100 functions seems quite impossible to me.

Nope. You need to consider that in one request 100 or 200 or more classes are being loaded, and for each you run the loop.
So you get sth like 100 x 100 = 10.000.
All this is based on estimates, and I don't even remember the exact numbers I did in the experiment..

All I'm saying is, if we already write our own class loader (which I doubt will happen) then we can be smarter than this loop.
If we backport the Symfony loader then we shall just use it as it is.

Symfony's class loader does the same kind of loops, I'm not sure it will be faster than this one!

Yes.
It is just a usual practice to rather backport something from D8 than writing new code for D7.

pounard’s picture

pounard
Primary language French
commented

Nope. You need to consider that in one request 100 or 200 or more classes are being loaded, and for each you run the loop. So you get sth like 100 x 100 = 10.000.

Oh yes indeed. You're probablty right, but look at all other frameworks, there is no magic recipe aside compiling a map based autoloader from your code.

All I'm saying is, if we already write our own class loader (which I doubt will happen) then we can be smarter than this loop. If we backport the Symfony loader then we shall just use it as it is.

I agree with the first part, optimizing this would be great! I'm really eager to see alternative algorithms, and because you actually wrote one for xautoload, I'd be glad to see a patch from you. Keep in mind that in the end I would rather keep something simple and easy to maintain than something very fast there, because the real targets are APC enabled / very large sites: small sites with slow FS or slow boxes don't really count because most will use page caching or such. Sites where every tiny bit of CPU cycle is critical will use APC, and have huge problems, a lot more serious to deal with: Drupal itself and its numerous SQL queries.

I don't agree with the second part, we cannot use Symfony's class loader as it is, because it's PHP 5.3. Drupal 7 still is PHP 5.2 compatible, using PHP 5.3 in there is not something we can consider. I'm really happy that PSR-0 original team wrote a sample implementation, it means something: it means that writing our own is definitely legal.

Let's come to an agreement or compromise here, we have three easy solutions:

  • Provide no autoloader, end of story, keep the actual mess in contrib modules.
  • Provide our own (this actual solution) and work all together to make it better.
  • Backport Symfony's class loader to make it PHP 5.2 compatible, we have two solutions here:
    • Backport it manually: long and dangerous, and most features won't be useful for us, or will never even be used by anyone. This is highly complex to maintain due to large codebase (at least 3 or 4 times bigger than this patch).
    • Use a script to convert all PHP 5.3 syntax to PHP 5.2 (highly doable I guess) but even more complex, and we are prone to pragmatic errors, which makes it a lot more dangerous than the previous solution, but a lot faster to do!

Do you have any other suggestions? I'm open to any code review or different approaches too. I desesperatly need a PSR-0 autoloader for a lot of projects, and I'm already using this patch on some of them.

donquixote’s picture

donquixote commented

Drupal 7 still is PHP 5.2 compatible, using PHP 5.3 in there is not something we can consider.

// pseudo code
if (PHP >= 5.3) {
  require_once ".../Symfony/../ClassLoader.php";
  $loader = new ClassLoader()
  $loader->register();
}

That's it.
The price: Sites running on 5.2 still need other solutions for their libraries. But those sites will still work.

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

I'm really eager to see alternative algorithms, and because you actually wrote one for xautoload, I'd be glad to see a patch from you.

The trick is to go backwards through the class name / namespace.
Pseudocode:

// example class to load
$class = 'Drupal\\my_module\\SubNamespace\\Nested\\Class';
$subdir = str_replace('\\', '/', $class);

// example namespace map
$map = array();
$map['Drupal\\my_module\\'][] = 'sites/all/modules/custom/my_module/lib';

foreach (array(
  // These things should be calculated with strrpos().
  'Drupal\\my_module\\SubNamespace\\Nested\\Class',
  'Drupal\\my_module\\SubNamespace\\Nested\\',
  'Drupal\\my_module\\SubNamespace\\',
  'Drupal\\my_module\\',
  'Drupal\\',
  '',
) as $partial) {
  if (!empty($map[$partial])) {
    // $map[$partial] will typically contain only one directory.
    foreach ($map[$partial] as $root) {
      $root = $map[$partial];
      $path = $root . ;
      if (file_exists($path)) {
        include $path;
        return;
      }
    }
  }
}

There are ways imaginable to further optimize this, but it already shows that we can avoid the loop over all namespaces.

donquixote’s picture

donquixote commented

Besides, I don't see what's the issue with adding a dependency.
I can understand that big and popular existing modules like Views or Devel want to avoid that. But if you code your own module, then what's so bad about
dependencies[] = xautoload
or
dependencies[] = classloader

Why we need two of them I never really understood, but that's a different story.

pounard’s picture

pounard
Primary language French
commented

That's it.
The price: Sites running on 5.2 still need other solutions for their libraries. But those sites will still work.

Not acceptable because 5.2 driven sites may use PSR-0 PEAR style code, and won't have any autoloader. If we ship something, it must work everywhere without exception. Either we ship it for 5.2 either we don't ship it at all. Drupal never broke requirements in a stable release, we won't start today.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: Add a generic PSR-0 autoloader in Drupal 7 core in order to solve stupid dependencies and various fallbacks everywhere » Add a generic PSR-0 autoloader to Drupal 7 to resolve dependencies
Issue tags: -contrib, -D7 feature +DX (Developer Experience), +API addition
pounard’s picture

pounard
Primary language French
commented
Issue tags: +contrib

Thanks for clarifying the issue title and tags, nevertheless the main target is contrib, and I think this tag should be kept, otherwise thank you.

pounard’s picture

pounard
Primary language French
commented
Issue summary: View changes

Update code sample (once again).

donquixote’s picture

donquixote commented
Issue summary: View changes

Mention several proposed alternatives

pounard’s picture

pounard
Primary language French
commented

Clarified the alternatives in order to be less subjective.

pounard’s picture

pounard
Primary language French
commented
Issue summary: View changes

Clarified alternatives

pounard’s picture

pounard
Primary language French
commented

Besides, I don't see what's the issue with adding a dependency.

Every dependency, especially for a such low level need, is IMHO a fail. When I see projects reaching 100+ modules my main concern is "which one can I disable right now?". Adding a dependency is hurting runtime performances, having such autoloader in core is making it part of the execution flow without any performance hit (except the one and only one added require statement for loading the autoloader).

donquixote’s picture

donquixote commented

Simpletest works fine without as of today

Simpletest sucks, if it forces me to use the files[] stuff in my contrib modules.
For all the rest I can use a custom or contrib class loading solution. But for simpletest this is not an option, because testbot will ignore PSR-0 tests.

pounard’s picture

pounard
Primary language French
commented

I both agree and disagree. I agree with you because it's indeed a pain in the ass to manipulate the files[] array. But I disagree with you because it's true that core is and contrib are working fine without PSR-0, even if it's painful to manage, it's working and modules are using it.

We want to achieve the same goal today. But I want to go a bit further and get rid of contrib dependencies: an autoloader is such a low level stuff that it should not belong to contrib to provide one.

I'd love more than anything to get all the good stuff that those modules bring into core, xautoload seems to have a lot, nevertheless the most important point before anything else is to have a PSR-0 loader, with or without additional features.

The #1693398: Allow PSR-0 test classes to be used in D7 (followups) issue is not only about adding a usable PSR-0 loader in core, but also about arbitrary namespacing all modules (e.g. Drupal\psr_0_test\Tests\ExampleTest for example) pretty much as D8 does. This isn't a bad idea, but this actually goes way beyond than just embedding a PSR-0 loader for manual external libraries registration and custom modules usage.

Both are not incompatible, and I think the previous issue should use a generic loader such as the one I'm proposing here.

pounard’s picture

pounard
Primary language French
commented
Issue summary: View changes

Various typo.

Status: Needs review » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.