Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
namespace Drupal;
class Extensions {
protected $modules = array();
protected $themes = array();
public function moduleEnable($module) {
}
public function moduleDisable($module) {
}
public function themeEnable($theme) {
}
public function themeDisable($theme) {
}
public function invoke($hook) {
}
public function alter() {
}
public function isImplemented($hook) {
}
public function moduleInfo($key) {
}
}
function extensions() {
static $extensions_instance;
if (!isset($extensions_instance)) {
$extensions_instance = new Extensions;
}
return $extensions_instance;
}
Comment | File | Size | Author |
---|---|---|---|
#249 | module.handler.249.patch | 147.87 KB | sun |
#249 | interdiff.txt | 6.01 KB | sun |
#236 | 1331486.module_handler.236.patch | 142.87 KB | katbailey |
#236 | interdiff.txt | 751 bytes | katbailey |
#215 | module.handler.215.patch | 142.13 KB | sun |
Comments
Comment #1
chx CreditAttribution: chx commentedFor those who abhor PHP debuggers (me too) the ideal outcome is that every. single. hook call can be caught by print statements inside module_invoke_all.
Do we want to nuke #foo callback arrays too? I feel a tentative yes but I need to figure out how to identify on those.
Comment #2
chx CreditAttribution: chx commentedSo
module_invoke_all('#submit', $form, $form_state);
inside form.inc. Pretty! We might want to rename module_invoke_all as a followup.Comment #3
chx CreditAttribution: chx commentedOn 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.
A tentative skeleton by Justin Randell is at http://paste.pocoo.org/show/505043/ Edit; if we move the extensions() static to drupal_static it can be reset and the constructor can reread the config and then reloading the modules is a configuration problem. So either CMI solves it or we allow for something like http://drupal.org/project/environment_modules
Comment #4
catchOK 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.
The module system as it is now is extremely fragile, see issues like #1061924: system_list() memory usage, #222109: module_list() should be rewritten as 2 functions: module_list() and module_list_bootstrap(), and #1103910: bootstrap_invoke_all() queries bootstrap modules twice (and countless others). Moving it to a class will not fix every issue, but the overall clean up effort would go a long way towards improving things, as well as the original intention here of making it easier to find out where hooks are invoked from, which is a major complaint at the moment in Drupal 7.
So I'm bumping priority of this to 'major', it's a refactoring task I don't think we can afford to skip.
Comment #5
chx CreditAttribution: chx commentedYes -- variable classnames combined with forbidding #363802: Document class methods with class, factory and defining interfaces is a blight I want to avoid.
Comment #6
Crell CreditAttribution: Crell commentedModule-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.)
Comment #7
chx CreditAttribution: chx commentedNoone wants to move modules into classes. module.inc itself, that's a different question.
Comment #8
Crell CreditAttribution: Crell commentedAh, I see. Yes, moving the logic in module.inc over to classes makes a lot of sense. (I had no context on this apparent conversation from last night.)
Comment #9
catchPasted Justin's draft direct into the summary so there is more context here.
Comment #10
Crell CreditAttribution: Crell commentedAside 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.
Comment #11
catchThe 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).
Comment #12
catchMaking it clearer what the current proposal is.
Comment #13
sunI agree with the fundamental idea. Putting this into the DI container will allow us to bootstrap the module/hook system on demand.
The draft/prototype in the summary looks "workable" to me. I'd call it Drupal\Core\Extension\Manager though.
Given #1497366: Introduce Plugin System to Core, perhaps a Module, Theme, and Profile could be variants/derivatives of an abstract Extension plugin type?
(...so a Libraries API in D8 could declare an additional Library variant...? hm!)
Please also note that #1608842: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config is touching many related parts of this puzzle, and working on that revealed that the current enable/disable routines of modules and themes have little differences (which of course is based on the embedded #1067408: Themes do not have an installation status change).
Comment #14
Crell CreditAttribution: Crell commentedSo 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.)
Comment #15
aspilicious CreditAttribution: aspilicious commentedBut in the end we need to add these bundles. So we should have a hook_info or some kind of symfony .info file. Or something different.
Comment #16
Crell CreditAttribution: Crell commentedSymfony 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.)
Comment #17
RobLoachYes, 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.
Event/hook registration discussion should probably be talked about over at #1509164: Use Symfony EventDispatcher for hook system and #1599108: Allow modules to register services and subscriber services (events).
Comment #18
catchThere's two parts to this I think:
- 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.
Comment #19
Crell CreditAttribution: Crell commentedIf 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.
Comment #20
pounardWhile 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.
Comment #21
Anonymous (not verified) CreditAttribution: Anonymous commentedi'll have a crack at a PoC patch for this tomorrow to get discussion going again.
Comment #22
Anonymous (not verified) CreditAttribution: Anonymous commentedsorry, this is beyond my programming skills.
i don't understand Symfony and our usage of it enough to know where to start here.
Comment #23
Crell CreditAttribution: Crell commentedYou 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. :-)
(Yes we also want that for themes, but one step at a time.)
Comment #24
pounard#23's plan sounds fine as a start.
Comment #25
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedJumping 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.
Comment #26
katbailey CreditAttribution: katbailey commentedI am taking an initial stab at this as it affects #1784312: Stop doing so much pre-kernel bootstrapping.
Comment #27
katbailey CreditAttribution: katbailey commentedOk, 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...
Comment #30
catchI 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 :)
Comment #31
katbailey CreditAttribution: katbailey commentedHmm, I think it makes a lot of sense to wait until #1608842: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config gets in before doing any more work on this. For one thing, I think that one of the problems I'm having with the installer might be solved by chx's patch there.
Comment #32
chx CreditAttribution: chx commentedThat 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.
Comment #33
katbailey CreditAttribution: katbailey commentedI've been reworking this on top of #1608842: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config which looks like it might land soon and touches a lot of the code that this patch needs to move around. Just wanted to roll a combined patch and see if testbot can give me some indication of where we're at with tests.
Comment #35
chx CreditAttribution: chx commentedThe 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.
Comment #36
katbailey CreditAttribution: katbailey commentedSo as it turned out, trying to solve this problem on top of #1608842: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config wasn't such a great idea - I had thought that one was closer to landing than it was and I had also thought there were problems with my intended approach here that relied on it, but that doesn't seem to be the case. Curious to see what testbot makes of this one...
Comment #38
katbailey CreditAttribution: katbailey commentedDurr... 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:
I'll try and tackle this tomorrow.
Comment #39
katbailey CreditAttribution: katbailey commentedOK, 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...
Comment #41
chx CreditAttribution: chx commentedDon't be so sure that the installer work is going to happen... I am certainly not :/
Comment #42
katbailey CreditAttribution: katbailey commentedThe one outstanding test failure was due to the following code:
There is no such statically cached variable anymore - this information is stored in an instance variable of the ExtensionHandler class. So I added a getter method for that to use in the test instead.
Comment #43
katbailey CreditAttribution: katbailey commentedComment #44
katbailey CreditAttribution: katbailey commentedOK, so now that it's green I should clarify what this patch does:
Given my obvious agenda with this patch, i.e. #1784312: Stop doing so much pre-kernel bootstrapping, I hope it still achieves the goals that others have in mind.
Comment #45
Crell CreditAttribution: Crell commentedWow, great work, Kat! Review below.
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*
Is it in module.inc? Hard to tell from the patch but it looks like it's in bootstrap.inc.
camelCase, please.
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...
camelCase, please.
camelCase, please.
You guessed it. :-)
Ibid.
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.
module_list() was a noun. Methods should be verbs. getEnabledModules(), perhaps?
Also see about regarding $type.
Can we eliminate the external call to drupal_get_filename()?
Why is this not just $this->moduleList = array()? (Or NULL, or whatever.)
Using moduleList() (or getEnabledModules(), more properly) as a reset is an artifact we can and should remove.
Same note as above regarding bootstrap vs. not.
We really ought to pull themes out of this, too. Ideally they should go in their own separate object. I think the "eliminate system table" patch moves in that direction, doesn't it?
This looks like something else that should get injected. (The class loader itself.)
Are the drupal_static_reset() calls still necessary?
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.
This method name should get verb-ified.
Isn't there another issue somewhere to make this injected? If not, it should be injected.
Comment #46
pounardIs 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.
Comment #47
chx CreditAttribution: chx commentedAs 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.
Comment #49
chx CreditAttribution: chx commentedOh doh.
Comment #51
chx CreditAttribution: chx commentedI 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.
Comment #52
amateescu CreditAttribution: amateescu commentedLet's try this one, it was a silly replaced-too-much error :)
Comment #54
amateescu CreditAttribution: amateescu commentedThe log is filled with this stuff, but I can't reproduce them locally:
Maybe we need this first? #1067408: Themes do not have an installation status
Comment #55
chx CreditAttribution: chx commented#52: 1331486-52.patch queued for re-testing.
Comment #56
chx CreditAttribution: chx commentedIt was 38,289 pass(es), 105 fail(s), and 13 exception(s)., let's see what a retest brings.
Comment #58
jthorson CreditAttribution: jthorson commentedMissed a spot!
Comment #59
chx CreditAttribution: chx commentedComment #61
chx CreditAttribution: chx commentedI ran all the aggregator tests on a bot, here are the failures:
wildly different from the "1 fail, go away" results. And the manual run on the bot reliably produces these fails so I will look into them now.
Edit: false alarm. These pass if I run the scripts as www-data.
Comment #62
chx CreditAttribution: chx commentedComment #64
chx CreditAttribution: chx commentedComment #66
chx CreditAttribution: chx commentedThat 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).
Comment #67
chx CreditAttribution: chx commentedLet's see this one.
Comment #69
chx CreditAttribution: chx commentedBy the way, if you want to see the interdiff between #59 and #64 then see #1608842-140: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config. Yes, this caused problems before and yes this was fixed already once.
Comment #70
chx CreditAttribution: chx commentedComment #72
chx CreditAttribution: chx commentedComment #73
chx CreditAttribution: chx commentedMy work here is done, I just wanted to get back where Kat was before I shattered her patch by removing {system} :)
Comment #74
katbailey CreditAttribution: katbailey commentedJust 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.
Comment #75
katbailey CreditAttribution: katbailey commentedJust want to see what testbot makes of this as there are a lot of changes...
Comment #77
katbailey CreditAttribution: katbailey commented#75: 1331486.extension_handler.75.patch queued for re-testing.
Comment #79
donquixote CreditAttribution: donquixote commentedWould it bring a benefit to split hooks into their own objects?
(not saying one class per hook, just one object per hook)
E.g.
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.
Comment #80
Crell CreditAttribution: Crell commenteddon: 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. :-(
Comment #81
catchJust a note, while I'd love this patch to land as soon as possible, it's going to be completely valid until code freeze, not feature freeze.
Comment #82
Crell CreditAttribution: Crell commentedSure, but my concern is that feature-sensitive patches get held up by the bootstrap fugly that this patch helps to address.
Comment #83
catchShould we bump it to critical then?
Comment #84
katbailey CreditAttribution: katbailey commentedNow to address Crell's review above (#45):
This patch doesn't remove system_rebuild_module_data() or list_themes() so it seems we still need those, yes.
Seems it was fairly trivial after all, unless I'm missing something...
As for untangling the existing logic of moduleList and friends, I created a follow-up here: #1815610: Untangle the logic of moduleList() and systemList() in the ExtensionHandler.
See this interdiff and the previous one for how I've addressed the other points.
Oh, and I don't know what's with the random test failures - possibly a problem in WebTestBase. I'll see how this one fares and then look into it.
Comment #86
katbailey CreditAttribution: katbailey commentedHmm, if no other patches are exhibiting this strange behavior then there must be a problem in WebTestBase or somewhere. Getting to the bottom of that is going to be fun... :-(
Comment #87
katbailey CreditAttribution: katbailey commentedJust want to see if this solves the rogue test failure problem...
Comment #89
chx CreditAttribution: chx commentedJust a little offset.
Comment #91
katbailey CreditAttribution: katbailey commentedJust a reroll against 8.x - also, I've pushed a branch for this to the wscci sandbox just for collaboration purposes because patches are dumb:
http://drupalcode.org/sandbox/Crell/1260830.git/shortlog/refs/heads/modu...
If anyone else wants to collaborate on this, please use the sandbox, but ping me if you're rebasing the branch (as in deleting and recreating).
Comment #92
katbailey CreditAttribution: katbailey commentedComment #94
katbailey CreditAttribution: katbailey commentedHackety-hack on the stupid WebTestBase tearDown() - ugh.
Comment #95
Crell CreditAttribution: Crell commentedWe 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.
The changes in the KeyValueStore are going to conflict with, or maybe duplicate, #1809206: KeyValueFactory hard-codes DatabaseStorage. I defer to Dratchick on how we want to deal with that race condition. :-)
I think this is really close to commitable, even though there's follow-up work.
Comment #96
katbailey CreditAttribution: katbailey commentedHeh, another great example of the lazy-ass copy/paste approach I've taken to this patch ;-) I've fixed this - see interdiff.
Yeah, this thing will definitely need further refactoring, but it would be great to get it in sooner rather than later because of how it affects the whole bootstrap problem.
Comment #98
chx CreditAttribution: chx commentedSome 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.
Comment #99
BerdirComment 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.
Not sure how all these pieces will fit together...
Comment #100
katbailey CreditAttribution: katbailey commented<sarcasm>Well, that was fun. </sarcasm>
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...
Comment #102
katbailey CreditAttribution: katbailey commentedUgh, 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 :-/
Comment #103
katbailey CreditAttribution: katbailey commentedComment #104
BerdirWill 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.
Comment #106
katbailey CreditAttribution: katbailey commentedOK, now I just want to smash something :-(
Comment #107
BerdirI 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?
Comment #108
katbailey CreditAttribution: katbailey commentedw00t! #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.
Comment #109
katbailey CreditAttribution: katbailey commentedThis patch is not fit for consumption by testbot. For starters it is rolled on top of #1849700: DrupalKernel's container.modules param should contain module filenames, not full namespace paths. It is functional, but probably fairly riddled with holes. The main differences between it and its predecessors are:
I just wanted to put it up because I won't be around over the weekend and thought people might be wondering where I'm at with it seeing as it's been a while.
Comment #110
andyceo CreditAttribution: andyceo commentedInvoking bot.
Comment #112
katbailey CreditAttribution: katbailey commentedWell, if we want the bot to look at it we need to do a combined patch with #1849700: DrupalKernel's container.modules param should contain module filenames, not full namespace paths
Comment #114
katbailey CreditAttribution: katbailey commentedHmm, might get further with this one...
Comment #115
Berdir@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...
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).
Comment #116
BerdirCrosspost.
Comment #118
katbailey CreditAttribution: katbailey commentedFigured 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:
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...
Comment #120
effulgentsia CreditAttribution: effulgentsia commentedWe 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.
Comment #121
catchI'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.
Comment #122
katbailey CreditAttribution: katbailey commentedWell, 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.
Comment #124
pounardThis 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.
Comment #125
katbailey CreditAttribution: katbailey commented@pounard, I totally agree - read my first paragraph in #122.
Comment #126
pounardOh right, I missed it, I skipped directly to the patch review. Sounds odd.
Comment #127
katbailey CreditAttribution: katbailey commentedHUGE thanks to chx and jthorson for pinpointing the problem that testbot was having. Turned out I had broken drupal_get_filename() - see https://twitter.com/chx/status/277634267947859968 :-D Fun.
This patch *should* get us to the point of seeing how many test failures there are - I have no doubt it will be a crap-ton, but that would still be progress...
Comment #129
Dave ReidThis section looked odd/wrong. getEnabledModules() vs the converse setModuleList() - doesn't match up. modulelList() [sp] vs getEnabledModules().
Comment #130
katbailey CreditAttribution: katbailey commentedSo, 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.
Comment #132
katbailey CreditAttribution: katbailey commentedThis 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.
Comment #134
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedI would move and rename the ExtensionHandlerMinimal class to:
DRUPAL_ROOT . /tests/Drupal/Core/Tests/Extension/ExtensionHandlerStub
@see #1872006: Add parity test classes for lib/Core and lib/Component for the idea.
Comment #135
chx CreditAttribution: chx commentedUsing 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().
Comment #136
katbailey CreditAttribution: katbailey commentedThis 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).
Comment #138
katbailey CreditAttribution: katbailey commentedBefore tackling the rest of the tests I really do want to get rid of the horrible drupal_extension_handler() wrapper, so let's see how this one fares...
Comment #140
katbailey CreditAttribution: katbailey commentedOne step forward, two steps back... :-(
Comment #142
Wim Leers#140: Seems like it's still another step forward! Very impressive work here! :)
Comment #143
katbailey CreditAttribution: katbailey commentedRather 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.
(Thanks for the words of encouragement, Wim :))
Comment #145
katbailey CreditAttribution: katbailey commentedI 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.
Also, I really want to try and get this green this weekend - I'm using a branch in the wscci sandbox in the hope that I might get some collaborators to help with the remaining tests: http://drupalcode.org/sandbox/Crell/1260830.git/shortlog/refs/heads/modu.... Just post a comment here if you are interested in helping and say which tests you are focusing on.
Comment #147
katbailey CreditAttribution: katbailey commentedWe *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).
Comment #148
David_Rothstein CreditAttribution: David_Rothstein commentedGiven 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:
This really leaves me wondering whether or not these methods are supposed to work for things other than modules.
Comment #150
katbailey CreditAttribution: katbailey commentedFixes 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.
Comment #152
katbailey CreditAttribution: katbailey commented#150: 1331486.extension_handler.150.patch queued for re-testing.
Comment #154
Anonymous (not verified) CreditAttribution: Anonymous commentedthe contextual links test fails because we haven't loaded contextual.module in to memory before _theme_process_registry() for node_theme runs.
so, although 'contextual' is checked as a candidate prefix in _theme_process_registry(), function_exists() fails:
Comment #155
Anonymous (not verified) CreditAttribution: Anonymous commentedso #154 is rubbish. the problem is this in _theme_process_registry():
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:
Comment #156
Anonymous (not verified) CreditAttribution: Anonymous commentedattached patch with change from #155.
Comment #158
Anonymous (not verified) CreditAttribution: Anonymous commentedsigh. so we had a variable called $m, and we used a variable called $module. renamed it so we might not do that again. and because i wanted more than 5 characters for the last 2 hours of effort :-\
will the bot have pity on us?
Comment #159
katbailey CreditAttribution: katbailey commented@beejeebus I owe you a very large whiskey. Thank you so much for figuring this out. (To think it was down to such sloppiness on my part... ugh.)
I've made the change suggested by David in #148, i.e. including the word "module" in each of the method names for clarity.
Also reverted a bad "fix" I had made to BundleTest - the real fix was done later on and the change here served only to cover up the underlying problem.
Comment #160
aspilicious CreditAttribution: aspilicious commentedContains ...
Can use a docblock
I'm not sure this is the correct docs block for constructors. You should check node/1354
\Drupal\Core...
Comment #161
msonnabaum CreditAttribution: msonnabaum commentedFirst time looking at this patch, couple of quick observations.
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.
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().
Comment #162
BerdirAgree, 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.
Comment #163
msonnabaum CreditAttribution: msonnabaum commentedWell, if it's not possible to pass "state" as an argument to the k/v factory when defining the ExtensionHandler service (I have nfi if that is possible), then we just need to create a new State class.
Comment #164
BerdirWe 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).
Comment #165
g.oechsler CreditAttribution: g.oechsler commentedI took time to review the whole thing. I could not find too much to complain about. So for the rest of this comment: nitpickery++!
I'm not entirely sure, but it feels like these functions are deprecated in favor to their ExtensionHandler method equivalent. Maybe this should be stated in the respective docblocks.
Gotcha, missing punctuation. ;)
Before we were relying on system_list_reset() to be there anyway - now we check on it. So the code before was either terribly broken or this if-statement is obsolete.
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.Comment #166
chx CreditAttribution: chx commented> Before we were relying on system_list_reset() to be there anyway - now we check on it. So the code before was either terribly broken or this if-statement is obsolete.
Neither, before we checked whether module.inc was ever included by checking on one function in that file, now that things got moved around we check on another.
Comment #167
katbailey CreditAttribution: katbailey commentedOK, 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.
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.
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?
Comment #168
tim.plunkettI agree with not using the array_keys() for that, it's an extra function call for only marginally better readability.
As far as the procedural functions, we now have #1876842: [policy, no patch] Use @deprecated to indicate a deprecated method or function, so you can put @deprecated on them until the conversion follow-up is done, hopefully mitigating any further additions of them to core.
Comment #170
Crell CreditAttribution: Crell commentedThere's discussion of allowing @deprecated in docblocks, which looks like it will happen: #1876842: [policy, no patch] Use @deprecated to indicate a deprecated method or function I'd say go ahead and use 'em.
I think the foreach() is fine as is; there's a couple places we do that, and since there's no performance difference either way as far as I know, meh.
Further comments, all should be relatively minor:
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.)
Comment should be updated, since I presume module_hook() is now another method here.
Shouldn't module_load_include() also turn into a method, rather than a function?
As above, I think module_load_include() should move into this class. And its parameter order should change to not be stupid. :-)
isLoaded() to me suggests "is this object loaded", not "have we loaded all the things". That may be too subtle a distinction here, but the method name seems odd to me.
What's a file system object? SplFile? That would be my guess, but it could also be an array structure of some sort. Doc needs improvement.
$files should be type hinted to array for completeness.
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.
katbailey++!
Comment #171
effulgentsia CreditAttribution: effulgentsia commented#167: 1331486.extension_handler.167.patch queued for re-testing.
Comment #173
tim.plunkett#1356170: Remove all uses of array_merge_recursive, or document why they are being used instead of NestedArray::mergeDeep() changed the following:
with
use Drupal\Component\Utility\NestedArray;
at the topComment #174
katbailey CreditAttribution: katbailey commentedThanks @tim!
Latest patch:
$files
toarray $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.Let's see what testbot thinks...
Comment #175
msonnabaum CreditAttribution: msonnabaum commentedNaming changes look very good to me.
Comment #176
Crell CreditAttribution: Crell commentedThe interdiff from #174 looks OK to me.
Comment #178
tim.plunkettThe fatal is:
Comment #179
katbailey CreditAttribution: katbailey commentedThis is now completely broken since #1792536: Remove the install backend and stop catching exceptions in the default database cache backend went in :-(
I just spent hours fixing the installer only to find WebTests are OOMing (somewhere in list_themes() as far as I can tell).
Not putting up a new patch in that state but the fix for the installer is in the branch in the sandbox in case anyone else feels like going nuts: http://drupalcode.org/sandbox/Crell/1260830.git/shortlog/refs/heads/modu...
Comment #180
sunThe changes visible in the sandbox commit are hinting at the topic of: Installer + System module.
#1798732: Convert install_task, install_time and install_current_batch to use the state system streamlines and removes a lot of special-casing that's currently going on there, and based on your debugging commit, you seem to struggle with exactly that (and this is not the only issue that runs into the problem). So it would probably make sense to get that patch over there finally in.
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.
Comment #181
Crell CreditAttribution: Crell commentedTagging so we don't break it again.
Comment #182
katbailey CreditAttribution: katbailey commentedThanks @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()
insetUp()
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 (duringconfig_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.
Comment #183
katbailey CreditAttribution: katbailey commentedOops
Comment #184
sunFWIW... I've merged this patch on top of #1798732: Convert install_task, install_time and install_current_batch to use the state system, and the installer seems to work pretty fine with that. Sent it for testing in http://drupal.org/node/1766036#comment-6933768
Comment #185
sunSo applying this patch on top of #1798732: Convert install_task, install_time and install_current_batch to use the state system gets us back to 1 fail, and 201 exceptions.
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.
Comment #186
katbailey CreditAttribution: katbailey commentedI 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.
Comment #187
sunHm. Why didn't you simply inject the Cache\MemoryBackend and KeyValue\MemoryStorage?
That's what we're doing with everything else in the installer and really the most simple way.
Comment #189
katbailey CreditAttribution: katbailey commentedBecause 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?
I'll try and fix that pesky test.
Comment #190
katbailey CreditAttribution: katbailey commentedWe can't call drupal_install_schema() without a 'module_handler' service having been registered to the container.
Comment #191
msonnabaum CreditAttribution: msonnabaum commentedI agree that the current design with the caching logic separated is much better. It's intent is clearer and it decomposes some of those methods that were huge to begin with.
Comment #193
katbailey CreditAttribution: katbailey commentedDoh! I missed that exception before. Easy fix...
Comment #194
chx CreditAttribution: chx commentedRerolling this monster should stop.
However, the following minor things needs to be done in a followup:
drupal_container()->get('module_handler')->setModuleList($module_list); and load() should chain
drupal_load runs drupal_container()->get('module_handler') twice
the doxygen drupal_container()->get('module_handler')->alter($hook). is incorrect it's >alter($type, $data, $context1, $context2);
->addArgument('%container.modules%') we need to give katbailey an award for this
information discovered during a Drupal\Core\SystemListing scan.
needs to be \Drupal.
loadInclude could use a more modern syntax that doesnt make me go bonkers
Comment #195
chx CreditAttribution: chx commentedAnyone 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.
Comment #196
sunComment #197
sunFirst of all, re-rolled against HEAD; didn't apply anymore (and the branch in the wscci sandbox doesn't contain latest changes).
Comment #199
sunJust 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.
Comment #200
chx CreditAttribution: chx commentedThanks 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?
Comment #201
chx CreditAttribution: chx commentedEh, it's CNW for the bot fails.
Comment #202
chx CreditAttribution: chx commentedI 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.
Comment #203
plach@chx:
The patch is empty :)
Comment #204
chx CreditAttribution: chx commentedSecond attempt.
Comment #205
chx CreditAttribution: chx commentedBlargh. (this patch was uploaded by registering a new user.)
Comment #207
sun~6 hours of debugging and fixing later, I'm feeling pretty exhausted. Here's the slightly corrected and also cleaned up patch:
The most noteworthy changes happened to
module_enable()
,module_disable()
, andmodule_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 thesystem.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, sincemodule_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 asmodule_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 intomodule_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 callmodule_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 renamegetModuleList()
intogetModuleFilenames()
, and introduce a newgetModuleList()
that would return whatmodule_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.
Comment #209
sun#207: module.handler.200.patch queued for re-testing.
Comment #211
sunComment #213
katbailey CreditAttribution: katbailey commented#211: module.handler.211.patch queued for re-testing.
Comment #215
sunFixed LocaleUpdateTest.
The test failure in Drupal\rest\Tests\Views\StyleSerializerTest seems to be a random test failure — potentially caused by not accounting for sanitized strings.
Comment #216
sunAnd we're green again. :)
Also filed #1892016: Random test failure in Drupal\rest\Tests\Views\StyleSerializerTest
Comment #217
podarok#215 looks good
Comment #218
katbailey CreditAttribution: katbailey commentedIt was with great trepidation that I opened that 80K interdiff last night... but what I found were a lot of sensible changes that amount to a great refinement of the patch. So yay! back to RTBC :-D
Let's put this beast to bed already!
Comment #219
Dries CreditAttribution: Dries commentedWhile I like this in principle, this requires performance profiling and benchmarking before it can be committed. This could add quite a bit of overhead to Drupal.
Not sure this is truly 'critical' but I'll leave it as 'critical' for now.
Comment #220
Dries CreditAttribution: Dries commentedComment #221
Dries CreditAttribution: Dries commentedIt 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.
Maybe for now just drop the '_handler' part?
Comment #222
Lars Toomre CreditAttribution: Lars Toomre commentedNot 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?
Comment #223
katbailey CreditAttribution: katbailey commentedIn 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.
Comment #224
msonnabaum CreditAttribution: msonnabaum commentedI agree with Dries, we should pick a separator and use it consistently. However, that would require changing all the previous ones, so I think it's best handled in a follow up.
Comment #225
Crell CreditAttribution: Crell commentedPer #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.
Comment #226
catchYes I agree that's a follow-up. Moving back to needs review for profiling. This /ought/ to be low impact (i.e. if there's a big performance hit it'll hopefully be because of something silly).
One thing we can potentially get out of it though is lazy-loaded modules which would be really nice for lightweight router items etc.
Comment #227
catchComment #228
msonnabaum CreditAttribution: msonnabaum commentedTested with the apc loader, showing a slight performance improvement. Here's the summary for 50 samples both before an after the patch:
Before:
After:
Comment #229
effulgentsia CreditAttribution: effulgentsia commentedIn terms of profiling, one of my concerns is:
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 thanglobal_function()
.This could possibly be partially mitigated with a
module_handler()
function that statically caches the result ofdrupal_container()->get('module_handler')
?Anyway, getting data first before micro-optimizing makes sense, but hopefully this helps identify what scenarios deserve profiling.
Comment #230
effulgentsia CreditAttribution: effulgentsia commentedx-post. If overall performance is looking like an improvement, then I'm ok with #229 being a follow up.
Comment #231
chx CreditAttribution: chx commentedProfiling 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.
Comment #232
effulgentsia CreditAttribution: effulgentsia commentedThe 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'.
Comment #233
effulgentsia CreditAttribution: effulgentsia commentedx-post. #232 can also be followup material.
Comment #234
effulgentsia CreditAttribution: effulgentsia commentedYeah, 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.
Comment #235
webchickThis terrifies me, so assigning to catch. :D
Comment #236
katbailey CreditAttribution: katbailey commentedNeeded 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..?
Comment #237
katbailey CreditAttribution: katbailey commentedComment #238
sunCreated two follow-up issues:
#1893680: Rename 'module_handler' service and ModuleHandler classes
#1893690: Find a new home and service for module_enable(), module_disable(), module_set_weight(), etc
Would be great to see this in, so we can unblock dependent issues and move on with follow-up issues to clean up the remaining parts.
Comment #239
catchThanks for the follow-ups.
I've gone ahead and committed/pushed this to 8.x. This will need a change notice.
Comment #240
jibrantagging
Comment #241
jibranStatus for change notice.
Comment #242
sunBtw, work and IRC discussions on this also triggered: #1892574: Remove hook_hook_info_alter() ;)
Comment #243
catchThis broke HEAD. Reverted for now.
Comment #244
xjmComment #245
xjm#236: 1331486.module_handler.236.patch queued for re-testing.
Comment #247
sunComment #248
sunupdate.php fails with:
due to #1479454: Convert user roles to configurables
Furthermore,
update_access_allowed()
manually loads user.module to calluser_access()
, which in turn circles back into user roles being converted into config entities, but not being migrated into config yet.Furthermore, the entire error stack triggers a fatal error in itself:
Conclusion:
1) update.php is completely hosed currently, regardless of how we put it, and regardless of which patches we roll back or forth.
2) We need a custom kernel/bundle for update.php, that overrides a range of services.
Working on this.
Comment #249
sunThe addition of UpdateBundle might be debatable, but it's a fully working solution. I'd suggest to do this for now, and if needed, re-evaluate a better solution in a follow-up issue.
Comment #250
xjmSee also: #1893800-9: Something is very, very wrong with update.php / upgrade tests... demons suspected (which @larowlan and I discussed for filing as its own issue) and #1894286: System updates are executed without priority.
Comment #251
xjmSorry, xpost.
Comment #252
xjmMaybe #249 should be in its own issue, and this one postponed on it? It might help solve a lot of problems.
Comment #253
moshe weitzman CreditAttribution: moshe weitzman commentedUpdateBundle looks small and clean to me. +1. Thanks also for fixing that error handler config dependency.
Comment #254
sunAnd we're green again. Hope we can move forward with this.
Ideally, before any other commits — this patch has an exceptionally high hit rate for commit conflicts.
Comment #255
katbailey CreditAttribution: katbailey commentedsun++
The changes in #249 look entirely reasonable to me - back to RTBC.
Comment #256
sunTo clarify upfront, because it's probably not obvious:
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 ;)
Comment #257
catchThe 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.
Committed/pushed to 8.x again.
Comment #258
sunChange notice: http://drupal.org/node/1894902
Comment #259
sun*cough* #1894910: Move module_invoke() into ModuleHandler *cough* ;)
Comment #260
jhodgdonSigh. 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.
Comment #261
TwoDThis patch broke installations not using English. Please see #1898236: Installing without English language leads to Fatal Error.
Comment #263
alexpottCleaning up tags...
Comment #264
YesCT CreditAttribution: YesCT commentedrelated #2007684: Update deprecated doc for module_exists() and call method inside
Comment #265
joachim CreditAttribution: joachim commentedThe params for invokeAll() are not the same as for module_invoke_all(), but the docs for the new invokeAll() have the old parameters. The change record doesnt state that the parameters have changed for invoking a hook. See #2095219: incorrect params in ModuleHandlerInterface::invokeAll().
Comment #265.0
joachim CreditAttribution: joachim commentedAdding pastie draft