Here are some fun facts:

1. Drupal runs updates for modules that are disabled.
2. Many modules use their own API functions in their update functions.
3. Many API functions such as mymodule_save() do something like module_invoke_all('mymodule_save') inside of them.
4. For reasons of code organization, many modules like to implement their own hooks in cases like the above, rather than sticking extra hardcoded logic into the API function directly.
5. module_invoke_all() does not invoke hooks for disabled modules.

Putting it all together, that means that when you invoke your own module's API function from inside an update function, it might work very differently depending on whether your module is disabled at the time the update is being run - and that can break things nicely.

Possible solutions:
1. Tell people that they should generally not be using API functions inside update hooks (unless they really know what they're doing), but rather do direct database queries instead. See also: #717902: Updated DB schema not available in module update functions. In general this doesn't seem desirable though.
2. When MAINTENANCE_MODE is 'update', sometimes module_invoke_all() and friends could invoke the hook on disabled modules as well? - needs a lot more thought though.

Files: 
CommentFileSizeAuthor
#22 module-enabled-before-uninstall-734710-22.patch3.69 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 58,583 pass(es). View
#22 interdiff-21-22.txt1 KBDavid_Rothstein
#21 module-enabled-before-uninstall-734710-21.patch3.22 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 58,896 pass(es). View
#18 module-enabled-before-uninstall-734710-18.patch2.25 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] 58,762 pass(es), 3 fail(s), and 2 exception(s). View
#16 module-enabled-before-uninstall-734710-16.patch1.06 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] 58,685 pass(es), 3 fail(s), and 4 exception(s). View
#15 module-enabled-before-uninstall-734710-15.patch679 bytesDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] 56,820 pass(es), 1,418 fail(s), and 235 exception(s). View

Comments

sun’s picture

If we run updates for disabled modules, then we also need to invoke hooks for disabled modules.

Am I paranoid or did we just happen to talk about this in #562694: Text formats should throw an error and refuse to process if a plugin goes missing? ;)

David_Rothstein’s picture

Sort of - related but not quite the same thing :)

There we talked about possibly invoking some hooks for disabled modules always. Here we are talking about possibly invoking all hooks for disabled modules during updates only.

This might get messy though. There are a few different ways modules tend to invoke hooks, so we'd have to hit all of them. Plus, having to load all of their .module files when running update.php (in addition to the .install files) might not do wonders for memory usage...

qasimzee’s picture

subscribe

chx’s picture

Status: Active » Closed (fixed)

Or will be fixed. Or duplicate. This is being worked on.

David_Rothstein’s picture

Version: 7.x-dev » 8.x-dev
Status: Closed (fixed) » Active

Yes, for Drupal 7 I believe we've settled on solution #1 (just avoid invoking the hooks). This can be documented in #794192: Documentation of hook_update_N() should explain that certain functions cannot be called from there.

I think this issue should be left open for Drupal 8 to discuss other possible solutions.

chx’s picture

Status: Active » Closed (fixed)

Drupal 8 needs a migrate module that we kind of agreed of at Coppenhagen. This sort of update is not viable any more due to the complexity we reached.

sun’s picture

Status: Closed (fixed) » Active

@chx: Can you please stop doing this? I understand that you're pissed off, but that doesn't mean this issue is resolved in any way. Definitely D8 though.

pwolanin’s picture

Why are we running updates for disabled modules?

sun’s picture

I guess because disabled modules are installed, and so their installed database schema/data needs to be updated.

David_Rothstein’s picture

Yes, I believe it was to make sure you can turn on a disabled module later without it exploding.

#194310: Check / run updates of disabled modules

pwolanin’s picture

I think we should NOT update disabled modules in D7 - the admin should either uninstall them (or remove the code) or enable them for the update.

David_Rothstein’s picture

Title: API functions that invoke hooks don't work as expected when a disabled module is being updated » API functions that invoke hooks don't work as expected when a disabled module is being updated or uninstalled
Component: update system » database update system

