Apologies if this is in the wrong "component" - there doesn't seem to be a "modules system".
I have noticed a problem when uninstalling dependent modules (the problem has existed since 5.x when module dependencies were introduced). Essentially, Drupal does not follow the order of dependencies when uninstalling, which may cause problems with heavily dependent modules.
For example, say you have two modules, module1 and module2. Module1 provides a complex API to other modules, and perhaps uses Drupal variables in some way for dependent modules. When Module2 is installed, it's module2.install->install() function calls the API of module1, which in turn (for example) places some variables into Drupal.
On uninstall, what should happen is that module2 is uninstalled first. As it does so, it's uninstall() function calls the API of module1 to remove whatever resources it has. In this example, module1 would be removing some Drupal variables to completely remove all traces of module2 on the system. After this has completed, module1 would then be uninstalled, removing it's API from the view of other modules.
This doesn't happen with Drupal 6. Instead, an apparently arbitrary order of uninstall means that module1 can be uninstalled before module2, which means that module2 has no access to module1's API, so could end up leaving resources in the system.
This problem only affects heavily dependent modules, and AFAIK, none of the core/optionals. That said, it's presumably not too complicated to fix, and has no implications for the user experience etc.
I'll look at producing a patch, but I'm deep in other stuff right now, so may not get a chance for a while. More as soon as I can...
Comment | File | Size | Author |
---|---|---|---|
#36 | 151452-uninstall-order-D7-36.patch | 20.49 KB | tstoeckler |
#29 | 151452-uninstall-order-D7-29.patch | 20.49 KB | David_Rothstein |
#29 | uninstall-forum-taxonomy.png | 32.9 KB | David_Rothstein |
#26 | uninstall-x-first.png | 35.6 KB | Jacine |
#15 | 151452-uninstall-order-D7-15.patch | 20.14 KB | David_Rothstein |
Comments
Comment #1
coofercat CreditAttribution: coofercat commentedActually, it's worse than this... It affects the install routine too.
To reproduce, select two dependent modules, and click enable. The order they are installed seems to be top-to-bottom as seen on screen. If this is not in the order of dependencies, then of course, you'll get installation problems.
It seems we need a recursive function that works out the order of dependencies/dependents. This feels a bit nasty - anyone got any better ideas?
The obvious workaround is of course to only install one module at a time. Not really ideal though :-(
Comment #2
coofercat CreditAttribution: coofercat commentedSorry, forgot to move to system.module, which is the component at fault.
Comment #3
douggreen CreditAttribution: douggreen commentedIf this issue is about disabling, and not uninstalling, then I think that this is a duplicate of #168487, which has already been fixed. Is this a dup?
If a module is dependent on another module that can be disabled, then it should put
if (module_exists('somemodule')) { ... }
around the dependent code.Comment #4
coofercat CreditAttribution: coofercat commentedThis issue is actually about the uninstall step, not disabling. Disabling just removes the module from Drupal's execution paths, but leaves the module's resources in place.
I haven't tested the patch you mention, but I can't see it doing anything for uninstall (although provides a good template of what to do). The uninstall needs to follow the dependency trail just like enable/disable does. So this bug isn't a duplicate, I'm afraid, although it's vaguely possible that it's fixed by the patch you mention.
Comment #5
Gábor HojtsyIf it is broken on enable, then it is definitely news, and definitely should be fixed there.
Uninstall is a completely different beast. If you could only disable a module once you uninstalled all the modules dependent on it, then you would be forced to loose data just to disable some modules you would not like to use for some time. What if I install Organic groups to start setting up some groups. This module requires Views. Now I can disable both modules, and get back to them later, having my Organic Groups and Views data intact. Now if I an only disable Views if I first uninstall Organic Groups, then I become very sad for my data loss.
Comment #6
chx CreditAttribution: chx commentedSo is it broken on enable?
Comment #7
coofercat CreditAttribution: coofercat commentedI've been doing a bit of work in this area recently, so can now demonstrate the problem with a handy test case (attached). I can also confirm all of this is (still) true in Drupal 6.1.
The test case is three modules: apple, banana and cherry. Apple and cherry both depend on banana. To demonstrate the problem:
1) tick the "enable" checkbox on all three modules, and press "save configuration"
2) Observe the information and errors: Neither cherry not apple find the banana module during their hook_install() phases
3) untick "enabled" on apple and cherry (press "save configuration")
4) untick "enabled" on banana (press "save configuration")
5) Click the "uninstall" tab
6) Tick all three modules and press "uninstall"
7) Observe errors: apple does not find the banana module present during it's hook_uninstall()
(try again, but enable banana first, then come back to do the others. Also, try uninstalling with banana still enabled, or uninstall apple and cherry with banana disabled).
So in short: The dependency tree is not followed during enable/install.
Disabling and uninstalling is even worse, and falls foul of a UI design problem, as much as not following the dependency tree. In this example, uninstalling apple and cherry require banana to be enabled - however, of course it can be disabled. Thus, it's actually remarkably easy to leave resources behind in modules that depend on each other's APIs to install/uninstall.
For enable/install, I would suggest a fix to the code - it shouldn't be too difficult to have the routine follow the dependency tree.
For uninstall, and it's associated disable issues, it would look like we either need a major design change to the modules administration tool, or else some hefty documentation constraints on what modules can do with each other.
Looking forward to hearing what people think about all of this... ;-)
Comment #8
David Lesieur CreditAttribution: David Lesieur commentedI have done a simpler test, just looking at the order of execution of the install/uninstall hooks.
hook_install() seems to work as expected: it is invoked on dependent modules after required ones.
hook_uninstall() however ignores the dependencies: it appears to be invoked on modules in module name alphabetical order. I also don't see any code in system_modules_uninstall() to handle the dependencies.
So install/enable are apparently fine, but uninstall is not.
Comment #9
jfxberns CreditAttribution: jfxberns commentedI can confirm that this is a problem on install as well - using Drupal 6.9
I just experienced it installing imagecache and imagecache_actions modules at the same time; the dependencies are properly described but the module's .info file.
On installation (only the first time--before imagecache.module has been installed), I get the following error:
Fatal error: Call to undefined function imagecache_action_definitions() in /home/tguide6/public_html/sites/all/modules/contrib/imagecache_actions/imagecache_customactions.install on line 8
The function imagecache_action_definitions() is in the dependent module imagecache.module
See http://drupal.org/node/366990 for a more detailed description of the problem.
Comment #10
Dave ReidNeed to confirm on HEAD first, then backport.
Comment #11
dealancer CreditAttribution: dealancer commentedsubscribing.
I'm using modules which stores tables only in PostgreSQL DB. There are "foreign key" dependences between module1 and module2 tables. module2 table has foreign key of module1 table. When installing modules everything is ok, but when uninstalling, Drupal attempt to uninstall 1st module and then 2nd, and this brings a problems. Module1 table couldn't be deleted first cause of the dependences in module2 table.
So it is a problem.
Comment #12
hass CreditAttribution: hass commented+
Comment #13
Dave ReidBumping to major.
Comment #14
Dave ReidI think it's just this easy? Although we'll want to add tests.
Comment #15
David_Rothstein CreditAttribution: David_Rothstein commentedI was just bitten by this bug earlier today. It's a nasty one.
I think it's not quite as easy as the above, though:
The attached patch implements the above and also adds some tests. I'm also attaching a screenshot of the UI for the disabled checkboxes on the uninstall screen.
Comment #16
pwolanin CreditAttribution: pwolanin commentedLooking at this, I'm trying to remember why in the world we are running updates on disabled modules.
Comment #17
Dave ReidHow would you be able to disable taxonomy module without disabling forum? I don't get why we have to make changes to the UI.
Comment #18
David_Rothstein CreditAttribution: David_Rothstein commentedDave Reid: You don't have to disable them in that order, you just have to disable both of them (forum first, then taxonomy).
And then go to the uninstall screen and uninstall taxonomy.
Actually, we saw this in Drupal Gardens with your module: XML Sitemap :) Someone had disabled all the XML Sitemap modules, but then uninstalled the main XML Sitemap module only (without uninstalling the submodules also). So all the main xmlsitemap database tables were gone. Therefore, when update.php tried to run the submodule's updates... boom.
Comment #19
effulgentsia CreditAttribution: effulgentsia commented#15 looks really good to me. I don't feel comfortable enough with this part of Drupal to RTBC it, but +1.
The bug and fix make total sense to me. We enforce dependency for enabling. We enforce it for disabling. But we do not currently enforce it for uninstalling, and we should. It makes no sense to allow the taxonomy module to be uninstalled while the forum module is disabled, but installed. Even if that particular use-case doesn't cause breakage, the equivalent in contrib-land does, as per #11 and #18.
Re #16: @pwolanin: please open an issue about that if you believe one is needed, but it's unrelated. Even if we didn't run updates on disabled modules, you would still have the foreign key problem of #11, so #15 is still useful. But I suspect one issue with not running updates on disabled modules, is that then what do you do when someone who isn't uid=1 enables an out-of-date module?
Comment #20
David_Rothstein CreditAttribution: David_Rothstein commentedThanks. Yup, regardless of whether or not we run updates on disabled modules, we'd still have issues with hook_uninstall(), which is what the original bug report and #11 are both about. The fact that the same issues can occur with hook_update_N() too is just more fun on top of that :)
Comment #21
sunJust stumbled over this bug in Skinr HEAD: #908946: Port to D7
However, I agree with Dave that the screenshot in #15 does not make sense.
- If both modules are already disabled, then both modules can be uninstalled in one go. All this patch has to ensure is that the modules are actually uninstalled in the proper order of dependencies.
- Taxonomy cannot be disabled on its own, because Forum requires it.
- Forum can be disabled on its own, but can also be uninstalled freely, because there is no dependency.
If I skimmed the patch correctly, then it additionally seems to fix the problem that not all modules can be uninstalled currently (hook_modules_uninstalled() never being called for some).
Comment #22
David_Rothstein CreditAttribution: David_Rothstein commentedTo allow the first via the UI would require some other device besides disabled checkboxes to prevent the second. I don't think we should invent a new UI pattern here. Currently, this patch simply does for uninstalling modules exactly what Drupal already does for disabling modules: It disables the appropriate checkboxes so you are prevented from removing both modules at the same time.
(In other words, if the admin interface already allowed you to disable Taxonomy and Forum at the same time, then it might be reasonable to try to preserve the ability to uninstall them at the same time, but since it doesn't allow the first, there is no reason to try to allow the second either.)
Comment #23
JacineThis bug was very confusing to come across.
I also think the screenshot is #15 is really confusing. I was immediately thrown off by the red "installed" text. The red color usually indicates that a module is disabled and the checkbox indicated the status of the module. It's a big WTF to see "installed" in the first place, but seeing it in red along with the checkbox disabled just makes it more confusing IMO. I honestly never even separated the status of a module being "installed" and "disabled." I think that's a good thing. Why should I need to worry about this?
After figuring out what this screenshot is trying to tell me, I'm left wondering whether the Taxonomy module is enabled or not and what I am supposed to be doing next to rectify the situation.
I think that's probably too technical to expose to users. It would be nice if Drupal would just work as expected without exposing these kinds of WTF's in the UI. If that's not possible, we definitely need to make the process clearer.
Comment #24
David_Rothstein CreditAttribution: David_Rothstein commentedEr, I obviously read @sun's sentence incorrectly. He wrote this:
Which I read and responded to as if it said this:
However, my point is that both statements are true :) We can't ever allow people to uninstall Taxonomy by itself, because that would mean (by definition) that its uninstall hook runs before Forum's ever can, and that is what we are trying to prevent.
Comment #25
David_Rothstein CreditAttribution: David_Rothstein commentedHm, perhaps it would be best to not use any color at all here (neither green nor red). There is no overwhelming reason to, since all referenced modules will be in the same state.
(And I'd love to find a different word than "installed", because it's true that is not really used anywhere else, but I don't know what that word would be.)
I also agree that the disabled checkbox UI is not amazing overall, but again, it's what Drupal already does. Improving it is the job of some other issue. I actually think it's more confusing on the main modules page (where it already exists) than it is here, since at least here, you typically only will have a few modules in the list, so it's easier to track down what you need to do.
Remember also, this page is basically for advanced use cases only. If you're going here, you've already made a choice to be thinking about the difference between disabled and uninstalled modules, nothing to do with this patch.
Comment #26
JacineI don't know. I think if it was for advanced use cases only then it wouldn't be in the UI at all. Yes, you are making a conscious choice to be thinking about the difference between disabled and uninstalled modules, but you if you are on the "Uninstall" page, seeing "installed" and not being able to proceed with the task is at bit of a WTF.
What if you just remove the whole "Required by: Forum" line and replace it with something like: "Uninstall the following module(s) first: Forum?" At least that way, the next step to complete the task is clear, and the disabled checkbox makes more sense.
Comment #27
David_Rothstein CreditAttribution: David_Rothstein commentedOh, I like that a lot!
Maybe something more like this, though:
"To uninstall Taxonomy, the following modules must be uninstalled first:"
It's more words and does not use the active voice, which is usually a no-no but I think it makes sense here to be extra clear; if we're going to flat-out recommend that people uninstall modules that they did not specifically come here to uninstall, we should be careful how we say it (given that it's a dangerous operation). Maybe I am just too paranoid though :)
Comment #28
JacineGreat! That's even better :D
Comment #29
David_Rothstein CreditAttribution: David_Rothstein commentedRerolled the patch to chase HEAD, and to make the changes to the user interface that were discussed above (shown in the attached screenshot).
Comment #30
catchThis looks sensible to me.
I didn't check if it's available here, but elsewhere the stupid install profile dependencies not really being dependencies behaviour is accounted for by checking the type, rather than checking for the actual name of the profile - if it's possible to do that here too that'd at least keep it consistent.
I like the text change from #26-28.
Comment #31
David_Rothstein CreditAttribution: David_Rothstein commentedWhat do you mean by "checking the type"? In the system table, the profile is stored as type 'module' (that's part of the problem), so you can't use that to distinguish them.
Do you have an example of somewhere else in core that does a different kind of check?
Comment #32
sun.core CreditAttribution: sun.core commented#29: 151452-uninstall-order-D7-29.patch queued for re-testing.
Comment #33
effulgentsia CreditAttribution: effulgentsia commentedEscalating to critical, because in addition to affecting UI and strings, it also changes the API of drupal_uninstall_modules(), so if this is going to go in at all, it should do so before RC, and since it can lead to WSOD, it should go in at some point.
An argument for why this shouldn't be critical is that D6 has the same bug, and that few people uninstall modules and of those, only some situations lead to WSOD. But, we're expecting D7 to reach a much larger user base than D6 has, and because of all the usability improvements in D7, we're hoping that user base includes less tech savvy people who've been too intimidated by prior versions of Drupal. So, that means more people will encounter the WSOD, and then not know what to do about it.
Comment #34
tstoecklerI thoroughly reviewed this patch and it is RTBC.
I didn't try it out locally, but it has tests for both the API change in module_uninstall() and for the UI, and since those tests pass, I feel there is no need to.
The patch is well documented, adheres to coding standards and removes another inconsistency from Drupal's module system.
Let's do it!
Comment #35
webchickT is an important part of RTBC. But thanks for doing a code review!
Comment #36
tstoecklerIf you insist :) ...
Tested with taxonomy and forum.
Patch nicely disables the taxonomy checkbox on the uninstall tab until I uninstall Forum first.
Rerolled to remove some fuzz. Literally no changes. I diffed the two files and double checked that I didn't miss anything.
Comment #37
threewestwinds CreditAttribution: threewestwinds commentedEdit: cross post. Oh well.
I did all the local tests I could think of, and was unable to break anything with the patch applied (compared to WSOD without it).
The worst I got was an unhelpful error message - if I try to uninstall a module that's a dependency for another (by enabling the checkbox that has been marked disabled by drupal and hitting submit), it says "No modules selected," when what it really means is that the selection I made was invalid. Just trying to be thorough in my review - don't let this tiny thing hold up a patch that
"is well documented, adheres to coding standards and removes another inconsistency from Drupal's module system."
Let's do it!
Comment #38
tstoecklerRe #37: If at all a bug, that's a form API bug, unrelated to this patch. I don't know if that is worth fixing, but if so, please open a new issue.
Comment #40
Berdir#37/38: This is actually not a bug but working as designed. Setting #disabled really means disabled in Drupal 7, you can't just change the HTML, form api will always use the default value for disabled elements.
Comment #41
threewestwinds CreditAttribution: threewestwinds commentedI agree that it's working as designed, denying me the option to change something I really shouldn't be able to change. I don't think it's worth fixing - it was just something I noted while trying to break things.
Comment #42
crimsondryad CreditAttribution: crimsondryad commentedWe should absolutely update disabled modules. The code still represents a potential vulnerability. Many corporations use Drupal and for PCI type situations, having outdated code on machines is bad. Last year a company web site was hacked because it had a disabled module installed that was outdated (don't ask me how, it happened before I got hired).
Now, a reasonable question would be "why would you keep a disabled module on the site in the first place?" Well, I could come up with some perfectly legitimate reasons, but the reality is that lots of people just don't uninstall stuff that they tried and didn't like. Or they're new and don't realize why it could be an issue. Drupal tries really hard to be secure out of the box, so let's protect people from themselves and help them out.
@ #33
When we are building brand new sites and we migrate them from dev to production, we uninstall devel and other non-prod type modules, or modules that we tried out and didn't work out for us. WSOD isn't end of the world, but it is a frustration that we don't need.
I understand that this is likely an edge type case, but it may be happening to more people than it seems.
Comment #43
catch#36: 151452-uninstall-order-D7-36.patch queued for re-testing.
Comment #45
carlos8f CreditAttribution: carlos8f commentedsubscribing
Comment #46
David_Rothstein CreditAttribution: David_Rothstein commented#36: 151452-uninstall-order-D7-36.patch queued for re-testing.
Comment #47
David_Rothstein CreditAttribution: David_Rothstein commentedBeats me what is causing that failure, but I think I've seen the testbot have problems with that test before. I don't think it can possibly be caused by this patch.
Comment #48
mattyoung CreditAttribution: mattyoung commented~
Comment #50
tstoeckler#36: 151452-uninstall-order-D7-36.patch queued for re-testing.
Comment #51
David_Rothstein CreditAttribution: David_Rothstein commentedWhew... finally!
Comment #52
coofercat CreditAttribution: coofercat commentedYes indeed - *phew*
Thank you to everyone who's worked on this. It's very much appreciated!
Comment #53
webchickThis doesn't need to go in before RC1; it doesn't break the API and fixes an edge case that doesn't render the system unusable. Nevertheless, I'm ok to commit it, but why did we change the string?
If you turn on Blog module and turn it back off again, you don't get an uninstall feature. So as far as I can tell, the previous note is still accurate.
Comment #54
sunI hoped we would have fixed that in the meantime. If that is really still the case, then we have to fix it. All modules have to be uninstalled. Otherwise, nightmare. Ugly stuff like hook_modules_uninstalled() is never invoked, {system}.schema_version never reset to -1, and such. Separate issue.
However, for that sake, I fully support that string change.
Comment #55
David_Rothstein CreditAttribution: David_Rothstein commentedYou don't? I do - I just tried it now and that's how it works for me with the patch applied. (This is included as part of the patch in order to avoid the possibility of a module that needs to be uninstalled, but can't be because it depends on a module that is "uninstallable". And as @sun says, it's a bugfix on its own also.)
Well, it does change the API of drupal_uninstall_modules() a bit. I suppose the only code it could break is code that was already doing something a bit fishy, though (trying to uninstall modules that were not actually able to be safely installed).
Although it's an edge case, I'm pretty sure a WSOD counts as rendering the system unusable :) Let's go with "major".
Comment #56
webchickAh, ok. I didn't try it with this patch before; was going by old HEAD behaviour.
Looks good, fixes both a DX and UX WTF, and a really bad (if you hit it) WSOD bug.
Committed to HEAD. Thanks!
Comment #57
rfayOK, so is this an API change or isn't it. Does it need to be announced? If so, please give a clear change of the implications to users. Thanks!
Comment #58
tstoecklerThe only API change is the behaviour of drupal_uninstall_modules(), which I assume very little modules are calling. I don't know if certain Install profiles call that function. The only user-facing change is that on the module uninstall tab, you have the same kind of dependent-modules-are-disabled behavior that you have on the regular module screen.
Comment #59
David_Rothstein CreditAttribution: David_Rothstein commentedRight, the API change is that if someone called (for example)
drupal_uninstall_modules(array('taxonomy'))
but the forum module (which depends on taxonomy) was not yet uninstalled, this call will no longer succeed; instead it will return FALSE.The reason it fails is that in general it is a dangerous thing to do. But if you want to restore the previous behavior of drupal_uninstall_modules(), you can do so by passing FALSE as the second parameter to the function. This parameter works basically the same as the second parameters of module_enable() and module_disable().
This patch also introduced a performance hit to the default behavior of drupal_uninstall_modules() which can be mitigated by passing in FALSE as well (again, similar to the other module API functions) - although I think it's unlikely that anyone calling this particular function really cares that much about performance.
Comment #61
fagoRelated, there is still an issue with uninstalling modules if dependencies are disabled. See #1029606: Regression: Not loading the .module file causes a fatal error when uninstalling some modules (as does loading it).