It looks like a module file gets included during uninstallation of a module, however at this point the dependencies might be already disabled too. This might lead to fatals when the module is making use of a class from the dependency, as it is the case for profile2 + entity see #1023526: Failure uninstalling Profile 2 if entity disabled.

I don't think drupal should ever include the .module file without the dependencies being met.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rschwab’s picture

Uninstall functions are always in .install files, right? If so, should drupal ever include a .module file when uninstalling, regardless of dependencies?

fago’s picture

>If so, should drupal ever include a .module file when uninstalling, regardless of dependencies?
I don't know whether there is a cause to do so, but if it does it has to make sure dependencies are met.

I just ran over the issue another time with rules, see: #1019982: Uninstallation of rules fails

David_Rothstein’s picture

Interesting bug. It looks like the offending code in drupal_uninstall_modules() is this:

    // First, retrieve all the module's menu paths from db.
    drupal_load('module', $module);
    $paths = module_invoke($module, 'menu');
...
    // Now remove the menu links for all paths declared by this module.
    if (!empty($paths)) {
...

Not really sure what to do about that off the top of my head. It's definitely problematic code.

catch’s picture

Is that code even necessary? We already have:

  // Find any item whose router path does not exist any more.
  $result = db_select('menu_links')
    ->fields('menu_links')
    ->condition('router_path', $paths, 'NOT IN')
    ->condition('external', 0)
    ->condition('updated', 0)
    ->condition('customized', 0)
    ->orderBy('depth', 'DESC')
    ->execute();
  // Remove all such items. Starting from those with the greatest depth will
  // minimize the amount of re-parenting done by menu_link_delete().
  foreach ($result as $item) {
    _menu_delete_item($item, TRUE);
  }
}

in _menu_navigation_links_rebuild();

Damien Tournoud’s picture

It looks to me like bad practice to have a class referencing the class of another module in a .module. Including a module file should be harmless, as much as possible.

Yaron Tal’s picture

Don't you think you should be able to depend on a module if you say you do in your .info? Otherwise you'll have to put everything between if (module_exists()) statements which makes the dependencies statements in the .info useless.

I think #4 is right and the code isn't needed because the menu is rebuild anyway.

Damien Tournoud’s picture

Classes are autoloaded. The .module file should be loadable without side effect in any context. It has always be the case and should remain that way.

fago’s picture

Status: Needs review » Active

@damien:
Just because we can use autoloading for classes, it doesn't mean we have to.

>The .module file should be loadable without side effect in any context. It has always be the case and should remain that way.
While this makes sense, as module developer I'd expect my dependencies to be met when the module is included.

ad #3:
Ouch, so we even invoke code from an uninstalled module, for which the dependencies might not be met. Unfortunately the code in #4 relies upon $path being determined by that.

From a short look, I've seen that there is already a 'module' tracked for each menu item. However, its values seem to be only 'menu' or 'system', but not the module providing the menu item. So if we'd manage to properly track the source module for menu items, existing installs would need an update.

So what about another approach: Just join menu_router with menu_links to identify deprecated links?

fago’s picture

Status: Active » Needs review
FileSize
2.6 KB

ok, let's see what the bot thinks of that.

catch’s picture

Status: Active » Needs review

See #4, is that doing anything that menu_rebuild() doesn't do already?

fago’s picture

ah! Sry I didn't realize this stems from _menu_navigation_links_rebuild() before.

    ->condition('updated', 0)
    ->condition('customized', 0)

The query is a bit different though, so custom menu links would survive the rebuild, but get deleted upon uninstall with #9. That said, I think it still makes sense.

Alan D.’s picture

Isn't this effectively an api change?

I can not remember seeing anything that requires the use of *.install files with Drupal 7 ports. Many small modules use the *.module for the installation hooks which will no longer be included with this change. Also, there could be code sharing between the files, meaning *.install would now need to call drupal_load('module', $module) manually.

IMHO, the bottom line is that it should be safe to always include the *.module file.

Alan D.’s picture

Maybe looping is required to only uninstall modules that are not listed as dependencies during each loop? I think there was a similar installation issue which installed the modules out of sync with dependencies...

