Closed quicksketch closed 10 years ago
quicksketch
commented
11 years ago Here's the branch where this work is happening: https://github.com/quicksketch/backdrop/compare/efficient_registry
Because we can't load classes of disabled modules, we essentially can't run tests. We need to give tests their own .info files (or equivalent) to solve this problem, before this approach can even be considered.
mkalkbrenner
commented
11 years ago Because we can't load classes of disabled modules, we essentially can't run tests. We need to give tests their own .info files (or equivalent) to solve this problem, before this approach can even be considered.
You talk about test classes, right? In case of a test it's not a big overhead to scan *.test files for classes instead of declaring a static list. And once a test enables a module, it's classes become available.
quicksketch
commented
11 years ago In case of a test it's not a big overhead to scan *.test files for classes instead of declaring a static list. And once a test enables a module, it's classes become available.
Tests for some reason use a static getInfo() method to return information like the name, description, and group of the tests. That means that in order to load the UI for showing a list of tests, PHP has to parse and then call the static method on every test class. It has to be able to show you tests even for disabled modules. While we could run regexes on the test classes too to pull out the information, it seems like we're doing it wrong.
Just like modules having .info files, tests should be described outside of the code itself. That way we can get the list of tests without needing to register the classes with the autoloader, or enable the associated module.
I filed a new issue for this at https://github.com/backdrop/backdrop-issues/issues/81(Move information about tests to a separate module.tests.yml file), but this depends on moving .info files to YAML (https://github.com/backdrop/backdrop-issues/issues/78)
jenlampton
commented
11 years ago I really like the info hook that registers classes, and a 9% performance gain is great! I also love that we're separating the procedural code from OO code in different files, I think that's certainly a DX improvement.
I already commented in the other issue about tests, but I think that's sensible and love how we're starting to see a great separation of when yaml should be used: when the system needs information about things that may not be "on". Makes sense :)
quicksketch
commented
10 years ago Test's have now been moved to separate .info files in https://github.com/backdrop/backdrop-issues/issues/81, so we no longer need to include tests in the test registry. That unblocks this issue so we can move forward with replacing the registry to get the performance gain. w00t!
quicksketch
commented
10 years ago Okay, well now that my branch is passing most tests over in https://github.com/quicksketch/backdrop/compare/efficient_registry, I re-benchmarked this set of changes.
ab -n 500 -c 10 http://backdrop/Before patch (no APC):
Requests per second: 16.70 [#/sec] (mean)
Time per request: 598.721 [ms] (mean)After patch (no APC):
Requests per second: 16.88 [#/sec] (mean)
Time per request: 592.253 [ms] (mean)Before patch (with APC):
Requests per second: 82.75 [#/sec] (mean)
Time per request: 120.848 [ms] (mean)After patch (with APC):
Requests per second: 82.98 [#/sec] (mean)
Time per request: 120.516 [ms] (mean)As you can see, by these numbers there is no performance benefit. I've been running the numbers for hours now and I can't figure out where that 9% performance improvement came from. :( :(
Some good news is that Backdrop is still almost 10% faster than it was when we started, probably due to the removal of modules though, rather than optimizations.
But as far as I can tell, we're not going to get a measurable performance improvement with this patch. What we do get however, is a more reliable registry that doesn't require the database or caching systems. This avoids issues like this one:
Registry can get hopelessly hosed
If your registry gets corrupted in D7, you have no choice but to rebuild it manually with a script like @rfay's Registry Rebuild project.
It also removes dependencies on both the database and caching systems when autoloading classes. All of it comes at no performance difference, but requires an API change. So the question is, does the benefit outweigh the cost of the API change? Without the measurable performance difference, this has to be weighed by it's other benefits of reliability, fixing a critical D7 bug, and developer experience.
sun
commented
10 years ago I don't really get why PSR-0 and PSR-4 were put into the same bucket with the same pros/cons. As the successor of PSR-0, PSR-4 seemingly resolves all the cons of PSR-0.
The Registry™ style of auto-loading classes, as implemented in D7, was a big mistake. We had it even for functions (without classes) in the beginning, but ran into even more breakage. We should have killed the approach for classes, too.
In the original discussions about introducing PSR-0 and namespacing, @donquixote and me argued heavily for a solution that follows the benefits of PSR-0, but takes the application's modularity into account. Fast-forward 2 years to today, the new PSR-4 spec de facto standardizes the exact same proposal that we originally suggested (give or take some wording differences).
I'm aware that there's some general doubt/hate regarding PSRs here (and I certainly +1 that on PSR-0, PSR-1, and PSR-2), but when it comes to class auto-loading, PSR-4 is your best bet, as it eliminates all offending DX problems and resolves the remaining issues of PSR-0.
In PSR-4, every module directory becomes a namespace root by itself (although it would be wise to register a subdirectory); e.g.:
Drupal\node -> core/modules/node/src
Drupal\node\Tests -> core/modules/node/tests
Drupal\node\Tests\NodeAddTest -> core/modules/node/tests/NodeAddTest.phpTo clarify: The base namespace is not repeated within the target directories.
The best recommendation I can give you is to ditch the Registry entirely, and leverage PSR-4 instead. It makes the most sense for an application like Backdrop (or Drupal).
It will not only show much better performance numbers, it will also make Backdrop forward-compatible with D8 (and simplify porting efforts as soon as D8 has been migrated to PSR-4).
Final sanity note: PSR-4 is not final/approved yet. However, the spec is blocked on wording and language (nitpicking) only. The key concepts of the spec are agreed-on. In short, php-fig bureaucracy madness. It is definitely safe to consider the architectural concept of PSR-4 as a given.
quicksketch
commented
10 years ago Yeah I was all over the newer iterations of the PSR-0/4 issue, and D8 definitely should (and almost certainly will) use PSR-4 instead of PSR-0 because it allows many Drupal/[module_name] nested directories to be removed. PSR-4 is definitely an improvement, but it still has the same directory-to-namespace rule, it's just that you get to specify the base namespace to point wherever you want.
I don't think your examples line up quite right. If you registered core/modules/node/src as the directory corresponding to Drupal\node, you'd end up with:
Drupal\node -> core/modules/node/src
Drupal\node\Tests -> core/modules/node/src/Tests
Drupal\node\Tests\NodeAddTest -> core/modules/node/src/Tests/NodeAddTest.phpWe still end up with long class names and nested directories (though not as bad as PSR-0 of course), and we'd certainly need to restructure modules to match this pattern.
It will not only show much better performance numbers
This isn't for certain. PSR-0/4 has not proven to be super-fast for Drupal. The best performance is accomplished by using a static class map (as D8 is doing for things in /core) which is essentially the same thing we're doing here, it's just that we're not conforming to the PSR-4 naming convention at the same time.
it will also make Backdrop forward-compatible with D8 (and simplify porting efforts as soon as D8 has been migrated to PSR-4).
We're more concerned about the other way around, where providing an upgrade/porting path from D7 is the number 1 priority, not backporting modules from D8. Moving to a PSR-4 structure means we need to restructure large swaths of code, and contrib/custom modules would need to follow suit. With this approach, everything can stay exactly where it is while the problems of the D7 registry are removed.
sun
commented
10 years ago If you registered core/modules/node/src as the directory corresponding to Drupal\node, you'd end up with...
Just like in PSR-0, for every namespace, you can register different locations to load classes from. \Tests is a different namespace than sans \Tests.
We still end up with long class names and nested directories
You will end up with namespaces for OO code (which are a good thing, because they truly resolve some long-standing issues).
Not sure what you see, but I do not count any additional directory in this:
-core/modules/node/tests/node.test
+core/modules/node/tests/NodeAddPageTest.phpThis [performance] isn't for certain.
Circles back into Symfony, Composer, and whatnot. Ignore that. The PSR-4 spec itself can be implemented in a very fast way. If desired, even in a single (and short) procedural function.
What you need to compare is (1) plenty of file reads, (2) [full] file parsing [using insane regular expressions], (3) collection + processing, (4) SQL writes for all discovered class names, and later at runtime, (5) loading of all class name values stored in SQL, (6) into a big PHP array, (7) lookup + matching, (8) actual class loading of the registry approach.
Compare with: (1) Minimal namespace registry (single cache item), (2) single lookup in early bootstrap, (3) map + load from namespace.
Classes + filenames can change too often and are too fragile. Namespaces do not.
If I've installed module Foo, then Backdrop\Foo\WhatSoEver is supposed to work, at any time, and regardless of what bootstrap phase I'm in. Because Foo is installed.
We're more concerned about the other way around
There's nothing in D7 related to OO/classes that is worth to keep. However, if the goal is to maximize compatibility, then your only choice is to retain the registry as-is. But in case people need to touch their OO code anyway (to either register files differently, or to adapt some other code), then you can be sure that developers who wrote any OO code in the first place (e.g., tests) will be much more happy to switch from a file-based registry to a namespace-based registry, because it simplifies on all fronts (and improves performance).
As you may know, I truly respect Backdrop's aims to do things "better" (or more sanely). Based on the very same sanity perspective, all I can recommend is to implement PSR-4. It is the architectural solution that every modular/extensible CMS has craved for, including Drupal (7). If you're fast and lucky, perhaps even before Drupal 8 manages to do it (equally stuck in stubborn bureaucracy as php-fig).
donquixote
commented
10 years ago A few updates about how D8 is (hopefully) going to do it. See https://drupal.org/node/2083547 "PSR-4: Putting it all together"
Test classes: So far (with PSR-0),
There was some controversy whether (module dir)/tests/ could contain classes from other namespaces. This is why this part has gone to a separate issue.
donquixote
commented
10 years ago This [performance] isn't for certain.
Circles back into Symfony, Composer, and whatnot. Ignore that. The PSR-4 spec itself can be implemented in a very fast way. If desired, even in a single (and short) procedural function.
I want to disagree here. The registry-based class loading is a pain in the directory scanning, but this happens only on cache clear. Once the registry is built up, it is actually quite fast for an average lookup. It is comparable to Composer loader with a class map, cause that's essentially what it does. It still needs to load the list from the database, but that is a 1x effort and not too expensive (although I did not scan this part).
Now I was going to write a very detailed comparison of different algorithms for a PSR-4 loader, and in which situation they shine or not. But I realize this is far too much for this issue.
However:
While class loading is great, the real benefit of PSR-4 (or PEAR, or PSR-0) is that you get some order into your code. "One class per file" is a blessing. And having a clear mapping from the class name to the file path is another blessing.
The registry in Drupal 7, and the lack of any namespacing or prefixing rules (e.g. using the module name as a prefix for the class) was madness.
sun
commented
10 years ago The #1 use-case of classes in D7 are tests.
With PSR-4, the porting path looks like this:
diff mymodule.info mymodule.info
-files[] = tests/mymodule.test
-files[] = tests/mymodule-other.test
diff tests/mymodule.test tests/FooTest.php
rename 99% similarity
<?php
+namespace Backdrop\mymodule\Tests;
-class MyModuleFooTestCase extends DrupalWebTestCase {
+class FooTest extends DrupalWebTestCase {
diff tests/mymodule-other.test tests/OtherTest.php
rename 99% similarity
<?php
+namespace Backdrop\mymodule\Tests;
-class MyModuleOtherTestCase extends DrupalWebTestCase {
+class OtherTest extends DrupalWebTestCase {That is, because the system automatically knows that Backdrop\mymodule exists, and likewise, Simpletest automatically checks each registered namespace (i.e., Backdrop\mymodule\Tests) for available tests.
For completeness, Simpletest discovers tests in all extensions, regardless of whether they are installed or not. This enables extension authors to test the installation procedure of their modules, too. But as long as you know how to map extensions to namespaces, it is piece of cake to do that for uninstalled or disabled extensions, too.
donquixote
commented
10 years ago It also means that the class registry is manually reading all of these files off of disk and then running regular expressions to find the names of classes they contain. This is crazy!
Well.. I was thinking the same when I saw this, but: The Composer class map generator, and PHPUnit test discovery are essentially doing the same! The algorithm might be different, but they do it.
If you want to improve on this, but still allow arbitrary locations for classes - which might be useful to make the transition easier - you could do a wildcard system like
files[] = includes/**
(see xautoload)
quicksketch
commented
10 years ago For completeness, Simpletest discovers tests in all extensions, regardless of whether they are installed or not. This enables extension authors to test the installation procedure of their modules, too. But as long as you know how to map extensions to namespaces, it is piece of cake to do that for uninstalled or disabled extensions, too.
We've already moved SimpleTests out of the autoload registry in https://github.com/backdrop/backdrop-issues/issues/81. We now use .info files to declare their location and classes, though we don't have autoloading at all on them, except for the base classes provided by SimpleTest itself.
sun
commented
10 years ago We've already moved SimpleTests out of the autoload registry in [...] We now use .info files to declare their location and classes, though we don't have autoloading at all on them...
Duly noting: A separate class loading practice that is specific to tests harms DX.
The (amount of) test class names in the registry might have caused positive performance benchmark numbers elsewhere (over in #81 mayhaps), but that is just a symptom of the class registry in D7. The actual root cause is the architectural concept and performance of the class loader, which applies to more than just tests.
If you want to KISS (and I'm dead sure you want), then it is best to implement and apply a consistent and simple class loading approach across the board, that makes no difference on purpose. A class is just a class that's provided by someone → load it.
It's not only going to be more consistent; but who knows, tests might actually run faster, too.
Much more generally, starting from D6, Drupal core continued to introduce the pattern of info hook based registries in more and more areas. These registries were the primary reason for why D6 became much slower than D5 (and D7 only advanced on that pattern). If you're shooting for simplicity and performance, then you want to get rid of almost all of them. (Encompassing much more than just the class registry.)
quicksketch
commented
10 years ago These registries were the primary reason for why D6 became much slower than D5 (and D7 only advanced on that pattern).
The theme registry in particular was probably the main culprit of performance, though it also added a huge amount of functionality (e.g. supporting more than just the 5 hard-coded template files of block, page, node, comment, and box). It wasn't the use of info hooks, but rather the functionality to which they were tied that slowed down the system.
Info hooks right now are the consistency in Drupal 7. Though each info hook has separate documentation and functionality, the use of info hooks is one of the most widespread patterns in Drupal. We're not going to reduce use of info hooks in Backdrop 1.0 because it means API changes, so if we're not replacing them, the best option is to continue the pattern. This issue basically hopes to remove the problems caused by the registry with the least amount of changes. Given our Goals for 1.0, the proposed approach is significantly less disruptive than converting to a directory-based autoloader.
rudiedirkx
commented
10 years ago (I didn't read all replies. I came from #51.)
Not using any PSR or any namespaces is a waste IMO. (And by using I mean harness its advantages.) You could have many pros and few cons if you'd use namespaces and a tiny bit of config:
.info:
classes = lib
; could be `include/views` or `.` insteadautoloader.php, or wherever this function lives:
spl_autoload_register(function($class) {
$components = explode('\\', $class);
$module = array_shift($components);
$location = system_module_class_location($module); // would be sombething like `modules/feeds/lib/`
include $location . implode('/', $components) . '.php';
});It would be slightly more foolproof of course.
A small module abc could then omit classes (which would implicitly make it ., being the module's root folder) and name its class abc\SomeClass, which would live in abc/SomeClass.php.
A module like Views would have classes = lib and a class like views\handlers\field\BooleanField, which would live in views/handlers/field/BooleanField.php.
All autoloading, only when necessary, on the fly, using namespaces, no hook, no registry. Autoloading logic should be available without any other app logic (no db, no hooks, some bootstrap). Two constraints:
I like D7's files[] better than a hook_class_info() (although it being structurally, unfixably broken is bad....).
Just my 2c.
matt2000
commented
10 years ago This simply adds a new autoloading & plugin system based on ideas from Quicksketch, Rudiedirkx, and the work I did in #51. It is not yet implemented to replace the registry, but I can work on that next, if this direction is agreeable.
I went with hook_plugin_info instead of hook_class_info, because not all classes need to be registered, but I don't want to bikeshed it. I'll rename if anyone cares.
The best place to start with a review is probably by reading plugin.api.php. Then there are SimpleTests to show autoloading in-action.
quicksketch
commented
10 years ago Thanks for your work on this @matt2000.
This entire discussion is probably more appropriate over in https://github.com/backdrop/backdrop-issues/issues/41 (Decide if we need to add plugin-system to core). That issue is a tough read though, so I'll summarize as best I can in the response here.
I think the new PR in https://github.com/backdrop/backdrop/issues/98 might be after a different goal than the original issue. There are some serious difficulties around the definition of a "plugin", so explaining what the "plugin system" does exactly is important. There are 3 concepts fundamentally at play when talking about plugins in the D8 and CTools contexts:
This issue on the class registry is only about the "loading" portion. It doesn't provide either discovery or definition. For the most part, discovery and definition combined make up the concept of "plugins" (at least in D8), but in CTools-land, loading is also a concern of the the plugin system because they're not classes. However, if a plugin is a class, the loading concern can be separated and the system-wide autoloader deals with that problem.
So in your example, a plugin hook looks like this:
function hook_plugin_info() {
$plugins['the_type']['mymodule\FullyNameSpaced\MyPluginClassName'] = array(
'title' => t('My own plugin'),
'path' => 'lib/classes',
'properties' => array('something' => 'anything I want'),
);
return $plugins;
}But if we use a hook for autoloading that's separate, then you end up with this:
function hook_class_info() {
$path = drupal_get_path('module', 'mymodule');
return array(
'MyPluginClassName' => $path . '/lib/plugins/MyPluginClassName.inc',
);
}
function hook_plugin_info() {
$plugins['the_type']['MyPluginClassName'] = array(
'title' => t('My own plugin'),
'properties' => array('something' => 'anything I want'),
);
return $plugins;
}But now hook_plugin_info() is nothing but a massive list of all plugins in the whole system. This makes documentation a difficult task, because each plugin is going to have separate "properties" that need to be registered, so it'd be better if these were separate hooks that could be loaded independently for efficiency, and documented separately. So you end up with this for the second function:
function hook_THE_TYPE_info() {
$plugins['MyPluginClassName'] = array(
'title' => t('My own plugin'),
'properties' => array('something' => 'anything I want'),
);
return $plugins;
}Ultimately, this results in going right back to where we are now: individual info hooks provide the discovery and definitions and an autoloader provides the loading. There are lots of upsides to this approach:
But the largest downside is that every individual class (or "plugin", since they're classes) needs to have it's path explicitly provided by hook_class_info(). From the appearance of the PR in https://github.com/backdrop/backdrop/issues/98, it looks like that would be required anyway with this suggested approach.
So maybe I'm missing some of the suggestion here, but it looks to me like the PR isn't a full replacement for the class registry anyway, so it may have been better to discuss this in https://github.com/backdrop/backdrop-issues/issues/41, because if there's need for further discussion on the need of a plugin system, it should be over there. If this does replace the autoloading system entirely for all classes, this needs further explanation as to how it would accomplish that.
matt2000
commented
10 years ago It does handle all autoloading, and it is Not required to register all classes. You only have to register a class if you want to use a namespace hierarchy which does match your filesystem hierarchy. (You can avoid both namespacing AND registering if your class .inc files are in your module root.) See the changes to system module files for the implementation of this.
One thing I need to fix is that the info hook uses the module name as if it were a namespace, but its not intended to be such, so that could be confusing, and resolving that is next on my to do list.
All the Best, Matt Chapman On Nov 27, 2013 4:50 PM, "Nathan Haug" notifications@github.com wrote:
Thanks for your work on this @matt2000 https://github.com/matt2000.
This entire discussion is probably more appropriate over in #41https://github.com/backdrop/backdrop-issues/issues/41(Decide if we need to add plugin-system to core). That issue is a tough read though, so I'll summarize as best I can in the response here.
I think the new PR in backdrop/backdrop#98https://github.com/backdrop/backdrop/pull/98might be after a different goal than the original issue. There are some serious difficulties around the definition of a "plugin", so explaining what the "plugin system" does exactly is important. There are 3 concepts fundamentally at play when talking about plugins in the D8 and CTools contexts:
- Discovery: Finding a list of all things of a particular concept (blocks, views, etc.)
- Definition: Provide information about a particular thing, usually providing a human-readable name and meta-data.
- Loading: Given a particular thing that you already know about, load an implementation of that thing.
This issue on the class registry is only about the "loading" portion. It doesn't provide either discovery or definition. For the most part, discovery and definition combined make up the concept of "plugins" (at least in D8), but in CTools-land, loading is also a concern of the the plugin system because they're not classes. However, if a plugin is a class, the loading concern can be separated and the system-wide autoloader deals with that problem.
So in your example, a plugin hook looks like this:
function hook_plugin_info() { $plugins['the_type']['mymodule\FullyNameSpaced\MyPluginClassName'] = array( 'title' => t('My own plugin'), 'path' => 'lib/classes', 'properties' => array('something' => 'anything I want'), ); return $plugins;}
But if we use a hook for autoloading that's separate, then you end up with this:
function hook_class_info() { $path = drupal_get_path('module', 'mymodule'); return array( 'MyPluginClassName' => $path . '/lib/plugins/MyPluginClassName.inc', );} function hook_plugin_info() { $plugins['the_type']['MyPluginClassName'] = array( 'title' => t('My own plugin'), 'properties' => array('something' => 'anything I want'), ); return $plugins;}
But now hook_plugin_info() is nothing but a massive list of all plugins in the whole system. This makes documentation a difficult task, because each plugin is going to have separate "properties" that need to be registered, so it'd be better if these were separate hooks that could be loaded independently for efficiency, and documented separately. So you end up with this for the second function:
function hook_THE_TYPE_info() { $plugins['MyPluginClassName'] = array( 'title' => t('My own plugin'), 'properties' => array('something' => 'anything I want'), ); return $plugins;}
Ultimately, this results in going right back to where we are now: individual info hooks provide the discovery and definitions and an autoloader provides the loading. There are lots of upsides to this approach:
- We don't have to rearrange or rename any of our current classes.
- _info() hooks are the most common mechanism of discovery and definition in Backdrop. We don't have to change most of those either.
- _info() hooks are very fast, can be independently loaded and documented.
But the largest downside is that every individual class (or "plugin", since they're classes) needs to have it's path explicitly provided by hook_class_info(). From the appearance of the PR in backdrop/backdrop#98https://github.com/backdrop/backdrop/pull/98, it looks like that would be required anyway with this suggested approach.
So maybe I'm missing some of the suggestion here, but it looks to me like the PR isn't a full replacement for the class registry anyway, so it may have been better to discuss this in #41https://github.com/backdrop/backdrop-issues/issues/41, because if there's need for further discussion on the need of a plugin system, it should be over there. If this does replace the autoloading system entirely for all classes, this needs further explanation as to how it would accomplish that.
— Reply to this email directly or view it on GitHubhttps://github.com/backdrop/backdrop-issues/issues/77#issuecomment-29429844 .
matt2000
commented
10 years ago I misspoke above. If you want your class to be autoloaded, without registering it, you need to use your module name as the namespace, at a minimum.
Also, quicksketch's summary of #41 is vastly more valuable than the thread itself.
My definition of a plugin is in the api doc, but for the lazy: a plugin is a class with meta data that can be read separately from loading the class itself. Not all classes are plugins, but all classes can be autoloaded by following simple conventions.
Yes, separately discovering, loadind, and caching of plugin metadata is an obvious optimization; I just want to start with something that works, and provides all the intended developers facing capabilities.
All the Best, Matt Chapman On Nov 27, 2013 5:00 PM, "Matt Chapman" matt@ninjitsuweb.com wrote:
It does handle all autoloading, and it is Not required to register all classes. You only have to register a class if you want to use a namespace hierarchy which does match your filesystem hierarchy. (You can avoid both namespacing AND registering if your class .inc files are in your module root.) See the changes to system module files for the implementation of this.
One thing I need to fix is that the info hook uses the module name as if it were a namespace, but its not intended to be such, so that could be confusing, and resolving that is next on my to do list.
All the Best, Matt Chapman On Nov 27, 2013 4:50 PM, "Nathan Haug" notifications@github.com wrote:
Thanks for your work on this @matt2000 https://github.com/matt2000.
This entire discussion is probably more appropriate over in #41https://github.com/backdrop/backdrop-issues/issues/41(Decide if we need to add plugin-system to core). That issue is a tough read though, so I'll summarize as best I can in the response here.
I think the new PR in backdrop/backdrop#98https://github.com/backdrop/backdrop/pull/98might be after a different goal than the original issue. There are some serious difficulties around the definition of a "plugin", so explaining what the "plugin system" does exactly is important. There are 3 concepts fundamentally at play when talking about plugins in the D8 and CTools contexts:
- Discovery: Finding a list of all things of a particular concept (blocks, views, etc.)
- Definition: Provide information about a particular thing, usually providing a human-readable name and meta-data.
- Loading: Given a particular thing that you already know about, load an implementation of that thing.
This issue on the class registry is only about the "loading" portion. It doesn't provide either discovery or definition. For the most part, discovery and definition combined make up the concept of "plugins" (at least in D8), but in CTools-land, loading is also a concern of the the plugin system because they're not classes. However, if a plugin is a class, the loading concern can be separated and the system-wide autoloader deals with that problem.
So in your example, a plugin hook looks like this:
function hook_plugin_info() { $plugins['the_type']['mymodule\FullyNameSpaced\MyPluginClassName'] = array( 'title' => t('My own plugin'), 'path' => 'lib/classes', 'properties' => array('something' => 'anything I want'), ); return $plugins;}
But if we use a hook for autoloading that's separate, then you end up with this:
function hook_class_info() { $path = drupal_get_path('module', 'mymodule'); return array( 'MyPluginClassName' => $path . '/lib/plugins/MyPluginClassName.inc', );} function hook_plugin_info() { $plugins['the_type']['MyPluginClassName'] = array( 'title' => t('My own plugin'), 'properties' => array('something' => 'anything I want'), ); return $plugins;}
But now hook_plugin_info() is nothing but a massive list of _all_plugins in the whole system. This makes documentation a difficult task, because each plugin is going to have separate "properties" that need to be registered, so it'd be better if these were separate hooks that could be loaded independently for efficiency, and documented separately. So you end up with this for the second function:
function hook_THE_TYPE_info() { $plugins['MyPluginClassName'] = array( 'title' => t('My own plugin'), 'properties' => array('something' => 'anything I want'), ); return $plugins;}
Ultimately, this results in going right back to where we are now: individual info hooks provide the discovery and definitions and an autoloader provides the loading. There are lots of upsides to this approach:
- We don't have to rearrange or rename any of our current classes.
- _info() hooks are the most common mechanism of discovery and definition in Backdrop. We don't have to change most of those either.
- _info() hooks are very fast, can be independently loaded and documented.
But the largest downside is that every individual class (or "plugin", since they're classes) needs to have it's path explicitly provided by hook_class_info(). From the appearance of the PR in backdrop/backdrop#98https://github.com/backdrop/backdrop/pull/98, it looks like that would be required anyway with this suggested approach.
So maybe I'm missing some of the suggestion here, but it looks to me like the PR isn't a full replacement for the class registry anyway, so it may have been better to discuss this in #41https://github.com/backdrop/backdrop-issues/issues/41, because if there's need for further discussion on the need of a plugin system, it should be over there. If this does replace the autoloading system entirely for all classes, this needs further explanation as to how it would accomplish that.
— Reply to this email directly or view it on GitHubhttps://github.com/backdrop/backdrop-issues/issues/77#issuecomment-29429844 .
matt2000
commented
10 years ago Also, maybe it's not obvious, so I'll mention that plugin module is not required at this stage. If a module wants to use the plugin system, it can just declare plugin as a dependency. Autoloading ("Loading") is in system module; meta-data ("Definition") is in plugin module.
"Discovery," when something more than hook_plugin_info is needed, ought to be defined by the module which exposes a plugin interface; I don't think core can make universal decisions about how discovery can be appropriately cached, altered, etc; hook_plugin_info may meet the need, or simple namespacing may meet the need, or the module may need to define it's own info hook or method or .info file properties, or whatever.
Case in point: As I understand it, Views plugin system was never fully ported to CTools; I intend to port Views to Backdrop keeping it's existing plugins/handlers pretty much as-is.
There's no one architecture to rule them all, so we force a useful minimum that encourages conventions, but doesn't mandate them, or implement heavy plugable meta-sub-systems.
quicksketch
commented
10 years ago It's been a long time since we had any movement on this issue. With Backdrop's 1.0 goals in mind, we're really shooting for compatibility above all else. I recognize some of the benefits of a PSR-4 approach, but given our timeline and the amount of change required by such a move, it's not possible for our initial release. Back in October/November we didn't know how far we would be by this point. Considering our general state is not as progressed as far as we would want and our release is targeted for this summer, I don't think the option of renaming/rearranging all our classes is possible.
Even so, we still want to get rid of the class registry. Although it doesn't provide any performance benefit (or loss), the original proposal still seems like our only option. Now that Views is part of Backdrop core, I was able to re-run the benchmarks and confirm we still don't have any performance change, even with Views nearly doubling the number of classes in the project.
I've merged the original branch with 1.x and updated it for all the core changes. The new PR is at https://github.com/backdrop/backdrop/pull/231. I had to separate out all classes from functional code, particularly in Views, where the mingling hadn't been cleaned up yet. Otherwise attempts to load functional code ends up triggering the autoloader.
So despite the availability of directory-based alternatives to the registry, this approach still seems best for Backdrop as it:
quicksketch
commented
10 years ago I've merged in https://github.com/backdrop/backdrop/pull/231, so we now have hook_autoload_info() and no more files[] array in .info files. Despite the overall kind of "sideways step" here, I still think eliminating the database as a requirement (though cache tables may still end up getting used) and making the registry incorruptible (since it's generated every request) is a good idea. We may see about PSR-4 in the future, but considering it would certainly cause massive backwards compatibility breakage, I'm not sure it's going to be a priority for us unless this new registry causes more problems than the old one.
Drupal 8 uses a PSR-0 (hopefully soon to be PSR-4) based class registry system. This means that module directory structures and namespaces are essentially the same thing. For example, to make a new "Attachment" display in Drupal 8, it looks sort of like this:
And the code for the Attachment.php class lives in:
After switching to PSR-4, it will be:
The trouble I have with PSR-0/4 is that it almost certainly requires the use of both namespaces and "use" statements. You don't have to, you could say something like this instead:
But that's definitely much worse.
Instead of using PSR-0/4 for loading our own internal classes, I think we should enhance the current class registry to optimize it for speed and remove the abuse of the .info files.
In Drupal 7, we have .info files that include a files[] array, listing all classes that have files:
The problem with this is the list of classes has nothing to do with describing the module. It also means that the class registry is manually reading all of these files off of disk and then running regular expressions to find the names of classes they contain. This is crazy!
However, what I like about the class registry is that you're allowed to name your classes whatever you want, and organize your module however you want. If you want to put your views classes in /views or in /includes/views, you can do either. It makes simple modules easy to navigate, and allows complex modules to use nested directories for organization. Because the class is independent of the location, modules can reorganize the location of classes at any time, and it won't break any modules that depend on those classes.
So in summary
PSR-0/4 autoloader:
D7 Class registry autoloader:
What I'd like to propose is that we replace the D7 class registry with a static class map, built through the hook system. This introduces hook_autoload_info(), which has the following summary:
Proposed autoloader:
I don't find the Con of typing out the class list to be a deal breaker. The ability to only run on active modules however means that we can't load the classes of individual tests on disabled modules. That means we'll need a different way of getting info on those tests (this has now been fixed in https://github.com/backdrop/backdrop-issues/issues/81)
Syntax Changes
Taking system.module as an example.
Before (in system.info file):
After (in system.module file):
This change also requires no mixing of procedural and OO code in the same files. Previously we had a few random classes defined in places like node.module and entity.module. These classes have to be in their own files so that they don't trigger the autoloader just by extending a class. This seems like an improvement rather than a downside, IMO.
Results
UPDATE: Testing this shows no significant performance change https://github.com/backdrop/backdrop-issues/issues/77#issuecomment-26461998. So making this change needs to be based on the remaining benefits of fixing an otherwise unfixable D7 bug and DX.