You are here

Move module_invoke_*() and friends to an Extensions class

Anyone putting a breakpoint on it should be able to catch all joint points (hooks). foreach (module_implements()) should disappear. This should be possible with the &$a1= NULL construct, making it possible to pass around references.

Following an irc discussion, we came up with this possible draft for a refactored hook/extension system. Needs fleshing out.

We check whether $hook[0] == '#' and if so then we invoke everything listed in $arg0[$hook]. This results in module_invoke_all('#submit', $form, $form_state); inside form.inc. Pretty! We might want to rename module_invoke_all as a followup.

$hook needs to be able to handle an array of hooks to accomodate drupal_alter(). drupal_alter becomes a 1 line wrapper for module_invoke_all.

We change module_implements to _module_implements to discourage from using it.

On further discussion we could do more awesome by moving these into a class, let's say \Drupal\Module and then hide everything in there (module_list and module_implements and friends) and just expose invoke() and isImplemented() for the sake of node_access and the like.

We could move module_implements writing into the destructor / investigate moving to DrupalCacheArray.

OK discussed this with some length with chx and beejeebus in irc. chx is adamant that we don't make the Extensions class pluggable for simplicity- which I'd fully agree with for the module system, especially if {system} storage is moved to CMI - pluggable storage for module/theme/install profile metadata would be the only reason to make it pluggable for me but if that is handled via CMI all the better.

Module-as-class is a non-starter, for many reasons we've gone over before. However, moving more functionality into classes would help with performance and modularity. I could also see an argument for a "module metadata as class", as long as it is not expected to contain the entire module's code base.

(I've been toying in the back of my head with hooks-as-classes, inspired by an old blog post of chx's, but there's considerable registration issues I've not worked out yet.)

Aside from the contents of the class autoloading, which will happen on virtually every request, is there any actual benefit from the proposal currently in the summary? It looks like it's just moving existing functions into methods without really rethinking them.

The draft in the summary was a very quick write up based on the tail-end of an irc conversation, I just wanted it in the summary rather than buried in a link since it looked like there was misunderstanding in this issue about what it was about at all.

There is more to this than 'just moving existing functions into methods' though already. Having a class would quickly allow for resolving some of the bizarre state tracking in the module system (cf. module_list() and module_implements_write_cache()).

The original motivation for this issue was being able to debug hook invocations from one place (so not having to deal with function calls come from module_implements() + $function, module_invoke_all(), module_invoke(), node_invoke(), user_module_invoke(), drupal_alter(), bootstrap_invoke_all() or any of the other places hooks can currently be called from. We even allow the same hook to be called in different ways from different places.

Autoloading of the module system is not about avoiding loading code on every request (same as autoloading http libraries does not do that either). What it would hopefully do is allow us to lazy-initialize that system during each request though - some requests need all modules loaded before others do, so it is about timing rather than frequency. That in turn could prevent some of the chicken and egg situations we have with full bootstrap at the moment. There may well be some parts of module.inc (and especially system module and install.inc) that could properly be lazy loaded and would be rarely installed (I'm not sure enabling modules belongs in this class for example).

The current brittleness of the module system is one of my concerns with the WSCCI patch as it currently stands, as you know (either you put things in a bootstrap hook and kill performance, or you hope they get called before something else needs them).

It's also the cause of a large number of race conditions in core (some 'fixed', some not), most of which come down to module_list() and module_implements() returning completely different things at different times (incomplete class registries, incomplete theme registries etc. etc.). A patch on this issue that left that bizarre behaviour intact is unlikely to get in.

Our tracking of state in the module system is a real mess with functions like module_implements_write_cache(), moving that logic into a class lets us clean it up almost immediately. And the class is called extensions.inc for a reason - the module, theme and install profile APIs are all parallel to each other and freely depend on each other in the strangest of ways (like half of module.inc depending on functions in system module for a start).

So it sounds like the idea here is to move module existence declaration to a per-module class. If we're going to do that, we should look at Symfony's Bundle definitions in the kernel which do precisely that. It also includes extension points for exposing information to Dependency Injection compiler passes, which we will likely need in order to make the DI performant.

(In fact, if we start using bundles, even subclasses of them, it opens the possibility of supporting at least some Symfony bundles in Drupal... which would be really cool.)

Symfony uses magic/conventional naming for its bundles, I think. So if we did something like Drupal\$modulename\$modulenameBundle, that should work. The only caveat there would be capitalization, but since the module name is taken from the info file, not the directory, we could use that? (So a module whose info file calls it My Great Module would become Drupal\mymodule\MyGreatModuleBundle.)

Yes, yes, and YES! Been wanting to get this pattern in ever since the Kernel hit. Instead of invoking hooks on the Bundles though, we would simply pass in Drupal's Container. From there, the Bundle registers what it needs to the Container. Symfony has a compiler pass, allowing it to "compile" its configuration and save it to improve performance, but I don't see us getting that far with it. As a first pass for this issue, I'd consider the initial Bundle set up pretty good. As long as Bundles are passed the $container on instantiation, then we could then move on to do fancy stuff with it.

- discovery/retrieval etc. of the list of modules/themes - currently the drupal_rebuild_*() functions, module_list() etc.
- invoking hooks which is what this issue deals with, I've been assuming the module list would be injected into this class.

Sounds like Symfony's bundle system is dealing with the former, we don't have an issue for that, and I don't think this is it - it might be worth discussing in another issue though.

This issue and the EventDispatcher one are dealing with the latter. Moving 100% over to EventDispatcher would make at least some of the current functionality redundant, but actual hook registration is thorny, so what would be a relatively simple change here that wouldn't affect hook registration/discovery at all (except for some internal refactoring since module_implements() moves into this class) seems worth doing to try to get the hook system loaded on demand and out of an explicit bootstrap stage. Obviously we can't properly lazy load it without also sorting out the module listing stuff alongside it.

If we move to EventDispatcher entirely, then registration is quite straightforward: We use the build method of the bundle class to specify what subscribers should be attached to the event dispatcher, the event dispatcher itself is put into the dependency injection container, and then the container is dumped to disk. That's what Symfony itself does, and after talking with Fabien last week at Symfony Live I think it makes sense. (Technically Symfony full stack uses config files to populate the EventDispatcher, not raw code; that's an implementation detail.) Given that we have to use EventDispatcher at least somewhat, this is something we need to do anyway.

If we keep hooks around for another version (which speaking pragmatically I think is likely), then I agree moving the core of the module system into an object is a good thing. Not so much for lazy loading, but for the testability. Rather than having an extensions() method, I'd actually go straight to putting it in the DIC. Then we could for testing purposes swap out the default object in the DIC (which uses the same PHP global state registration mechanism as now) with one just for testing that has a hard-coded list of functions to call for a given hook. Just that simple change would greatly improve our ability to test things (we could eliminate a fair number of test modules, I think), so I support that change on that ground alone.

The code dump currently in the summary made me think it was intended to be an Extension class per module a la Bundles, which I think is what confused me. Looking at it again, I see that it's just a singleton version of module.inc. As long as that ExtensionManager class ends up in the DIC, I'm +1 on it.

While I agree I'd love to maximise the use of the event dispatcher and listeners, I'd be afraid that it might create a lot of CPU overhead. I have as example a Commerce site which calls almost 2000 times drupal_alter() per page run for example, which is a lot. If we go this way, we'd probably have to reduce the number of hooks we have (all kind of hooks) in the flavor of cached/PHP built code registration mecanisms, at least for all stuf such as plugins/components (I'm not thinking only cache backends or views handlers, but also for form API elements definition, all info array, etc...) and discourage modules to allow those alterations at normal runtime.