drunken monkey’s picture

Isn't this effectively an api change?

Can't say for sure, but using .install files certainly is best practice and what other people reading your modules will expect (as well as being better for performance reasons).

On the other hand it is, I'm pretty sure, nowhere stated that a module needs to explicitly check whether it is enabled when functions inside its .module file are called, which just is a major WTF. I came across this, too, when using my own objects in hook_menu() — during uninstallation, the autoloading of these objects wouldn't work anymore, of course, resulting in a fatal error.

The patch looks good to me, and the test bot is also happy. Probably someone more at home with the menu system should set this to RTBC, though.

bojanz’s picture

Ran into this bug myself.

rszrama’s picture

Just wanting to add a +1 here, as this issue hits us with Commerce modules... we have UI modules that implement bundle edit forms for entities defined in corresponding API modules. So, we're using hook_menu() in w/ a call to the API function, which seems to be playing havoc with general uninstallation. More info, including my disbelief b/c I wouldn't have imagined this being an issue, here: #1017006: Unable to uninstall due to PHP fatals because hook_menu() is called by core when uninstalling

fago’s picture

Patch in #9 is fine afaik, however it could need a review of someone with more knowledge of the menu system.

>Isn't this effectively an api change?
hm, I thought having uninstall/install hooks in .install is a requirement nevertheless. If it isn't, then yes - we would need to keep the "drupal_load('module', $module);" line. The rest of the patch wouldn't be affected though.

mgifford’s picture

I just pulled this down from git to see if I could apply the patch still:

mgifford@asus-laptop:~/rules$ git apply drupal_uninstall.patch
error: install.inc: No such file or directory

Not sure what's up, but would like to be able to reliably uninstall modules when I need to.

bfroehle’s picture

mgifford: git apply -p0 ...

mgifford’s picture

Thanks @bfroehle - I think that this pointed out a few flaws in my previous approach.

I was still thinking of this as a rules issue and not a core issue.

I don't have a git repository (yet) for D7 or D8. I had thought I could keep with the D8 version, but probably not.

Nice to know that the -p0 carries forward from the CVS days! Now I just need to set up a repository for it:
http://drupal.org/node/3060/git-instructions

This needs to be updated so that it redirects to the git or states clearly that it's an archive:
http://drupal.org/documentation/cvs

Thanks!

Damien Tournoud’s picture

Status: Needs review » Needs work
+    $query = db_select('menu_links', 'l')
+      ->fields('l')
+      ->condition('external', 0)
+      ->orderBy('depth');
+    $query->leftJoin('menu_router', 'r', 'l.router_path = r.path');
+    $query->isNull('r.path');

This doesn't look right: at the minimum we need a condition('module', 'system') in there.

But anyway, just forcing a menu rebuild should do all this already. In addition, those modules are *already* disabled, so they should not have any dandling links anyway. I suggest we just completely remove this code.

bojanz’s picture

Status: Needs work » Needs review
FileSize
1.92 KB

Here's a patch that just kills the offending code.

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

Let's pull the trigger.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

What about @fago's comment in #11?

If your site has customized menu links, it seems like after this patch they will remain in the database forever (even after the module they point to has been uninstalled). The consequences of that probably aren't too serious, but it looks like this makes it so we are failing to clean up correctly...

catch’s picture

It's not inconceivable to replace the router path of one module with the router path of another (especially with views or something). In those cases, I'd rather keep the customized menu links then have them cleaned up for me.

Not moving back to RTBC, but I agree with the current patch.

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

When you disable a module, the customized menu links pointing to one of its router paths get orphaned: their 'router_path' column is emptied in the {menu_links} table.

As a consequence, the removed code was *always* a no-operation: there cannot be any menu link with a router_path of a disabled module, and modules have to be disabled before possibly getting uninstalled.

Fago's patch in #9 remove all the orphaned links from the database, regardless which module has initially created them: it will affect all disabled modules, not only those that are getting uninstalled. This is just not acceptable. The only thing we can do is ignore the issue and accept orphaned links to remain in the database forever. As described, it is not a regression. Just accepting the current situation as the best we could possibly do.

