Issue description updated: comment #22
This issue started with me trying to follow up on #620298: Schema not available in hook_install(), where among other things we removed node access specific code from module_enable() and put it in node_modules_enabled() instead. I was trying to do the same thing here with the node access code in module_disable(), and quickly ran into the following problem.
As per the "Invoke hook_modules_disabled" comment in the below excerpt from module_disable():
if (!empty($invoke_modules)) {
// Refresh the module list to exclude the disabled modules.
system_list_reset();
module_list(TRUE);
module_implements('', FALSE, TRUE);
// Invoke hook_modules_disabled before disabling modules,
// so we can still call module hooks to get information.
module_invoke_all('modules_disabled', $invoke_modules);
// Update the registry to remove the newly-disabled module.
registry_update();
_system_update_bootstrap_status();
}
the intention is that disabled modules hooks are still available when hook_modules_disabled() is called.
However, as can easily be seen from the code order, this does not actually happen. It's not hard to fix by moving the code around, but this then has some odd side effects. For example, any API function call you make from inside hook_modules_disabled() that invokes hooks of its own will then invoke that hook on the disabled module as well, which could lead to unexpected results.
In order to preserve the ability indicated in the code comment, the only way I can think of to do this is to add a new hook, and then do an overall rewrite of module_disable() to make it more sane and make it work similarly to the way we made module_enable() work in #620298: Schema not available in hook_install(). In particular, the function should work like this:
- Invoke hook_disable() on the first module that will be disabled.
- Invoke a new hook, something like "hook_module_disabling", which allows other modules to respond to that module alone, before it is disabled - i.e., fulfilling the actual intended functionality of the code comment above.
- Update the database and caches to actually disable the module.
- (repeat steps 1-3 for other modules)
- Invoke hook_modules_disabled() to inform all modules about the list of modules that were disabled. This should happen after the module's hooks can no longer be called, so it is safe to call API functions and know that the disabled modules are really disabled and won't still be hanging around. In other words, I think this hook should work as it does work now, not as it is documented to work.
It's possible that steps 1 and 2 in my above list should actually be switched around - would need to think more about that.
Beta phase evaluation
Issue category | Task because it's not a bug, but improvement/deduplication of logic |
---|---|
Issue priority | Normal |
Disruption | Disruptive for core/contributed and custom modules because it will change the way modules are enabled/disabled |
Comment | File | Size | Author |
---|---|---|---|
#13 | 727876-13.patch | 4.12 KB | fubhy |
#12 | 727876-12.patch | 4.12 KB | fubhy |
#9 | 727876-9.patch | 4.12 KB | fubhy |
#1 | hook-modules-disabled-node-access-rebuild.patch | 3.2 KB | David_Rothstein |
Comments
Comment #1
David_Rothstein CreditAttribution: David_Rothstein commentedHere is the code I started working on before I fully realized the above problem. As can be seen, the node access code essentially needs to act both before and after the disabled modules' hooks stop being available - therefore, I think it needs the full list of steps proposed above in order to actually be able to move it out of module_disable().
If the node access code needs this capability, then presumably contrib modules might need it as well, which is why this really should be fixed.
Comment #2
David_Rothstein CreditAttribution: David_Rothstein commentedClosely related to this issue, I wonder if there is a need for a parallel hook in module_enable(), something like "hook_module_enabling"?
The reason is that if multiple modules are being enabled in a single request, your module may have important things to do after each module is enabled (e.g., clearing caches) that is important to do right away so as not to adversely affect the next module in this list when its enable hook is being called. In those case, hook_modules_enabled() is too late, since it makes you wait until the end.
See the patch at #588882-19: Filter fallback format clean-ups for an example of this, where I'm trying to use hook_modules_enabled() and hook_modules_disabled() to clear some caches in the filter module, but in reality, that won't work correctly if a long list of modules is being enabled or disabled all at once.
Comment #3
moshe weitzman CreditAttribution: moshe weitzman commentedOne way to bail on this for now is to call node_access_needs_rebuild(TRUE). Then an admin will get a drupal_set_msg to that a rebuild is needed. We already do this for the node access system in other flows. I think it is sane here too.
Comment #4
David_Rothstein CreditAttribution: David_Rothstein commentedThe best way to bail is not to bother moving the node access code at all - the world won't end if it stays where it is :)
However, I do think this issue really reveals a bug. Calling
module_enable('module1', 'module2')
in one page request should work the exact same as callingmodule_enable('module1')
in one page request andmodule_enable('module2')
in the next - and similar for module_disable() - but with the hooks that core currently provides it doesn't seem that's 100% possible to achieve.Comment #5
ksenzeeDavid in #2 and #4 was quite prescient. It turns out this is the root cause of a bug I just upgraded to critical: #882364: re-enabling Forum module after disabling Taxonomy module causes an exception because Forum thinks taxonomy_forum is inactive
Adding a hook at this late date is likely a total no-go. But I'm guessing the only correct way to fix this is to add the intermediate hook David describes in #2 - something like hook_modules_enabling, or hook_module_enable (in the singular). If we bump this to D8, then we'll run into bugs like #882364: re-enabling Forum module after disabling Taxonomy module causes an exception because Forum thinks taxonomy_forum is inactive and #588882-19: Filter fallback format clean-ups that people will have to work around on a case-by-case basis.
Comment #6
rszrama CreditAttribution: rszrama commentedJust adding a note to this issue that the behavior is different between module installation through the admin UI vs. install.php. Is this because the installer is using a batch and installing modules in order one by one? If so, is there any reason the admin UI doesn't work via a batch for the installation of modules so each can safely presume the prior module has been fully installed?
This problem is affecting our modules using hook_enable() to add fields to entities defined by the module itself, because even though the dependencies defining the Fields have been installed, the Field data isn't available for some reason.
Comment #7
rfayNote that #821290: Unexpected error after disabling - unistalling - re-enabling forum module went in, and that #882364: re-enabling Forum module after disabling Taxonomy module causes an exception because Forum thinks taxonomy_forum is inactive had been marked as a duplicate of that one.
Comment #8
fubhy CreditAttribution: fubhy commentedJust ran across this in #1872874: Remove the module name property from the role permission table. It's still broken in D8. Will try to roll a patch later.
Comment #9
fubhy CreditAttribution: fubhy commentedComment #10
fubhy CreditAttribution: fubhy commentedWe should at least backport a fix for the documentation. It's very misleading currently.
Comment #12
fubhy CreditAttribution: fubhy commentedThis should work better and also make more sense...
Comment #13
fubhy CreditAttribution: fubhy commentedAhrm... This I meant...
Comment #14
fubhy CreditAttribution: fubhy commentedComment #15
David_Rothstein CreditAttribution: David_Rothstein commented#2079513: hook_modules_installed runs in a inconsistent environment seems related (and possibly a duplicate).
Comment #16
tim.plunkettClosing the other as a dupe, but copying over the priority.
Comment #19
timmillwoodNeeds re-roll for three files, should be quite an easy one.
Comment #20
nlisgo CreditAttribution: nlisgo commentedComment #21
hitesh-jain CreditAttribution: hitesh-jain at Acquia commentedComment #22
valthebaldAdded beta-evaluation
Removed Novice tag
This issue is committable before 8.0, only if core maintainers decide that it reduces fragility (i.e. by merging code that enables/disables modules by one, or in a bulk). Otherwise should be postponed
Comment #35
quietone CreditAttribution: quietone at PreviousNext commentedhook_modules_disabled was removed in #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed. I think there is an issue for adding the ability to disable modules but I could be wrong.
I am closing this as outdated.