How to reproduce:
1. install Drupal 6.16
2. enable the taxonomy module and create a vocabulary
3. disable the taxonomy module
4. upgrade to Drupal 7

Fatal error: Call to undefined function taxonomy_get_vocabularies() in /Applications/MAMP/htdocs/d7git/modules/taxonomy/taxonomy.install on line 263
(similar problem with the contact module)

If we want to upgrade all the modules in one go (including the disabled modules) we need to include their files during the upgrade process.

Files: 
CommentFileSizeAuthor
#4 773828.patch991 bytesericduran
PASSED: [[SimpleTest]]: [MySQL] 20,157 pass(es).
[ View ]
#3 773828.patch990 bytesericduran
PASSED: [[SimpleTest]]: [MySQL] 20,138 pass(es).
[ View ]

Comments

prabhu.k’s picture

good

jbrown’s picture

Priority:Normal» Critical

Reproduced.

ericduran’s picture

Component:base system» update system
Status:Active» Needs review
StatusFileSize
new990 bytes
PASSED: [[SimpleTest]]: [MySQL] 20,138 pass(es).
[ View ]

I'm assuming the updates are supposed to run even if the module is disable.

The attached patch checks if the module is enable before running the update functions for the module.

This might not be the greatest approach to it. I'll wait for feedback (it's pretty hackish).

Also I'm sure this can be written to be more efficient, since right now is enabling and disabling a module multiple time if the module is disable and it has multiple updates to run on. I'll see if I can make this better.

ericduran’s picture

StatusFileSize
new991 bytes
PASSED: [[SimpleTest]]: [MySQL] 20,157 pass(es).
[ View ]

minor style clean-up

KarenS’s picture

We ran into this in the D6 cycle too. The recommended solution was that install functions should never include anything but core functions that would always be available, if you needed other functions you had to reproduce all the necessary code in the install file. So that would say that all the code needed by the missing taxonomy_get_vocabularies() function should be replicated in the install file.

When we did the D5 -> D6 CCK updates we ran into this because the content module had to create some tables and the field module could not be upgraded before those tables existed. And the code to create the tables was complex. Plus some of the field modules could not be updated if they were not enabled because they needed to use some complex functions from their own code.

The solution ended up using the #abort option #211182: Updates run in unpredictable order, which let us test if the module was enabled and abort the update if it was not. Later, when the module is enabled, the admin has to come back and run update.php again to process the update. That was what we ended up using for the CCK upgrade path -- lots and lots of updates that started with some testing that everything was in place for the update to work correctly and a switch to abort the update if not.

So that is one option here, to use #abort on these updates if the module is disabled. The disadvantage of this is that we need to be sure the admin does come back and run the update later, but that's true of all updates. The advantage is that it does not auto-enable anything that was presumably turned off for a reason.

We had a long discussion in another thread about force-enabling modules that were disabled and there were mixed feelings about doing it. For contrib modules you had no way to be sure they were available to be enabled. For core modules that isn't a problem. The question is what will happen when you enable that module during the update, will it do anything we don't really want done? I'm not sure, just something to consider and test.

KarenS’s picture

Oh, and we also used module_load_include() in some updates rather than enabling the module to make the functions available. That will work if the functions are self-contained and doesn't involve actually enabling the module.

Doing module_enable() during an update starts a whole chain reaction of other things that maybe shouldn't happen in the middle of an update. Even if this doesn't create problems in our simple testing, it could have other consequences that will only appear later. See #228860: Upgrading a module with a newly added dependencies breaks things badly for some of the discussion about problems enabling modules during updates.

David_Rothstein’s picture

Oh, and we also used module_load_include() in some updates rather than enabling the module to make the functions available. That will work if the functions are self-contained and doesn't involve actually enabling the module.

Yeah, just including the .module file is the best option when it works, but unfortunately there are situations where it doesn't :( I haven't checked, but I'm guessing the taxonomy_get_vocabularies() situation mentioned here might be one of them, since it uses the entity system, and taxonomy module has an implementation of hook_entity_info() which needs to get called but won't if the module is disabled, etc.

See also #734710: API functions that invoke hooks don't work as expected when a disabled module is being uninstalled