David_Rothstein’s picture

Damien, that's not what I see happening at all. When I disable a module, the 'router_path' column for its menu links is not emptied, nor is the uninstall code being removed here a no-op.

Example:

  1. Enable the Forum module.
  2. Customize the 'forum' menu item and rename it to "My site's forum".
  3. Check the {menu_links} table:
    mysql> SELECT menu_name, link_path, router_path, link_title, customized FROM menu_links WHERE link_path = 'forum';
    +------------+-----------+-------------+-----------------+------------+
    | menu_name  | link_path | router_path | link_title      | customized |
    +------------+-----------+-------------+-----------------+------------+
    | navigation | forum     | forum       | My site's forum |          1 | 
    +------------+-----------+-------------+-----------------+------------+
    1 row in set (0.00 sec)
    
  4. Disable the module and run the same query as above. You get the exact same result.
  5. Now uninstall the module and run the query again.
    • Without patch: The above row is gone from {menu_links}.
    • With patch:The above row is still there.

So this patch definitely does have the effect of orphaning menu links that previously would not have been orphaned.

However, I think @catch makes an interesting point. There are cases where this failure to clean up actually winds up being a good thing...

Damien Tournoud’s picture

Status: Reviewed & tested by the community » Needs review

Hm. Not sure what happened in my testing yesterday, you are right. Seems that menu links keep their router path on disabling a module.

Remains that we have no way to know that a given menu link created from a router item belong to a given module. At least not without restoring the module from its grave, and we cannot do that.

Damien Tournoud’s picture

Title: .module file gets included during uninstallation » hook_menu() should not be called during uninstallation

Retitling. The core issue is that we are calling a hook from a disabled module :|

Damien Tournoud’s picture

So I guess we have no choice, then to track the original module the link was created for...

rfay’s picture

subscribe - bit by this.

rfay’s picture

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

We have to get this in to 8.x before it can come back to 7.x. Assigning.

This is a critical for Drupal Commerce.

Anonymous’s picture

This is also affecting SPARQL Views, so subscribe.

catch’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests, +Needs backport to D7

How does original_module help here?

- tracker module defines a link
- admin customises the link (moves it to a different menu, renames or whatever)
- admin enables tracker2 or views and overrides it.
- admin disables then uninstalls tracker.

Looks like the code here would remove my customized link pointing to a valid router item. We could possibly join on menu_router and check if there's a valid router item or not as well, didn't look into this though.

I would go for #22, especially for D7. If it's a choice between data-loss and cruft, I'd go for cruft - it's not as if we don't have plenty of other places in core (like nodes) where uninstalling modules leaves things lying around.

Adding tags.

Berdir’s picture

Got hit by this too. Has nothing to do with a class in my case, just the fact that I'm jusing an API function of a module that I depend on in hook_menu().

Damien Tournoud’s picture

Status: Needs work » Reviewed & tested by the community

I'm all for cruft. Let's get #22 in.

Damien Tournoud’s picture

Issue tags: -Needs tests

We don't need any test for #22. We are just removing broken code.

Dries’s picture

The patch in #22 no longer applies to 8.x and will need a quick re-roll.

Dries’s picture

Status: Reviewed & tested by the community » Needs work
bojanz’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.92 KB

Here's the reroll for 8.x
Setting to RTBC since it's the same patch that got committed to 7.x

Dries’s picture

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

Committed to 8.x. Thanks. Moving to 7.x for @webchick.

Tor Arne Thune’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs backport to D7
Alan D.’s picture

Status: Fixed » Active

The removal of the drupal_load() has cause one (probably two) errors in the uninstallation hooks in core, or 5% of core modules. So if that correlates to 5% of contrib., will there be 300+ broken uninstallation scripts out there.

Related to #1169894: Fatal error when uninstalling Shortcut module, where the shortcut module calls shortcut_sets() from shortcut_uninstall(). Also, not reported, the simpletest module calls simpletest_clean_environment() from simpletest_uninstall(). This should also cause the same error (or some magic autoloading is happening that should be removed if the fix is not to reinclude the main module file during uninstallation).

