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:

  1. Invoke hook_disable() on the first module that will be disabled.
  2. 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.
  3. Update the database and caches to actually disable the module.
  4. (repeat steps 1-3 for other modules)
  5. 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

Reference: https://www.drupal.org/core/beta-changes
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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

David_Rothstein’s picture

Status: Active » Needs work
FileSize
3.2 KB

Here 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.

David_Rothstein’s picture

Closely 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.

moshe weitzman’s picture

One 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.

David_Rothstein’s picture

The 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 calling module_enable('module1') in one page request and module_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.

ksenzee’s picture

Title: module_disable() and hook_modules_disabled() don't work as documented » Enabling modules one at a time works differently than enabling them all at once

David 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.

rszrama’s picture

Just 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.

rfay’s picture

fubhy’s picture

Version: 7.x-dev » 8.x-dev
Assigned: Unassigned » fubhy

Just 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.

fubhy’s picture

Status: Needs work » Needs review
FileSize
4.12 KB
fubhy’s picture

Issue tags: +Needs backport to D7

We should at least backport a fix for the documentation. It's very misleading currently.

Status: Needs review » Needs work

The last submitted patch, 727876-9.patch, failed testing.

fubhy’s picture

Status: Needs work » Needs review
FileSize
4.12 KB

This should work better and also make more sense...

fubhy’s picture

FileSize
4.12 KB

Ahrm... This I meant...

fubhy’s picture

Assigned: fubhy » Unassigned
David_Rothstein’s picture

tim.plunkett’s picture

Priority: Normal » Major

Closing the other as a dupe, but copying over the priority.

anavarre queued 13: 727876-13.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 13: 727876-13.patch, failed testing.

timmillwood’s picture

Issue tags: +Needs reroll, +Newbie
--- a/core/includes/module.inc
+++ b/core/includes/module.inc

--- a/core/modules/system/lib/Drupal/system/Tests/Module/EnableDisableTest.php
+++ b/core/modules/system/lib/Drupal/system/Tests/Module/EnableDisableTest.php

--- a/core/modules/system/tests/modules/system_test/system_test.module
+++ b/core/modules/system/tests/modules/system_test/system_test.module

Needs re-roll for three files, should be quite an easy one.

nlisgo’s picture

Issue tags: -Newbie +Novice
hitesh-jain’s picture

Assigned: Unassigned » hitesh-jain
valthebald’s picture

Assigned: hitesh-jain » Unassigned
Category: Bug report » Task
Priority: Major » Normal
Issue summary: View changes
Issue tags: -Needs reroll, -Novice

Added 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

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

Status: Needs work » Closed (outdated)

hook_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.