This issue affects hook_uninstall() also - changing title. (See discussion in #1029606: Regression: Not loading the .module file causes a fatal error when uninstalling some modules (as does loading it) for more.)

rfay’s picture

Such a mess.

I find these related issues:
#923432: Disabled modules break update
#773828: Upgrade of disabled modules fails

It looks to me like we're committed to upgrading disabled modules. But if we are we ought to make it work somehow.

sun’s picture

Issue tags: +upgrade path

Tagging.

David_Rothstein’s picture

Status: Active » Needs review
FileSize
679 bytes
FAILED: [[SimpleTest]]: [MySQL] 56,820 pass(es), 1,418 fail(s), and 235 exception(s). View

I think the update part of this issue was fixed by #2001310: Disallow firing hooks during update, though I'm not changing the issue title for now.

As for uninstall, I'm posting this patch out of curiosity, to see if it passes tests. The idea is simply that we don't allow disabled modules to be uninstalled; instead, we force the module to be enabled first and uninstall it directly from that state. (In reality, it's probably not a good idea to do this at the API level only as the attached patch does, since we'd be lying to site admins about what's going on and enabling modules for them without their knowledge. But I think it's still a reasonable start.)

David_Rothstein’s picture

FileSize
1.06 KB
FAILED: [[SimpleTest]]: [MySQL] 58,685 pass(es), 3 fail(s), and 4 exception(s). View

Well, it's probably not that simple, since there are some things that the disable() method currently does that the uninstall() method would need to do now too (and there may be some new cache issues to deal with also). But in theory it's not hard to port those over.

Here's a version that does one of those things - an important one if you don't want the module to show up as enabled on the modules page after it has already been uninstalled :)

Status: Needs review » Needs work

The last submitted patch, module-enabled-before-uninstall-734710-16.patch, failed testing.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
2.25 KB
FAILED: [[SimpleTest]]: [MySQL] 58,762 pass(es), 3 fail(s), and 2 exception(s). View

That's pretty close. I wonder if moving over a few more things from the disable() method would fix the rest?

Status: Needs review » Needs work

The last submitted patch, module-enabled-before-uninstall-734710-18.patch, failed testing.

David_Rothstein’s picture

Hm, remaining failures are all because calling watchdog() after the dblog module is uninstalled is still trying to write to that module's database tables.

Should be fixable, although I thought it would have already been fixed by the changes in the above patch.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
3.22 KB
PASSED: [[SimpleTest]]: [MySQL] 58,896 pass(es). View

Partially resolving one of the @todos in the code copied over from the disable() method may be enough to get the tests passing.

Trying out these patches manually, everything seems to work but there's an exception thrown if you try to uninstall modules while using the Overlay. This appears to be a preexisting, independent bug that is just manifesting itself in exciting new ways when combined with the patch here (see #2084115: ModuleHandler::isLoaded() returns FALSE immediately after a module is enabled, causing an exception when theme() is called).

David_Rothstein’s picture

FileSize
1 KB
3.69 KB
PASSED: [[SimpleTest]]: [MySQL] 58,583 pass(es). View

There's an issue that doesn't show up in the tests but which occurs if you uninstall multiple modules at once (or a single module with dependencies). The dependencies need to be enabled first and in the correct order, then restored to the disabled state if that's where they were before.

The attached patch should fix it. As mentioned above, it's a bit dicey to do these kinds of things without corresponding changes in the user interface, and this is an especially good example of that. Most likely we would want to change the user interface to force the site administrator to enable a module before uninstalling it, and then all the code added in this interdiff could go away and it would just be up to the calling function to only pass already-enabled modules to the uninstall() method. But I think as a first step this patch still stands on its own.

David_Rothstein’s picture

Title: API functions that invoke hooks don't work as expected when a disabled module is being updated or uninstalled » API functions that invoke hooks don't work as expected when a disabled module is being uninstalled

Also changing title, as I think it's pretty clear that #2001310: Disallow firing hooks during update did resolve this (albeit in a brute force way) for the case of module updates.

jhedstrom’s picture

Issue summary: View changes
Status: Needs review » Closed (won't fix)

I think this is no longer an issue since #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed landed, and modules cannot be in a disabled state.