I like the config based definitions, it's clean and highly readable, and all it takes it merging configs altogether when enabling modules instead of doing it at runtime. We'd end up with larger registries, consuming more memory, but more static ones, consuming a lot less of CPU time on normal runtime. And if the kernel and DI patches goes as expected, we can save a lot of memory at many places and use those savings for this kind of registry mecanism.

You don't need much Symfony-fu here. Really just the DIC, which isn't even step 1. :-)

Basically, I'd start by porting modules.inc to a Modules class, which takes in its constructor a DB connection or whatever. It should contain methods that are largely a direct port of whatever is in modules.inc, with any statics converted to object properties (not class properties). It may be hard to unit test since by design it relies on global state, but at least you can get a sense for how that would work. Post that here and we'll see where you get to. :-)

Jumping from #1774388: Unit Tests Remastered™ #14, if I understand the raison d'être of this issue is to move from a Template Method to a Strategy pattern solution with Dependency Injection in mind for hook system ?

This would allow greater encapsulation and more testable unit of source code.

Ok, this is as far as I got with this this weekend - I only tackled moving system_list() and related functions, i.e. leaving everything to do with hooks alone. Maybe I am slightly hijacking this issue - my main priority is allowing us to not bootstrap beyond configuration before instantiating the kernel. In this patch I have a Drupal\Core\Extension\Modules class (which I actually think should be renamed to just Drupal\Core\System but naming can wait...). We instantiate it in DrupalKernel and then inject it into the brand new DIC as a synthetic service.

It took a stupidly long time for me to get this to work for WebTests, which is why I only got as far as I did. Maybe dealing with the invoke/implements stuff won't be as scary as I'm imagining, I just haven't gotten to it yet...

I think we'll need two services - one which manages the lists of extensions (modules + themes), and one which handles hook invocation (which can have the former injected into it). Maybe not but it may not be possible to do the one without the other so I don't think it's a hijack anyway :)

That will solve some, raise new concerns but I am a little worried how this got steered from " Allow debugging all multiple-variable-function-call at one place." let's keep that as a very major concern at least in a followup it really needs to happen.

The Creating default object from empty value, Undefined property: stdClass::$filename, Undefined property: stdClass::$parsed_info, Undefined property: stdClass::$name, Undefined property: stdClass::$name family of notices usually arise when you have a profile mismatch in what drupal_get_profile sees and what is in config('system.modules')->get().

usort(): Array was modified by the user comparison function can easily mean that an error was thrown. It doesnt mean the array was actually was changed.

Durr... I had only tested the installer with an existing settings.php :-/ Totally forgot about this problem, which Sun had talked about in the other issue:

The Extensions service issue primarily needs to combat the installer and tests [which share the same condition: There is no Drupal before Drupal.], and for that, it needs to introduce a separate InstallerExtensions or FixedExtensions service that is used instead of the regular one and which doesn't attempt to query or store anything anywhere.

OK, here's a fairly crude abstraction of the ExtensionHandler stuff into an interface and two implementations, one for the installer that never looks to the db, and one for regular use. Although it is crude, I'm not sure there'd be much point in refining it as the installer is going to get rewritten anyway. Testbot should get a bit further with this one...