***

@scor, you mentioned contact.module fails too - how is that one failing? I'm guessing it might be this:

<?php
/**
 * Enable the 'access user contact forms' for registered users by default.
 */
function contact_update_7002() {
 
user_role_grant_permissions(DRUPAL_AUTHENTICATED_RID, array('access user contact forms'));
}
?>

If so, see #737816: user_role_grant_permissions() throws PDOException when used with non-existent permissions (e.g. a permission for a disabled/uninstalled module)

ericduran’s picture

I used module_enable() because taxonomy_vocabulary_load_multiple() which calls entity_load() fails unless the module is enable. I try almost every function (drupal_load(), module_load_include()) before resorting to module_enable().

Also I called module_enable with out enabling dependents module in order to keep it to a minimum. I'm not sure any module would call dependents function during the updates.

My first attempt was to ignored any modules that was disable but then I realized there's no guarantee the admin would re-run the update after enabling the module and I assume we wanted to run all the updates.

It seems that #734710: API functions that invoke hooks don't work as expected when a disabled module is being uninstalled is related to this and it might actually fix this problem that's happening here.

scor’s picture

@david: that's right. To reproduce, simply enable and disable the contact module on D6 and run the upgrade. it's failing with:

Update #7002
    * Failed: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'module' cannot be null

This is what #773792: contact_update_7002() depends on user_update_7006() fixed (for when contact is enabled only), see #737816: user_role_grant_permissions() throws PDOException when used with non-existent permissions (e.g. a permission for a disabled/uninstalled module) for the case where it's disabled.

catch’s picture

On phone so can't review code but it shouln't be difficult to replace taxonomy_get_vocabularies() with a direct query. Don't think this can be solved generically, at least nott
this release.

scor’s picture

KarenS’s picture

So instead of checking in every individual update hook whether modules are enabled and temporarily enabling if not, another way to approach this would be to take care of this all at once before doing any updates. So the first step of the update would be to collect a list of modules that have updates and see which of them are not currently enabled. Temporarily enable them all, run all the updates, and then disable the ones that were not previously enabled.

The reason I think this is a better approach is that enabling a module involves some system work to update and cache the list of available modules. Doing that over and over as the updates are processed will involve more work, and you have to keep flushing the module list to correct it. Fixing this all one time before running the updates seems like it should be much more performant.

I'm still wondering if there will be problems created from enabling modules that were disabled and I guess that depends on what things each of them is doing in hook_enable. But whatever it is, doing all the enables one time at the beginning is likely to create fewer problems than interrupting the update numerous times to enable and later disable specific modules.

KarenS’s picture

There is another critical issue at #228860: Upgrading a module with a newly added dependencies breaks things badly which is discussing the problems that happen if an enabled module has a dependency and the module it is dependent on is disabled. So one example of the problem of auto-enabling modules that are disabled during the update is that they might have been disabled because they have missing dependencies. If you auto enable them during the update, you could create havoc.

catch’s picture

Priority:Critical» Normal
Status:Needs review» Needs work

@scor: the generic solution is never to use API functions in hook_update_N(), it's a documented limitation, albeit easy to forget. I opened #788370: taxonomy_update_7002() fails when taxonomy module is disabled for the taxonomy update, downgrading this to normal otherwise.

jbrown’s picture

Priority:Normal» Critical

@catch: this is a definitely critical issue as D7 can't be released until it is fixed. Why create another issue for this bug? Its not just taxonomy that is affected.

Modules should not be updated when they are not fully enabled. A module's update functions should assume that the module is enabled.

A module should either fully enabled or not at all. Just including a module file should never be attempted.

Enabling all modules that need updating could potentially enable a considerable number of modules and hit the memory limit.

Ideally what we would do is change update_get_update_list() to only update modules that are currently enabled - other modules could be updated when they are re-enabled. This would remove a lot of problems.

However, this is not possible due to hook_update_dependencies(). The notion that a module is not dependant on another, except at update time is ridiculous.

hook_update_dependencies() should only be implemented for core modules that are required.

So - I recommend

  • hook_update_dependencies() not be implemented for comment, contact and taxonomy. Some other solution should be found
  • Only run update hooks for modules that are enabled.