Simply having the module file included should not cause errors! So the solution is to re-include the module load IMHO. BTW I totally agree with the removal of the call to hook_menu().

Unrelated, but may have been left overs from this or a related issue, the taxonomy module is loaded from the forum_uninstall() for no apparent reason. Any ideas?

catch’s picture

entitycache_uninstall() also broke due to this, but I know other module installs were broken without it due to missing classes from other disabled modules. Either way it's not going to be pretty. We could use a test that installs, uninstalls, then re-installs every core module - that should have caught the shortcut regression.

David_Rothstein’s picture

Title: hook_menu() should not be called during uninstallation » Regression: Not loading the .module file causes a fatal error when uninstalling some modules (as does loading it)

I can confirm that both shortcut module and simpletest module are affected by this (both give a fatal error when uninstalling). From looking at the code, those are hopefully the only two examples in core. Yup, we should definitely have a test for that.

Trying to retitle this issue to encompass both the original problem and the new one... because yes, the original motivation for this issue was that the forced drupal_load() was breaking some modules, and now we know that removing the drupal_load() broke others.

Tough to figure out what to do here, but I think it might be best to just accept that this was an API change and fix the broken modules accordingly? Modules implementing hook_update_N() already have to deal with the fact that they might be disabled when that code runs (and therefore shouldn't use API functions, or at least must load the .module file themselves if they absolutely need to). It would make some sense if the same rules applied to hook_uninstall().

Damien Tournoud’s picture

Title: Regression: Not loading the .module file causes a fatal error when uninstalling some modules (as does loading it) » hook_menu() should not be called during uninstallation
Status: Active » Fixed

We fix bugs, and that uncovers other bugs. This is bound to happen.

The bug is definitely in the shortcut and simpletest modules here. A module cannot rely on having its .module loaded when uninstalling, has it is simply *not enabled* at this time.

Let's fix those and write a test on the lines of what catch has proposed in #44. But let's open a new issue for this. It's nothing more then a bug uncovered while fixing this issue.

David_Rothstein’s picture

Title: hook_menu() should not be called during uninstallation » Regression: Not loading the .module file causes a fatal error when uninstalling some modules (as does loading it)
Version: 7.x-dev » 8.x-dev
Status: Fixed » Needs review
FileSize
17.55 KB

Here's a test that asserts all core modules can be installed and uninstalled. (It's not actually a new test; I just rewrote an existing one that was hardcoded to do this for the Aggregator module only.)

We should expect a few failures here associated with shortcut and simpletest.

Re #46, I mostly agree with that analysis and proposed solution. However, up until Drupal 7.2 modules could rely on the file being automatically loaded, so we introduced a non-backwards-compatible change in this issue, and we know it broke contrib modules too. A possible solution (which I don't agree with either, but which is not unreasonable) would be to add back the drupal_load() during module installation [edit: uninstallation] and preserve backwards compatibility - i.e. to say that the original bug here is basically "won't fix" for D7. That's why it makes sense to continue the discussion in the current issue.

Either way, I think the attached tests are what we want to make pass.

David_Rothstein’s picture

Issue tags: +Needs backport to D7

Missing tag.

Status: Needs review » Needs work

The last submitted patch, module-uninstall-1029606-47.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
18.48 KB

If you can't require_once() a .module file unless the module is enabled there is something wrong with that module. However, between fixing all those modules and requiring some others to add a drupal_load('module', $module); to hook_uninstall(), I'd go for the latter, especially since this is already the case in 7.2.

Attached patch does this for simpletest and shortcut modules. I haven't reviewed the test changes in David's patch though, just confirmed this makes the tests pass.

Status: Needs review » Needs work

The last submitted patch, uninstall.patch, failed testing.

catch’s picture

Oooh interesting, I actually got those locally but assumed it was my crufty local install. Since we weren't testing simpletest uninstallation prior to #47 I assume that's a real bug in simpletest_uninstall().

So let's define what actually needs to be done here, I'd say the following:

- Add some docs to hook_uninstall() to state that .module files aren't included.