It moves most of the functions out of module.inc and into a Drupal\Core\ExtensionHandler class.

It overrides the Kernel class's boot() method in DrupalKernel, where it instantiates an ExtensionHandler for use when registering bundles (to check which modules are enabled that might be providing bundles). It then registers this ExtensionHandler as a synthetic service to the DI container it builds so that it can be reused throughout the request.

In index.php we now only bootstrap to DRUPAL_BOOTSTRAP_CONFIGURATION instead of DRUPAL_BOOTSTRAP_CODE before instantiating the kernel.

+++ b/core/includes/bootstrap.inc@@ -2469,6 +2473,96 @@ function drupal_container(Container $new_container = NULL, $rebuild = FALSE) { /**+ * Returns an ExtensionHandlerInterface object.+ *+ * This is a temporary function that will instantiate a new ExtensionHandler if+ * there isn't one present in the container. This will only happen if the+ * boostrap container is being used, e.g. during installation.+ * @todo Remove this and convert all calls to it to+ * drupal_container()->get('extension_handler') once the installer is using a+ * Kernel.+ */+function drupal_extension_handler($installer = FALSE) {

I'm contractually obligated to note that the description shouldn't specify the interface but the object, but there should be a @return directive that specifies the interface. *duck*

+++ b/core/includes/update.inc@@ -408,10 +408,10 @@ function update_module_enable(array $modules) {- // system_list_reset() is in module.inc but that would only be available+ // drupal_extension_handler()->systemListReset() is in module.inc but that would only be available

Is it in module.inc? Hard to tell from the patch but it looks like it's in bootstrap.inc.

+++ b/core/lib/Drupal/Core/DrupalKernel.php@@ -36,6 +49,21 @@ public function init() { /**+ * Overrides Kernel::boot().+ */+ public function boot() {+ // Instantiate an ExtensionHandler which the Kernel itself needs in order to+ // find out which modules are enabled. In the buildContainer() method we+ // register this and the database connection we pass to it as synthetic+ // services to the container so that they do not need to be instantiated+ // over again.+ $this->connection = Database::getConnection();+ $this->extension_handler = new ExtensionHandler($this->connection, CacheFactory::get('cache'), CacheFactory::get('bootstrap'));+ parent::boot();+ drupal_bootstrap(DRUPAL_BOOTSTRAP_CODE);+ }+

Just an FYI, there are something like a half dozen patches in the queue right now that would break this. :-)

CacheFactory may not survive http://drupal.org/node/1764474. I think that issue is also trying to move database.info into the DIC, and then reference it for instantiating the database.

And of course there's http://drupal.org/node/1608842, which just went RTBC again as I was writing this and eliminates the database need here. So, all in this is clever but is the main place we'll need to chase head, methinks. Hopefully it gets easier with each patch, not harder...

Is this a direct port of existing code? Because it looks like it replicates existing design flaws, too. Folding multiple operations (load bootstrap module list and non-bootstrap list) into the same operation was never a good idea, so we shouldn't keep doing it if we don't have to.

The drupal_load() function call should get turned into a method, too, I think.

The dependency parsing should get moved to an object, too, if it isn't already. Probably a follow-up, as that's likely non-trivial.

+++ b/core/lib/Drupal/Core/ExtensionHandler.php@@ -0,0 +1,563 @@+ public function moduleImplementsWriteCache() {+ // Check whether we need to write the cache. We do not want to cache hooks+ // which are only invoked on HTTP POST requests since these do not need to be+ // optimized as tightly, and not doing so keeps the cache entry smaller.+ if (isset($this->implementations['#write_cache']) && ($_SERVER['REQUEST_METHOD'] == 'GET' || $_SERVER['REQUEST_METHOD'] == 'HEAD')) {+ unset($this->implementations['#write_cache']);+ $this->bootstrap_cache->set('module_implements', $this->implementations);+ }+ }

Is that really useful that the extension handler is a property of the kernel? Can it be registered directly into the DIC? It seems it implies lots of static still being in here. Just for discussion, I linked this patch into another issue: https://drupal.org/files/modules_install_list-2-do_not_test.patch which may propose some bits of alternative solution to get rid of all that statics. Because you hardcode database and cache stuff into the kernel it makes it harder to boot it earlier in the request and so harder to get rid of old school procedural bootstrap: ideally -and I think this can be achieved without sweting too much- the kernel can boot right after the autoloader has been loaded: the only chicken and egg problem being the module list.

This can be a follow up, but I don't like those statics introduced in the Kernel.

As I promised, I did a reroll of the patch after the system removal. Only some of Crell's concerns are addressed. Also, there's a state() call now in systemList which is not injected but I couldn't figure that one out. Drupal\Core\KeyValueStore\DatabaseStorage is ripe for injection now 'cos I converted it to use $this->connection instead of db functions, I just dont know what to inject into in the bootstrap container. That might need to wait until we remove the bootstrap container?

I am not 100% this will pass, but it installs and passes a few tests, so let's hope.

I can continue tomorrow, if someone else wants to continue, all I could reproduce was an exception in system_get_info() saying Undefined index: testing which means on test installs the info cache is not wiped properly. As system_info is cleared in systemList (and that's the correct cid indeed) I am at a bit of a loss what happened. I can't find any other error much less a fatal.

That worked, now real errors remain. The only change between #59 and #64 is that when we reset loadAll, we no longer rebuild immediately ( I could add an interdiff but that'd be hard to understand, an if is changed to an elseif, that's it).

Just wanted to post an update here because I'd intended to have this rerolled by now - I've been working on it, but some of the innocuous-seeming comments in Crell's last review amount to some serious overhaul :-P And now the installer is of course giving me grief.
Anyway, I'll be back at it tonight and will hopefully be able to produce a new patch.

There could be one class per hook signature (number of by-ref and by-value arguments, and possibly the type of return value), and then have one instance per hook. Of course, we can only take full advantage of this if we actually know the hook signature.

Currently the only benefit I see is breaking a big class into smaller ones - which can already be a good thing for itself.
It might also open up some possibility for performance gain, by eliminating some if/else, array merge, func_get_args(), by replacing call_user_func_array() with $function(), etc. This would need further investigation, obviously.

don: I don't think that buys us enough for the amount of work it would take. If someone wants to use a more powerful dispatching/notification system, they can use EventDispatcher. For now, we're just trying to clean up code and let is lazy-load, and put the caches in the right place so that we can unbork bootstrap. Heavier refactoring of hooks is, I think, unnecessary work with EventDispatcher already available. Let's stick to bootstrap unborking for now, when we are 6 weeks from feature freeze. :-(

We shouldn't be using $_SERVER. That information is provided in the Request object. Which means... ugh, does that mean extension handler depends on the request??

I'm a bit concerned that the Extension class is so huge. That feels wrong, and I think we still need to split modules from themes in this regard. However, I'm comfortable making that a follow-up at this point.

Some observations: [20:35:01] Review host: (drupaltestbot659-mysql). Checking tailing the Apache error log on 659 shows no errors between 20:01:51 and 22:00:21 and the mysql error is not available. Will try to get hold of that.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined@@ -36,6 +49,22 @@ public function init() { /**+ * Overrides Kernel::boot().+ */+ public function boot() {+ // Instantiate an ExtensionHandler which the Kernel itself needs in order to+ // find out which modules are enabled. In the buildContainer() method we+ // register this and the database connection we pass to it as synthetic+ // services to the container so that they do not need to be instantiated+ // over again.+ $this->keyValue = new KeyValueFactory();+ $class_loader = drupal_classloader();+ $this->extensionHandler = new ExtensionHandler($this->keyValue, CacheFactory::get('cache'), CacheFactory::get('bootstrap'), $class_loader);+ parent::boot();+ drupal_bootstrap(DRUPAL_BOOTSTRAP_CODE);

Comment talks about database connection but it's actually creating a keyvaluefactory? I assume the database connection will only happen within that, once accessed?

Also wondering how that will play together with having the database in the DIC too, we'll see about that :)

Because my issue where I'm working on that does inject the database info as an argument to Database() and actually instantiates that, instead of parsing global variables.

I guess that together with HttpCache wrapping the Kernel, wie can still avoid connecting to the database completely if we have non-database cache/state backends? Considering that cache should live in the container itself too.

Anyway, I finally got this working with a compiled DIC. I know the patch won't be green, because there are 2 fails that I already know about, not to mention the random fails I haven't figured out yet, but just wanted to see if testbot has any other surprises for me...

Ugh, apart from two of the DrupalKernelTest fails, I haven't found any others that I can reliably reproduce. And I still don't understand the problem with the DrupalKernelTest fails. In the meantime I think that moving the dumping of the container to after we bootstrap code might help.
I guess I'm running into the race conditions that sun keeps talking about :-/

Will try to reproduce when I find the time. Did you run it in the UI or with run-tests? Maybe it doesn't work with run-tests.sh because there's no global kernel, I've had problems with that too in other issues.

I can confirm that I can reproduce these random failures when running run-tests.sh with a concurrency of 8 but I haven't gotten much further.

I'm not sure how exactly we're trying to delete these service container php storage directories but I guess it might be that when we running into some sort of unexpected state when multiple instances are dumping containers?

w00t! #1784312: Stop doing so much pre-kernel bootstrapping just got in - that contained the whole unborking the bootstrap piece that I originally started working on in this issue. So now this issue can just focus on the ExtensionHandler, will try to get a new patch up today.

@katbailey if you don't want to send a patch to the bot, use do-not-test.patch, otherwise, all patches will be sent to the bot once the issue is set to needs review :)

Had a quick look through it, nice that this can now actually focus on what it's supposed to do and not do 10 other things that are required first. Much easier to review :)

Just one question at the moment...

+++ b/core/includes/bootstrap.incundefined@@ -2460,6 +2473,92 @@ function drupal_container(Container $new_container = NULL) { /**+ * Retrieves the ExtensionHandler that manages the list of enabled modules.+ *+ * This is a temporary function that will instantiate a new ExtensionHandler if+ * there isn't one present in the container. This will only happen if the+ * boostrap container is being used, e.g. during installation.+ * @todo Remove this and convert all calls to it to+ * drupal_container()->get('extension_handler') once the installer and the+ * run-tests.sh script are using a Kernel.+ *+ * @return Drupal\Core\ExtensionHandlerInterface+ */+function drupal_extension_handler() {

That happend, shouldn't that mean that this function is no longer necessary?

(Not sure if you just didn't get to remove it yet or if there's a reason it's still in here).

Figured I'd at least throw up a rebased patch now that #1849700: DrupalKernel's container.modules param should contain module filenames, not full namespace paths is in. I have fixed some tests too, although testbot can't even get to the point of running tests it seems and I don't understand what's happening there. Installation works fine locally for me, as does enabling modules, and running tests using run-tests.sh. Having trouble with the module enable/disable tests, but I think it will be easy enough to figure out what's going wrong there.

@berdir:

shouldn't that mean that this function is no longer necessary

Actually I think I still need it, because we need to be able to use a minimal extension handler (one that doesn't attempt to talk to the db) in certain situations (installer, tests), although it is quite possible we could come up with a way around it. I certainly need to update the docblock at least though :-/

Basically, there is still a lot of work to do on this patch, but figuring out why testbot is choking on it is probably as important a piece as any...

We certainly are in no need of any more justification for this issue, but just as an FYI, I opened #1859684: Expand routeBuilder unit test and postponed it on this issue. We probably have a lot of other similar examples.

I'm going to bump this to critical. There's a lot of one-off event listeners cropping up in patches which could just as well be hooks if we'd got this done, and it's necessary to remove DRUPAL_BOOTSTRAP_CODE. #1860026: Remove hook_exit() for cached page requests is a good example of where we're re-implementing exactly the same functionality in two different places with two different systems.

Well, I tried to get rid of the drupal_extension_handler() wrapper function and just use drupal_container()->get('extension_handler') - installation worked fine without it, but tests exploded. I've changed the docblock, leaving a todo to figure this out.

This version of the patch also eliminates module_list() entirely.

At this point, without understanding what exactly is going wrong when testbot tries to enable simpletest module, I have no idea how to move this along, so I'm hoping someone else can help.

This function is useless and adds one level of indirection in legacy wrappers. Plus it caches something that is already referenced in the container. You probably should drop it. I might be wrong, but the sole use case I see it usefull is for downgrade (the ExtensionHandlerMinimal instance) but I think the code for which this is targeted should deal with the exception, and not the normal runtime code.

So, drupal_alter was borked - specifically for multi-hook calls (form_alter, form_FORM_ID_alter, etc) - I'm thinking that will account for a lot (a majority even?) of the fails.
That's the only change I'm making in this new patch as I am anxious to see how far it gets us towards green.
@Dave yeah, it should just be getModuleList and setModuleList - I will change that in the next reroll.

This last reroll was a bit hairy so I'm hoping I haven't broken more tests than I fixed. I also renamed getEnabledModules() to getModuleList() (and that change constitutes most of the interdiff so it'll be hard to see the other changes).

One test I am battling with is BareMinimalUpgradePathTest - it has this weird Heisenbug thing going on where if I put debug statements in certain places the test passes, but otherwise it fails because somehow the list of module filenames gets corrupted such that action module ends up with image module's filename. Fun. Anyway it only accounts for 1 fail so I guess I'll deal with all the others first and then get back to it.

Using debug during update is not a good idea; write to a file instead or use a debugger. Update AFAIK has its own error handler and I have seen it doing the weirdest things and debug works by trigger_error().

This should fix the DUTB tests and is largely thanks to chx. DUTB tests that installed system module after a bunch of other modules were enabled by the parent's setUp() method resulted in the enabled modules config getting wiped (not sure why this problem was only exposed by this patch).

Rather than *merely* being a slave to testbot, I added an actual important improvement in this latest version: got rid of module_load_all(), which currently performs 4 different functions. I replaced it with 4 different methods on the ExtensionHandler class - loadAll, loadBootstrap, isLoaded and reloadModules.

Who knows what the net effect will be, test-wise, though I did also add some fixes to the ModuleAPI and Dependency tests.

I had been ignoring the upgrade path tests for the last while because they were so difficult to get to the bottom of (the heisenbug mentioned in #132) - apparently they were still a problem in the last patch, but after rebasing and doing some minor cleanup which shouldn't really affect anything, I am getting them all passing locally. Posting a new patch to see if the problem has miraculously gone away.

We *should* now be down to just the 2 ContextualDynamicContextTest failures, which I can't for the life of me figure out but are probably due to something totally stupid, and the ThemeRegistry test, which beejeebus said he might be able to look into today (and hopefully the exception in ThemeTest is related).

The ExtensionHandler deals only with modules, so system_list() has been left in place to deal with themes.

Given that, does it make sense to still call it "ExtensionHandler"? If we're adding new terminology, it should mean something new also.

I think it could work (based on the potential for future additions of non-module things to the same system), but if so, I think it's important to make sure (in this patch) that all methods which are specific to modules have the word "module" somewhere in the method name, or otherwise things will get very very confusing. For example:

Fixes the ThemeRegistry fail and the ThemeTest exception. The slippery upgrade path test fail is back, more slippery than ever because I can't even get it to fail locally now. Also, MTimeProtectedFastFileStorageTest passes for me locally :-/

Re #148 - agreed, I will change the method names to include the word module once the patch is green.

<?php// HEADif ($type == 'module') {// Default variable processor prefix.$prefixes[] = 'template';// Add all modules so they can intervene with their own variable // processors. This allows them to provide variable processors even // if they are not the owner of the current hook.$prefixes += module_list(); }

// PATCHif ($type == 'module') {// Default variable processor prefix.$prefixes[] = 'template';// Add all modules so they can intervene with their own variable // processors. This allows them to provide variable processors even // if they are not the owner of the current hook.$prefixes += array_keys(drupal_container()->get('extension_handler')->getModuleList()); }?>

head returns an array keyed by module name, array_keys doesn't. so $prefixes[] = 'template'; causes the first module to be discarded. nice and quiet like. the fix is just:

If ExtensionHandler depends on a state k/v object, that's what it should get. ExtensionHandler doesn't need to know how to get a state object from a KeyValueFactory. If this is being injected as a dependency, we probably need a State object.

<?php/** * Class representing an ExtensionHandler that tests can use. * * Overrides all methods in the ExtensionHandler class that attempt to talk to * a database. */class ExtensionHandlerMinimal extends ExtensionHandler {?>

The comment clearly describes what this classes intention is, but the name does not. Can we come up with an intention-revealing name for this class?

Also, we're calling these classes ExtensionHandler*, yet all but one method has the "module" noun in it. This is perhaps a sign that this interface has too much responsibility or is not appropriately named. ExtensionHandler->loadAllModules() is not as clear as ModuleHandler->loadAll().

If ExtensionHandler depends on a state k/v object, that's what it should get. ExtensionHandler doesn't need to know how to get a state object from a KeyValueFactory. If this is being injected as a dependency, we probably need a State object.

Agree, that's a general problem, not just with state, but all services that come from a factory. Currently, we don't have a 'state' service, we just have a keyvalue services with a state collection. Not sure about a general solution though, for frequently used things, we can add it directly to the container, For state, it will be "solved" as a side effect of #1786490: Add caching to the state system.

We can pass state as an argument if we define it explicitly in the container. That is easily possible with factoryService + factoryMethod. And the caching issue does this already. However, it doesn't really scale because every argument needs to be defined like that in the container (e.g. config objects).

Bonus nitpick: When looking at the full context of this, you'll find that $filename is not used in the loop. This can be found a few times in the patch. Surely we could go with foreach (array_keys(...) as $module) {...} but I doubt that it will make the code any more readable.

OK, this takes care of the class/method naming issues, hopefully to everyone's satisfaction: using ModuleHandler instead of ExtensionHandler and removing "module" from most of the method names (I left getModuleList and setModuleList because meh).

Also added the 'state' service, I think it's perfectly ok to special case this particular k/v collection.

Took care of the nitpicks too.

When looking at the full context of this, you'll find that $filename is not used in the loop

Does that really matter? I'd prefer to leave it as is but if it really needs to be changed to use array_keys() I don't feel that strongly about it. Have left it as is for now.

it feels like these functions are deprecated in favor to their ExtensionHandler method equivalent. Maybe this should be stated in the respective docblocks.

Well, I haven't converted all of core to use the ExtensionHandler method equivalents because I wanted to keep the patch size down and keep it relatively easy to rebase. Not sure what the rule is about deprecating functions like this. Anyone else know?

I don't know if it matters that this leaves the array 1-based, not 0-based. I think we normally array_shift() here to avoid that. (If it works, though, I'm fine to ignore it since we'll be removing this function anyway.)

+++ b/core/lib/Drupal/Core/InstallModuleHandler.php@@ -0,0 +1,141 @@+ // Since module_hook() may needlessly try to load the include file again,+ // function_exists() is used directly here.

Comment should be updated, since I presume module_hook() is now another method here.

+++ b/core/lib/Drupal/Core/Routing/RouteBuilder.php@@ -76,11 +85,9 @@ public function rebuild() { // We need to manually call each module so that we can know which module // a given item came from.- // @todo Use an injected Extension service rather than module_list():- // http://drupal.org/node/1331486.- foreach (module_list() as $module) {+ foreach ($this->ModuleHandler->getModuleList() as $module => $filename) {

This is probably a follow-up, but this "we need to know which module said what" problem comes up a couple of times. We ought to have an invokeAll() version that does track that for us. Probably not worth doing here unless it's really easy to do, but something to think about for later.

converts module_hook into the implementsHook method in the ModuleHandler class because I had left it out inadvertently

removes getHookInfo from the interface as it is only used internally

adds a loadInclude() method BUT I have to leave the module_load_install and module_load_include functions in place because the ModuleHandler currently only deals with enabled modules, but these functions know how to load the files of disabled modules. It just needs some thinking through but I'd rather leave that for a follow-up if that's ok. Added a todo in module_load_include()

changes the parameter name in buildDependencies from $files to array $modules (and adds a better explanation of what it is) as well as changing all the local variable names in there as they were very confusing.

Aside from that, I briefly glanced over this patch. Looks relatively good. I'm not sure what the overall status is, but I can try to do a more in-depth review over the weekend and help to complete and polish it.

Also, there's one particular change contained, which I don't really like though, and it's unrelated to this issue here: The introduction of DrupalWebTestBase::$install (next to ::$modules). If that's required for this patch for some reason, then we should move it into a separate issue and discuss the changes first.

Thanks @sun - appreciate anything you can do to help move this along. I must say the idea of blocking it on another issue sickens me though - it's been so painful to chase head on and it was green just the other day.

To explain the change to DUTB, i.e. the addition of the $install property, this was necessary for scenarios where system module was being enabled in a DUTB test using $this->enableModules() in setUp() when the test's parent class had already enabled some modules in the same way. The act of installing system module wipes the system.module.enabled config (during config_install_default_config()) so the module handler loses the previously installed modules. So with this change, we can at least make sure that if system module is going to get installed, this will happen before any other module gets installed.

I can understand your wanting this particular change to be pulled out into a separate issue, but if you could accept it (or some version of it) as part of this patch, it would be great to avoid stalling the progress here.

Note: I reverted most of @katbailey's latest commit during the course of that merge, since the commit tried to work around the actual cause/problem of System module not getting installed correctly.

I'll have to look into the exceptions, which should no longer occur with the installer-services patch, so they must be caused by some legacy/BC code of the new code here. I'd recommend to get the installer-services patch in ASAP, so we can move on here. Since I already merged these two monsters, we can extract the necessary conflicts/changes from there.

I decided to spend some time reworking the ModuleHandler classes (based on some discussion with msonnabaum) - there was a lot of duplicated code between the InstallModuleHandler and the regular ModuleHandler. At first I tried to use a CacheDecorator to wrap a basic ModuleHandler with no knowledge of caching, but that seems to be fairly impossible mainly due to the way the hook implementations are incrementally discovered. I couldn't come up with a satisfactory set-up using a decorator anyway. So now I've made a CachedModuleHandler that extends the ModuleHandler, and the latter is used during installation. It tidies up the methods quite a bit.

I'm leaving my fix for the installer in place because I am very resistent to the idea of holding this up because of installer lameness.

I still haven't figured out the problem with the db backend cache test, but want to at least find out if there are any more fails now.

Why didn't you simply inject the Cache\MemoryBackend and KeyValue\MemoryStorage?

Because nobody suggested it and I didn't think of it :-P Actually I quite like having the caching logic separated out from the main logic of the module handler, but maybe your suggestion is more consistent with what we're doing elsewhere so I don't feel that strongly about it. Anyone else care to weigh in?

Anyone else reviewing this: consider we are at #195 and the patch works and does not eat babies. Is there anything that really needs to be done and can't be done in a followup? This is the one of the most important patches in bringing sanity to the module data build scenarios -- currently Drupal *only* works because we fill modules in first in system_list and themes second. You change that, it breaks. That's madness.

Just a quick note: Still working on this. The current patch contains some logic that isn't right. I've resolved it for the most part, but I'm still debugging remaining problems. Need to run now, but will follow up with a new patch tonight. Will also look into those two test failures.

Thanks for fixing the new failures! However, deciding what logic is right, I feel the need to remind you of #1784312-52: Stop doing so much pre-kernel bootstrapping. It's not just me, again, who is worried what is happening to this patch. Also, you still did not add back all the bugfixes you removed from the {system} removal patch despite I filed and assigned them to you close to half a year ago. Let's not repeat that here, okay?

I fixed the two tests. What DrupalUnitTestBase is doing probably falls under the "not right" category but that's regardless of this patch and definitely a separate issue -- how can you enable a module first and install it later? Boggles the mind.

Restored deprecated module_list() for DX and to not break dozens of current other patches.

Moved ModuleHandler into Extension\ModuleHandler.

The most noteworthy changes happened to module_enable(), module_disable(), and module_set_weight(). Those were previously operating on the current/active module list of ModuleHandler, but that wasn't exactly right. A module is only enabled when it is in the system.module:enabled configuration. A module is not inherently enabled, just because it is loaded and in the active module list. Due to various reasons (install.php, update.php, tests, etc) we need to be able to operate with a fixed module list, but still be able to enable/install a module, without a hard binding between these two lists.

This is also what previously "broke" the DrupalUnitTests. Especially for those, we actually want to operate with merely loaded (but not installed) modules most of the time. Installing modules is actually discouraged for DUTB tests - by the time of the D8 release, I hope we got rid of most of the install operations in DUTB tests. Furthermore, DUTB::enableModules() actually contained a @todo that referenced exactly this issue, since module_enable() is expected to be able to install a module with the module_handler service. And it is. :) Therefore, all of the test changes have been reverted. And instead, new test coverage for loading/enabling/disabling/weight-changing/etc of modules in DUTB has been added.

What's slightly concerning with this overall patch and change proposal is the double-housekeeping of the active module list, once in DrupalKernel (%container.modules%) and once more in ModuleHandler. While it is true that both should be the same, because ModuleHandler gets the list from DrupalKernel injected, experience tell us that a "should" in combination with "duplicated data" is guaranteed to not be true, and to break badly. As long as everything runs through module_enable(), and every other code does the same as module_enable(), that's not an issue, because it ensures to rebuild/reboot the kernel appropriately. However, that's a very bold assumption, which is invalidated in this very patch already; drupal_install_system() does NOT call into module_enable() and does NOT perform the required kernel rebuild. I did not change or correct that, since #1798732: Convert install_task, install_time and install_current_batch to use the state system will change that function to call module_enable() instead. Anyhow, long story short, we should revisit whether we can eliminate the duplicate tracking and management of the module list in a follow-up issue.

The previous two test failures in #197 were caused by new calls to module_list() in core. I must say that I consider this to be a serious DX regression:

For now, I restored it as @deprecated in order to decrease the chance for conflicting commits introducing new instances only. But personally, I don't think it makes sense to remove module_list() (just yet). I also considered to rename getModuleList() into getModuleFilenames(), and introduce a new getModuleList() that would return what module_list() returned before (i.e., module names both as keys and values). I think we should consider to do that in a follow-up issue, since the module list is needed very often (especially in contrib), and the filenames are needless, unexpected, and getting in everyone's way.

Anyway, all in all, I'm happy with this patch now. Let's see what the testbot has to say.

It sounds like we also want to discuss the proper way to name services. It seems like '_handler' is inconsistent with how we name other services. Sometimes we use nothing (e.g. 'request' for the request handler), something we use '*_manager' (e.g. paths). Sometimes we use dots, sometimes we use underscores.

Not sure where in the current core process the 'documentation gate' should be addressed. There are missing @param, @return directives and one line descriptions for class properties missing from the last patch. Should that be addressed now or be put off to some as yet follow-up issue?

In general, we've been using underscores when it takes multiple words to describe what the thing is - e.g. 'alias manager': it's not an alias, it's not just a manager, it's an alias manager, hence alias_manager (mirroring the class name AliasManager). The 'path.' prefix indicates it's a service provided by the path subsystem. So in the case of the ModuleHandler, calling the service simply 'module' would seem to fall short of describing what it actually is.

Per #223, this is named consistently with what we've been doing to date, which is inspired by Symfony's de facto conventions, and it's a discussion we already had a few months ago. If we want to rethink our overall approach to service naming, this is not the place to do it.

Currently, calling drupal_alter() when no modules implement the alter hook is nearly free, which is good, because it means modules can add alter steps liberally without worrying about performance (modules choosing to *implement* rather than *add* an alter hook need to evaluate the performance of doing so, but that places the responsibility where it belongs). Here, we're adding quite a few stack calls, which I fear will result in too much pressure to remove alter steps entirely in some places. Maybe this will get mitigated if all frequently called code is in objects with an injected module handler, though I have my doubts about that being a reality in D8.

The same concern may also apply to module_invoke_all(), module_list(), module_implements(), etc. Even without the procedural wrapper, there's still a lot of extra stack calls in doing drupal_container()->get('module_handler')->method() rather than global_function().

This could possibly be partially mitigated with a module_handler() function that statically caches the result of drupal_container()->get('module_handler')?

Profiling showed an improvement there's agreement that service name bikeshedding is a followup.

Re. #229 there always was a little cost to drupal_alter, it's still there, a little more perhaps, but then again, it's just a few function calls that are added (not many!). But we can, again look at this in a followup.

It seems like '_handler' is inconsistent with how we name other services.

The class is currently named Drupal\Core\Extension\ModuleHandler. If we want to be consistent with Drupal\Core\Path\AliasManager, then we can rename it to Drupal\Core\Extension\ModuleManager, and name the service 'extension.module_manager'. However, we don't currently have an "extension system" in the Component drop-down of the d.o. core issue queue: we can either add it, or decide it's not really its own system and therefore name the service simply 'module_manager'.

Re. #229 there always was a little cost to drupal_alter, it's still there, a little more perhaps, but then again, it's just a few function calls that are added (not many!).

Yeah, I just grepped core for where drupal_alter() is called, and it looks like the only ones where a couple extra stack calls might matter is:
- drupal_alter('url_outbound') which will likely go away with #1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence, or can get $moduleHandler/$moduleManager injected into it.
- drupal_alter('query') which if it remains, I'm assuming Drupal\Core\Database\Query\Select can get $moduleHandler/$moduleManager injected into it.
- drupal_alter('translated_menu_link'), which already has an optimization to only be called conditionally, and _menu_link_translate() will need to move into an OOP system anyway, so can get $moduleHandler/$moduleManager injected into it.
- drupal_alter('user_format_name'): eh, we might just need to bite the bullet on that one

There are probably some contrib cases of drupal_alter() happening on something that gets called very often at runtime, but saying that those cases can be optimized on a case by case basis by moving that code into a proper service is probably not so bad.

Needed a reroll after #1848998: Problems with update_module_enable() landed as that patch introduced a new call to module_list_reset().
The update function introduced by that patch worries me slightly too (I was not aware of the update_module_enable() function's existence before) but SystemUpgradePathTest is passing for me locally so I guess that means everything's hunky-dory..?

Failed: InvalidArgumentException: The entity (user_role) did not specify a controller_class. in Drupal\Core\Entity\EntityManager->getControllerClass() (line 330 of /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Entity/EntityManager.php).

Furthermore, update_access_allowed() manually loads user.module to call user_access(), which in turn circles back into user roles being converted into config entities, but not being migrated into config yet.

That idea, however, turned out to be a too tight entity reference between a text format object and user role IDs (which are equally entities now) — the user role permissions can be changed without touching the text format configuration at all, so the text format configurations contained stale information about which roles have access to which format.

Therefore, we retained, but scaled down the FilterFormat::$roles property to only have an effect upon initial creation of a new text format. This allows default configuration in installation profiles (and also tests) to declare role access within the configuration, which vastly simplifies the setup of text formats.

However, the $roles are not exported into config for existing formats. The value of $roles is completely ignored for existing formats. Therefore, there's also no point in populating the 'roles' key of text format configuration objects in the upgrade path.

Thanks for listening. :) Decide for yourself whether to put this into the Things I Didn't Want To Know™ or the Good To Know™ drawer ;)

The error_handler changes are duplicate with #1845646: error_displayable() cannot be converted from variable system safely. That's already a major task blocking another critical task, so let's just go with sun's change in this patch for now, and we can continue looking at it in another issue. The problem with setting the error handler later is we'll just be using whatever native PHP error handling until that point.

Sigh. This has probably killed everyone's favorite feature on api.drupal.org -- the links to where the hooks are invoked. I've filed#1895024: Support Core's 8.x module invoke system
in hopes that maybe we can do something else to continue to have this feature for 8.x code. If you have any implementation suggestions, please put them there.