Fatal error when uninstalling Shortcut module

I am getting the same.

Adding the following to the shortcut.install file fixes the problem:

if (!function_exists('shortcut_sets')) {
  function shortcut_sets() {
    return db_select('shortcut_set', 'ss')
    ->fields('ss')
    ->execute()
    ->fetchAllAssoc('set_name');
  }
}

Related to #1029606: Regression: Not loading the .module file causes a fatal error when uninstalling some modules (as does loading it). I'm not sure if we should mark this as a duplicate or not.

After a visual review of some of the uninstall files, this seems redundant in the forum module:

forum.install

/**
 * Implements hook_uninstall().
 */
function forum_uninstall() {
  // Load the dependent Taxonomy module, in case it has been disabled.
  drupal_load('module', 'taxonomy');

  variable_del('forum_containers');
  ....
}

And this should cause an error in the simpletest module, unless class autoloading saves the day, which was the reason that the module_load_include() was removed in the first place.

simpletest.install

/**
 * Implements hook_uninstall().
 */
function simpletest_uninstall() {
  simpletest_clean_environment();

  // Remove settings variables.
  ....
}

Just reporting that I've also run into this issue and it is easily duplicated as outlined in the original post.

Subscribing

A quick one off hack for subscribers, simply add the following line to the top of the uninstall function of the problematic module:

module_load_include('module', 'shortcut');

D'oh.

I presume there's some reason .module files aren't loaded when their uninstall hooks run, so I opted not to circumvent that. I'll let smarter people determine what the best approach is.

Also, will someone go kick the test robot, please?

There was no real reason for the removal of the include other than to tidy things up after hook_menu() was removed. Sadly a simple API change that caused this bug. There should be zero danger of including the module file.

By including logic in the install file results in duplication of code, which means this then needs to be maintained twice, so I would favor the include approach.

Test Bot had issues yesterday. But it's passed since then (looks like someone kicked it).

I concur with Alan D. that code duplication is not ideal. If including the .module file won't cause problems, then that is the preferable approach.

The patch was already made against D7 core, and also applies cleanly to HEAD of 8.x (I just tested). So removing the tag.

The tag is a reminder for committers to not forget to commit this patch to D7 and for reviewers to track the correct issues. Re-adding tag.

Since Shortcut module wasn't the only one broken by this change (not even in core, let alone contrib!) I think we should postpone this on the discussion in #1029606: Regression: Not loading the .module file causes a fatal error when uninstalling some modules (as does loading it), so we can figure out how to solve this consistently across all modules. We don't want to keep it postponed for too long, though, since this ideally should be fixed before 7.3 is released.

In terms of this issue, either patch works, but I think I prefer Garrett Albright's approach a bit. The automatic loading of .module files was removed because it was breaking some modules' uninstallation (ones that defined classes that depended on other modules). That's not an issue for shortcut.module at the moment, but if we're saying it's bad practice to load a disabled module's .module file we probably should follow that here too. We already have similar conventions for hook_update_N(), so it seems like it would make sense to apply the same convention here. Let's wait and see what happens with the discussion in the other issue.

This probably also needs tests, although the goal of the other issue is to have a test that handles this for all core modules at once, so it might be covered by that one instead.

Subscribe. Annoying to have to do this dance every time I want to uninstall some of the core modules.

Based on discussion in #1029606: Regression: Not loading the .module file causes a fatal error when uninstalling some modules (as does loading it) it sounds like we should go with the approach of including the .module file, so let's reopen this.

But I'm pretty sure to be consistent with general usage we should use drupal_load() rather than module_load_include() here?

This is now a duplicate, last patch in the other issue does this alrdeady.

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