- Add some docs to hook_uninstall() to show that the menu link magic removal no longer occurs.

- Add API change documentation on d.o for both of these.

- Fix shortcut_uninstall()

- Fix simpletest_uninstall()

- Add the tests from #47.

sun’s picture

David_Rothstein’s picture

If most of us are in agreement that we should leave the original patch in and go ahead and fix this in the individual modules, perhaps we should reopen #1169894: Fatal error when uninstalling Shortcut module (and get that one fixed right away, without the tests).

Then we can continue to figure out what's wrong with the simpletest module here and add the tests once we get them passing.

 function shortcut_uninstall() {
+  drupal_load('module', 'shortcut');
   // Delete the menu links associated with each shortcut set.
   foreach (shortcut_sets() as $shortcut_set) {

There was some debate in the Shortcut issue about whether or not the above is a good idea. The alternative is to use a direct database query, rather than an API function that requires loading the .module file... Since direct database queries is generally what we recommend for hook_update_N() implementations, my thought was that we should do the same for hook_uninstall().

If we're going to add some documentation in this issue (as per #52) we should figure that out here.

David_Rothstein’s picture

Um, not sure how that happened...

David_Rothstein’s picture

By the way, if I uninstall Simpletest module through the UI, I don't see any issues with the patch applied, so it definitely helps.

Maybe those 4 warnings seen in the tests only appear when you're running the uninstall from inside the testing framework.

catch’s picture

For Drupal 7 I don't think we can enforce drupal_load() vs. direct database queries - we are barely doing that for update functions and this is way after release (core updates still use some API functions, even if it's variable_get()). However if we're going to move to direct database queries (or possibly copying API functions to .install files) then we should stick to that in core at least. I don't really mind either way, IMO it should be safe to include a .module file whether it's enabled or not, and it's up to the module in question to decide whether it's safe to call any of its own API functions, but can live with it if others strongly disagree.

fago’s picture

Not including the .module file by default and leaving it up to the module to use API functions or direct queries sounds good to me + fits to how we handle update functions.

>Since direct database queries is generally what we recommend for hook_update_N() implementations, my thought was that we should do the same for hook_uninstall().

Makes sense, let's better recommend the safe-way and leave the drupal_load() approach for developers who know what they are doing.

Alan D.’s picture

This seems like a big discussion about a single line of code, but I'd just like to raise a couple of points for its' inclusion, in relation to the post removal of the direct call to hook_menu().

  1. When has loading the *.module file triggered a bug?
  2. There should now be no danger of triggering errors by the inclusion of *.module file.
  3. This policy promotes the duplication of code (indirectly).
  4. AFAIK there is no doco on the exact API of all of the loading rules.
  5. Update functions need to be handled differently, by the very nature of these being update functions
  6. Like it or not, it is an API change.

1) I haven't really read the issue queue on a regular basis, but when has loading the *.module file actually caused an error?

2) Since the module hooks are not called once the module is disabled, only "developers who know what they are doing" will have the danger of doing something stupid in the *.module file that results in a direct function call.

i.e.

/**
 * @file *.module
 * My module that does stuff on every page load.
 */
function my_function_in_non_drupal_way() {
  // I'm going to call the API and if I expect a result, I will probably crash and burn during uninstall...
}

my_function_in_non_drupal_way();

All other developers will only code in functions and classes that mean there is no danger in simply including the function.

3) There has already been the suggestion to recreate the db_query()'s in the *.install files in core, creating direct duplication of logic from the *.module file. This has a number of negative issues; increases the amount of code that needs to be maintained, includes new code that will probably have minimal testing, etc.

Although I think the core developers are leaning towards drupal_load_include('module', *) solution, what does this say to contrib maintainers.

4) When can we rely on the module file being included? Since the same arguments for excluding the *.module file during installation as to uninstallation, so should we rely on have access to *.module files during installation in the future? Or only during enable / disable hooks?

Also will hook_schema() and hook_field_schema() be ignored in the future if they don't reside in *.install?

[As an aside, maybe these should be in a *.schema file to minimize the server load]