catch’s picture

Priority:Critical» Normal

jbrown, please see #194310: Check / run updates of disabled modules. There is a reason for the current situation, the limitations are documented, current behaviour is exactly the same as D6, hence, not a release blocker. I don't like either the status quo or reverting back to D5 behaviour much, but at the moment, any improvement in one direction makes this worse in another.

David_Rothstein’s picture

Priority:Normal» Critical

@jbrown, for hook_update_dependencies() see #717834: The dependencies declared in core's hook_update_dependencies() implementations aren't actually correct. I'm not sure it has much to do with this issue, though. I think we do need to avoid having required core modules updates depend on optional core module updates (since that makes no sense), but the other way around there's nothing wrong with. The whole point is for optional modules (e.g. contrib modules) to use that hook.

I think this issue needs to stay critical until (at least) there is a critical spinoff for the contact.module issue, which I don't think there is yet (#737816: user_role_grant_permissions() throws PDOException when used with non-existent permissions (e.g. a permission for a disabled/uninstalled module) or some other one). I guess that one could be fixed via a direct database query also if it had to, but it's getting pretty ugly.

jbrown’s picture

Yeah - I suspect this will just get fixed on a case-by-case basis to get D7 out the door.

The module system needs to be totally refactored for D8.

catch’s picture

Yep, I agree it's crap, I just don't think it can be fixed without a complete refactoring, I'll stop playing priority pong too.

KarenS’s picture

I still think that auto-enabling modules to run the update is going to lead to another set of problems. The more I think about it the more I worry that it will lead to serious problems.

For instance, I have some modules that I disabled because they have fatal errors caused by some bug or interaction with other modules. I plan to re-enable them once the bug is fixed, but I would NOT want core auto-enabling them during an update because that will break the update with fatal errors.

Ditto if core tries to enable modules that have missing dependencies.

I just do not think it is a good idea for core to start enabling modules that have been disabled.

The only safe path forward is that modules should make every attempt to write update code that will work even if the module is disabled. If that is not possible, they should abort their updates until the module is enabled.

This is what we did in D6, we can also do it in D7, and then hopefully someone can figure out a better system for D8.

webchick’s picture

Version:7.x-dev» 8.x-dev

Yeeeaahh. This sounds like a huge can of worms that we do not need to be getting into this late in the release cycle. Folks found ways to work around in D6, they'll do so in D7 as well. Let's try again in D8. I committed #788370: taxonomy_update_7002() fails when taxonomy module is disabled instead.

But it would definitely be good to document somewhere (I guess hook_update_N()) that these hooks will run even for disabled modules, and so you should take care to not call module API functions unless you do an explicit module_load first. Would someone be able to open a separate documentation issue for that? I don't want to derail this one, which has some good information on the problem and associated challenges.

David_Rothstein’s picture

OK, but moving this to D8 means we need a critical D7 issue for the contact module as well - I created one here:
#794184: contact_update_7002() fails when the contact module is not enabled

I also created a D7 issue for better hook_update_N() documentation, but it's not going to be so simple to document.... Explicitly loading the .module file only makes some of your module's functions work correctly, not all of them, and there are a number of standard Drupal API functions that won't work correctly either no matter what you do. Anyway, here's the issue:
#794192: Documentation of hook_update_N() should explain that certain functions cannot be called from there

beejeebus’s picture

this is absolutely $%#!@* crazy. putting working on this mess in the D8 cycle on my list.

willmoy’s picture

Issue tags:-D7 upgrade path
bjaspan’s picture

subscribe

Bojhan’s picture

Priority:Critical» Major
Damien Tournoud’s picture

Version:8.x-dev» 7.x-dev

Bugs cannot target D8, as the tree is not open yet.

webchick’s picture

Version:7.x-dev» 8.x-dev
Category:bug» task
catch’s picture

catch’s picture

Component:ajax system» database update system
tim.plunkett’s picture

Priority:Major» Normal

#1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed seems like a parent or blocker for this issue, and they don't both need to be major.

David_Rothstein’s picture

catch’s picture

#2055605: Disallow use of plugins during update is still open. This might be a duplicate of that now though.