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.
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.
Comment | File | Size | Author |
---|---|---|---|
#4 | 773828.patch | 991 bytes | ericduran |
#3 | 773828.patch | 990 bytes | ericduran |
Comments
Comment #1
prabhu.k CreditAttribution: prabhu.k commentedgood
Comment #2
jbrown CreditAttribution: jbrown commentedReproduced.
Comment #3
ericduran CreditAttribution: ericduran commentedI'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.
Comment #4
ericduran CreditAttribution: ericduran commentedminor style clean-up
Comment #5
KarenS CreditAttribution: KarenS commentedWe 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.
Comment #6
KarenS CreditAttribution: KarenS commentedOh, 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.
Comment #7
David_Rothstein CreditAttribution: David_Rothstein commentedYeah, 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:
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)
Comment #8
ericduran CreditAttribution: ericduran commentedI 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.
Comment #9
scor CreditAttribution: scor commented@david: that's right. To reproduce, simply enable and disable the contact module on D6 and run the upgrade. it's failing with:
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.
Comment #10
catchOn 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.
Comment #11
scor CreditAttribution: scor commented@catch we should try to find a more generic solution that works for all cases, such as #737816-7: user_role_grant_permissions() throws PDOException when used with non-existent permissions (e.g. a permission for a disabled/uninstalled module).
Comment #12
KarenS CreditAttribution: KarenS commentedSo 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.
Comment #13
KarenS CreditAttribution: KarenS commentedThere 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.
Comment #14
catch@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.
Comment #15
jbrown CreditAttribution: jbrown commented@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
Comment #16
catchjbrown, 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.
Comment #17
David_Rothstein CreditAttribution: David_Rothstein commented@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.
Comment #18
jbrown CreditAttribution: jbrown commentedYeah - 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.
Comment #19
catchYep, 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.
Comment #20
KarenS CreditAttribution: KarenS commentedI 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.
Comment #21
webchickYeeeaahh. 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.
Comment #22
David_Rothstein CreditAttribution: David_Rothstein commentedOK, 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
Comment #23
Anonymous (not verified) CreditAttribution: Anonymous commentedthis is absolutely $%#!@* crazy. putting working on this mess in the D8 cycle on my list.
Comment #24
willmoy CreditAttribution: willmoy commentedComment #25
bjaspan CreditAttribution: bjaspan commentedsubscribe
Comment #26
Bojhan CreditAttribution: Bojhan commentedComment #27
Damien Tournoud CreditAttribution: Damien Tournoud commentedBugs cannot target D8, as the tree is not open yet.
Comment #28
webchickComment #29
catchRelated to #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed.
Comment #30
catchComment #31
tim.plunkett#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.
Comment #32
David_Rothstein CreditAttribution: David_Rothstein commentedIs there anything to do here that wasn't already covered by #2001310: Disallow firing hooks during update and #794192: Documentation of hook_update_N() should explain that certain functions cannot be called from there?
Comment #33
catch#2055605: Disallow use of plugins during update is still open. This might be a duplicate of that now though.
Comment #46
quietone CreditAttribution: quietone at PreviousNext commentedI happen to notice this issue while searching for something else.
This was filed on Drupal 6 and is about upgrading to Drupal 7 so maybe this should go back to the Drupal 7 issue queue? 10 years ago catch thought this was a duplicate of #2055605: Disallow use of plugins during update. That was closed as won't fix but had a followup of #2193673: Let minor updates invoke hooks again. That was closed as a duplicate of #2233403: Let update.php invoke hooks again which was fixed, committed to 8.x, 9 years ago.
Based on that I am going to close this as outdated. I am sure someone will correct that if I am wrond.