5) Update functions can not rely on a modules API as they have to be able to span branches that will almost certainly contain different APIs. Because we need to do this here, it doesn't mean it is the best approach.

6) Well it broke the standard behavior that even core developers were relying on. By this fact alone, the include should stay until Drupal 8.

Right, that is about the limit to what I can argue about in relation to changes in a single line of code. :)

catch’s picture

To answer the first question:

module a defines a class/interface

module b implements the interface or extends the class in its .module file

module a and b are disabled.

require_once b.module - fatal error.

To reproduce at the most basic level:

foo.php

<?php
class foo implements bar {};

Then

catch@catch-toshiba:~/www$ php foo.php
PHP Fatal error:  Interface 'bar' not found in /home/catch/www/foo.php on line 3

Both modules don't have to be disabled either - the enabled module could rename its interface, and the user who wants to uninstall the disabled module, is not going to be happy if they have to upgrade the module first just to get it to uninstall.

hook_schema() already has to be in a .install file, it is not guaranteed to work if it's in a .module file. I agree .schema files would be good to save peak memory, there's an issue for that somewhere.

I think we should just do drupal_load() here, at least for shortcut_uninstall() - it is an extremely annoying pattern not being able to use API functions in update hooks, and this is something we should eventually try to reverse rather than propagate everywhere - there's no good reason to not use the API function here unless your module does something weird.

On the API change issue, yes it is one (albeit small), but it's one that's already in Drupal 7.2, so I'm averse to changing it back again.

geerlingguy’s picture

Subscribe.

chrisjlee’s picture

Subscribe

pillarsdotnet’s picture

@#45:

Modules implementing hook_update_N() ... shouldn't use API functions, or at least must load the .module file themselves if they absolutely need to

News to me, and perhaps to others. Posted a comment on the api page.

catch’s picture

Priority: Major » Critical

I'm bumping this to critical since it's a regression from 7.0 to 7.2 (in terms of the API change for contrib modules). Not being able to uninstall a core module is only borderline critical by itself, but I imagine the API change broke more contrib modules than it fixed so we need to get it sorted out and documented, ideally before the next point release in a couple of weeks.

Also the test added here should help with #1115510: Entity providing modules must call field_attach_delete_bundle() in hook_uninstall() (also marked critical) but it's not currently failing for that, however it will help to get that patch in to have the tests in place.

David_Rothstein’s picture

I think we should just do drupal_load() here, at least for shortcut_uninstall() - it is an extremely annoying pattern not being able to use API functions in update hooks, and this is something we should eventually try to reverse rather than propagate everywhere - there's no good reason to not use the API function here unless your module does something weird.

There can actually be some pretty subtle issues due to the module being disabled when the function is called - see #734710: API functions that invoke hooks don't work as expected when a disabled module is being uninstalled

Those issues likely don't affect the cases we're talking about here though. Also, for Shortcut the function we're calling here is pretty simple (just a wrapper around a direct DB query basically), but for Simpletest it's a much bigger function that we really wouldn't want to duplicate in the .install file. So in that case, maybe we just use the drupal_load() method for both, for consistency, and document why you would/wouldn't want to. I think that's fine for moving this issue forward.

@#45:

Modules implementing hook_update_N() ... shouldn't use API functions, or at least must load the .module file themselves if they absolutely need to

News to me, and perhaps to others. Posted a comment on the api page.

Thanks! You might also be interested in #794192: Documentation of hook_update_N() should explain that certain functions cannot be called from there

pillarsdotnet’s picture

Note that I posted in that one too.

catch’s picture

Status: Needs work » Needs review
FileSize
18.78 KB

