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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

coofercat’s picture

Title: Module Uninstall Does not Follow Module Dependencies » Module Enable/Uninstall Does not Follow Module Dependencies

Actually, 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 :-(

coofercat’s picture

Component: base system » system.module

Sorry, forgot to move to system.module, which is the component at fault.

douggreen’s picture

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

coofercat’s picture

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

Gábor Hojtsy’s picture

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

chx’s picture

Status: Active » Postponed (maintainer needs more info)

So is it broken on enable?

coofercat’s picture

Version: 6.x-dev » 6.1
FileSize
1022 bytes

I'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... ;-)

David Lesieur’s picture

Title: Module Enable/Uninstall Does not Follow Module Dependencies » Module Uninstall Does not Follow Module Dependencies
Version: 6.1 » 6.8
Status: Postponed (maintainer needs more info) » Active

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

jfxberns’s picture

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

Dave Reid’s picture

Title: Module Uninstall Does not Follow Module Dependencies » Uninstalling modules does not follow dependencies
Version: 6.8 » 7.x-dev

Need to confirm on HEAD first, then backport.

dealancer’s picture

subscribing.

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.

hass’s picture

+

Dave Reid’s picture

Priority: Normal » Major

Bumping to major.

Dave Reid’s picture

Component: system.module » base system
Assigned: Unassigned » Dave Reid
Status: Active » Needs review
FileSize
1.27 KB

I think it's just this easy? Although we'll want to add tests.

David_Rothstein’s picture

I 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:

  • In addition to ensuring the correct order of dependencies, we also need to ensure their presence. Uninstalling taxonomy.module without also trying to uninstall forum.module (or having already uninstalled it) is the same bug - bad things will happen if and when forum.module's update hooks or uninstall hook are called later on.
  • We therefore need to prevent this in the UI also (the checkbox for uninstalling taxonomy needs to be disabled unless forum has been uninstalled.)
  • Because the UI must enforce a specific order in which modules need to be uninstalled, we need to make all disabled modules appear on the uninstall page, not just a limited set which implement certain hooks. (However, if you look at what drupal_uninstall_modules() actually does, the old behavior of hiding certain modules on the uninstall page didn't make sense anyway and was a bug in and of itself.)
  • The new behavior of drupal_uninstalled_modules() enforcing dependencies should be controlled by a parameter, similar to the way the behavior of module_enable() and module_disable() is.

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.

pwolanin’s picture

Looking at this, I'm trying to remember why in the world we are running updates on disabled modules.

Dave Reid’s picture

How would you be able to disable taxonomy module without disabling forum? I don't get why we have to make changes to the UI.

David_Rothstein’s picture

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

effulgentsia’s picture

#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?

David_Rothstein’s picture

Thanks. 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 :)

sun’s picture

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

David_Rothstein’s picture

If both modules are already disabled, then both modules can be uninstalled in one go.
.....
Taxonomy cannot be disabled on its own, because Forum requires it.

To 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.)

Jacine’s picture

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

David_Rothstein’s picture

Er, I obviously read @sun's sentence incorrectly. He wrote this:

Taxonomy cannot be disabled on its own, because Forum requires it.

Which I read and responded to as if it said this:

Taxonomy cannot be uninstalled on its own, because Forum requires it.

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.

David_Rothstein’s picture

Hm, 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.

Jacine’s picture

FileSize
35.6 KB

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

David_Rothstein’s picture

Oh, 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 :)

Jacine’s picture

"To uninstall Taxonomy, the following modules must be uninstalled first:"

Great! That's even better :D

David_Rothstein’s picture

Rerolled the patch to chase HEAD, and to make the changes to the user interface that were discussed above (shown in the attached screenshot).

catch’s picture

This looks sensible to me.

+    $profile = drupal_get_profile();
     while (list($module) = each($module_list)) {
       if (!isset($module_data[$module]) || !$module_data[$module]->status) {
         // This module doesn't exist or is already disabled, skip it.
@@ -477,7 +478,7 @@ function module_disable($module_list, $d
       // Add dependent modules to the list, with a placeholder weight.
       // The new modules will be processed as the while loop continues.
       foreach ($module_data[$module]->required_by as $dependent => $dependent_data) {
-        if (!isset($module_list[$dependent]) && !strstr($module_data[$dependent]->filename, '.profile')) {
+        if (!isset($module_list[$dependent]) && $dependent != $profile) {
           $module_list[$dependent] = 0;

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.

David_Rothstein’s picture

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.

What 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?

sun.core’s picture

#29: 151452-uninstall-order-D7-29.patch queued for re-testing.

effulgentsia’s picture

Title: Uninstalling modules does not follow dependencies » Uninstalling modules does not follow dependencies and can lead to WSOD
Priority: Major » Critical
Issue tags: +API change

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

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

I 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!

webchick’s picture

Status: Reviewed & tested by the community » Needs review

T is an important part of RTBC. But thanks for doing a code review!

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
20.49 KB

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

threewestwinds’s picture

Edit: 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!

tstoeckler’s picture

Re #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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 151452-uninstall-order-D7-36.patch, failed testing.

Berdir’s picture

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

threewestwinds’s picture

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

crimsondryad’s picture

Status: Needs review » Needs work
Issue tags: +String freeze, +API change

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

catch’s picture

Status: Needs work » Needs review
Issue tags: -String freeze, -API change

#36: 151452-uninstall-order-D7-36.patch queued for re-testing.

The last submitted patch, 151452-uninstall-order-D7-36.patch, failed testing.

carlos8f’s picture

Status: Needs work » Needs review

subscribing

David_Rothstein’s picture

#36: 151452-uninstall-order-D7-36.patch queued for re-testing.

David_Rothstein’s picture

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

mattyoung’s picture

~

Status: Needs review » Needs work
Issue tags: -String freeze, -API change

The last submitted patch, 151452-uninstall-order-D7-36.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
Issue tags: +String freeze, +API change

#36: 151452-uninstall-order-D7-36.patch queued for re-testing.

David_Rothstein’s picture

Status: Needs review » Reviewed & tested by the community

Whew... finally!

coofercat’s picture

Yes indeed - *phew*

Thank you to everyone who's worked on this. It's very much appreciated!

webchick’s picture

Priority: Critical » Normal

This 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?

-      return '<p>' . t('The uninstall process removes all data related to a module. To uninstall a module, you must first disable it on the main <a href="@modules">Modules page</a>. Not all modules support this feature.', array('@modules' => url('admin/modules'))) . '</p>';
+      return '<p>' . t('The uninstall process removes all data related to a module. To uninstall a module, you must first disable it on the main <a href="@modules">Modules page</a>.', array('@modules' => url('admin/modules'))) . '</p>';

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.

sun’s picture

If you turn on Blog module and turn it back off again, you don't get an uninstall feature.

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

David_Rothstein’s picture

Priority: Normal » Major

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.

You 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.)

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

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

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ah, 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!

rfay’s picture

OK, 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!

tstoeckler’s picture

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

David_Rothstein’s picture

Right, 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.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

fago’s picture

Related, 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).