I found out why the test here doesn't fail for taxonomy module, it's because that bug is more of an edge case than I realised and needs its own test case (which I've now added over there).

Patch should fix simpletest_uninstall(), it is actually trying to delete all its files twice in hook_uninstall() so that is a real but minor bug that the test is picking up one way or another. I don't think we need all the drupal_set_message() malarkey when uninstalling either.

tstoeckler’s picture

I thought I get why you should not call API functions from hook_update_N(), but hook_uninstall() seems different to me. Unlike hook_update_N(), hook_uninstall() should always contain the most recent version of code. Just like when you change your API you might have to change hook_schema(), you also change hook_uninstall(), but not any hook_update_N() (instead you add a new one). Could anyone elaborate on that?

catch’s picture

Right hook_update_N() can't rely on API functions because you can't tell when an update hook will run - so if the API function depends on a schema, that schema may have changed.

In hook_uninstall() it's more complex, any API function that invokes hooks, especially if the hook is implemented by your module, may not include anything added by your module since it could be disabled, a good example of that is #1115510: Entity providing modules must call field_attach_delete_bundle() in hook_uninstall(). However some things will work OK, but it is a big mess and we can't fix it in D7 apart from patching up the worst bits of it.

David_Rothstein’s picture

Patch should fix simpletest_uninstall(), it is actually trying to delete all its files twice in hook_uninstall() so that is a real but minor bug that the test is picking up one way or another.

Hm, I'm not sure I understand that... where is the code that deletes files twice?

Looking at it now, I actually wonder if the problem is just that simpletest_clean_temporary_directories() assumes that 'public://simpletest' will always exist? But if you install the Simpletest module and then immediately uninstall it (as these tests do), without ever having actually run any tests, then that directory will never have been created in the first place. If that's the case, we should actually leave the call to simpletest_clean_environment() in simpletest_uninstall() but fix this within simpletest_clean_temporary_directories() instead.

Besides that, looks like we're pretty close here? (I think we wanted to add some documentation also.)

Since we seem to have a plan of action, I'm going to un-postpone #1169894: Fatal error when uninstalling Shortcut module so we can at least make sure that one-line patch makes it into 7.3 (with or without the rest) - that's probably the ugliest part of the bug since Shortcut is turned on in the default profile so there will be more people (especially less experienced people) trying to uninstall it than Simpletest.

catch’s picture

simpletest_uninstall() runs both simpletest_clean_environment() - which tries to remove files from the simpletest tests directory, then and later calls file_unmanaged_delete_recursive() on the same directory, that's the 'twice'.

Since simpletest_clean_environment() does a lot of stuff that is superfluous to uninstalling the module, so I just copied the database table cleanup from there, that's the only bit of code that's relevant to uninstall that's not already covered by simpletest_uninstall() already.

If we wanted to keep the use of that function in simpletest_uninstall(), I'd want to see it converted to a proper API function without the drupal_set_message() etc. It would also have to deal with removing the directory as well as just emptying it, which it currently doesn't.

I have no interest in doing that as a requirement to having this issue fixed but if you really feel strongly about it then go ahead ;)

Not sure what documentation should be added to core, I was thinking http://drupal.org/update/6/7 for this.

Maybe http://api.drupal.org/api/drupal/modules--system--system.api.php/functio... to explicitly point out that the .module file is not automatically loaded when hook_uninstall() runs and you should drupal_load() it yourself if you need it?

David_Rothstein’s picture

simpletest_uninstall() runs both simpletest_clean_environment() - which tries to remove files from the simpletest tests directory, then and later calls file_unmanaged_delete_recursive() on the same directory, that's the 'twice'.

But the test failure comes from the first attempt, so the duplication can't be the cause of the bug (and it's not really duplication so much as an overkill way to delete the leftover empty directory, as you said). I think to fix the bug directly we just need an is_dir() call or similar.

Ideally we should keep simpletest_clean_environment() in there, since it's the "right" API function to call in order to clean things up; the drupal_set_message()/etc is ugly but cleaning that up is definitely a separate issue.

Not sure what documentation should be added to core, I was thinking http://drupal.org/update/6/7 for this.

Maybe http://api.drupal.org/api/drupal/modules--system--system.api.php/functio... to explicitly point out that the .module file is not automatically loaded when hook_uninstall() runs and you should drupal_load() it yourself if you need it?

Exactly. That is more or less what you suggested in #52 also :)

David_Rothstein’s picture

How about this? It should pass the tests and seems like a direct fix of the bug @catch found. I also added the documentation to hook_uninstall().

catch’s picture

FileSize
19.61 KB

But the test failure comes from the first attempt, so the duplication can't be the cause of the bug (and it's not really duplication so much as an overkill way to delete the leftover empty directory, as you said). I think to fix the bug directly we just need an is_dir() call or similar.

Yeah I'm not claiming the duplication is the cause of the bug, rather that it is needless (and the call to the function happens to cause those exceptions). I really have no interest in looking into it any further - it is four exceptions, for files that don't exist, in simpletest_uninstall(), which are only triggered during test runs, which have nothing whatsoever to do with the critical regression from 7.0-7.2 that this issue concerns. I'm not going to work on it, review patches about it, or discuss it any more, you're welcome to if you like but I also think this issue is not the place to do that.

Added the docs to hook_uninstall(), reads like a lot of apologetics but that's all that can be said for the broken mess that is disabled modules.

catch’s picture

Crossposted. I said I wouldn't review it, but you're still trying to delete some files in a directory, then trying to delete the whole directory if you do it that way.

chx’s picture

Status: Needs review » Needs work

Um. The limitations of hook_uninstall (class foo extends bar_from_already_deleted_module kaboom fatal) is extremely far reaching apparently. We should document at least in hook_uninstall but also possible in the .info files documentation where we speak about files[] = we should recommend putting classes that inherit from elsewhere or no uninstall for you.

catch’s picture

Status: Needs work » Needs review

I opened #1187074: Document that extending/implementing classes can not be safely placed in .module files, hook_uninstall() is by far not the only place that limitation exists, hook_boot() is another.

David_Rothstein’s picture

it is four exceptions, for files that don't exist, in simpletest_uninstall(), which are only triggered during test runs

It isn't limited to test runs. You can see the same PHP warnings in the UI (for example, by clicking the "Clean environment" button). I thought it preferable to get the tests passing by fixing the source of the bug, rather than avoiding it in the uninstall code only.

Anyway, it looks like both patches do pass the tests right now.

catch’s picture

We cold do both, fix the bug in simpletest_clean_environment(), but also not use it in hook_uninstall().

David_Rothstein’s picture

FileSize
20.38 KB

That sounds reasonable. The attached patch does both.

I also merged the docs we wrote for hook_uninstall() (both said almost the exact same thing anyway).

catch’s picture

New docs look great, I don't think I can mark this RTBC since I proposed the drupal_load(), despite the bulk of the patch being David's code, but +1.

chx’s picture

Status: Needs review » Reviewed & tested by the community

While the core patch itself is very small (like, two drupal_load commands), the testing benefit is big and #1187074: Document that extending/implementing classes can not be safely placed in .module files will take care of my documentation concerns which truly do not belong here. Go for it!

catch’s picture

FileSize
20.42 KB

D7 patch does not apply due to #1136130: Regression: Reinstate WATCHDOG_* constants and document why they are necessary., I seem to remember someone pointing out this would happen shortly after it was committed.

Fortunately a find and replace on the patch file works in this case, so here's that.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, uninstall-1029606-80.patch, failed testing.

webchick’s picture

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

Switching version temporarily to 7.x so the testbot can have a crack at that patch.

webchick’s picture

Status: Needs work » Needs review
Issue tags: -Needs backport to D7

#83: uninstall-1029606-80.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs backport to D7

The last submitted patch, uninstall-1029606-80.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
20.42 KB

Here's a patch (I lost count of the number of folllowups hence the file name)

Status: Needs review » Needs work
Issue tags: -Needs backport to D7

The last submitted patch, uninstall_ftl_aleph0.patch, failed testing.

webchick’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D7

#88: uninstall_ftl_aleph0.patch queued for re-testing.

webchick’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Fixed

Ok, those failures are just a random thing that happens with testbot sometimes. See #1188880: Comment module test fails randomly. So I took a chance.

Committed to 8.x and 7.x! Yay!! :D

webchick’s picture

Oops, wait. I lost my comment in all that retesting.

These tests are simply awesome, and EXTREMELY comprehensive. Thanks so much for the hard work on those!!

HnLn’s picture

sub

Status: Fixed » Closed (fixed)
Issue tags: -Needs backport to D7

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