Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Fatal error: Call to undefined function shortcut_sets() in /path/to/drupal/modules/shortcut/shortcut.install on line 36
How to replicate:
- Fresh 7.2 install
- Disable shortcut module
- Uninstall shortcut module
If using overlay the error flashes briefly before redirecting to /admin/modules, turn overlay off to see the full error message.
Comment | File | Size | Author |
---|---|---|---|
#12 | 1169894-uninstall-shortcut.patch | 490 bytes | alexweber |
#9 | 1169894-uninstall-shortcut.patch | 832 bytes | Garrett Albright |
#7 | 1169894-uninstall-shortcut.patch | 0 bytes | Garrett Albright |
Comments
Comment #1
jamestombs CreditAttribution: jamestombs commentedI am getting the same.
Adding the following to the shortcut.install file fixes the problem:
Comment #2
alexweber CreditAttribution: alexweber commentedIt'll work but the proper way to handle this would be to make sure shortcut.module is included at that point and, if not, include it.
Comment #3
Alan D. CreditAttribution: Alan D. commentedRelated 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
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
Comment #4
Devin Carlson CreditAttribution: Devin Carlson commentedJust reporting that I've also run into this issue and it is easily duplicated as outlined in the original post.
Comment #5
Andrew Schulman CreditAttribution: Andrew Schulman commentedSubscribing
Comment #6
Alan D. CreditAttribution: Alan D. commentedA quick one off hack for subscribers, simply add the following line to the top of the uninstall function of the problematic module:
Comment #7
Garrett Albright CreditAttribution: Garrett Albright commentedComment #9
Garrett Albright CreditAttribution: Garrett Albright commentedD'oh.
Comment #11
alexweber CreditAttribution: alexweber commentedI'm unclear as to what the "best" solution here is: should we manually load the .module file as proposed in #6 or make a db call to get the info as proposed by #9?
Despite accessing the db being the more practical route its less future-proof as the "correct" way would be to use the API whenever possible.
Comment #12
alexweber CreditAttribution: alexweber commentedThis should work :)
Comment #14
Garrett Albright CreditAttribution: Garrett Albright commentedI 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?
Comment #15
Alan D. CreditAttribution: Alan D. commentedThere 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.
Comment #16
alexweber CreditAttribution: alexweber commentedAgreed!
On a lighter note, why did the test bot spit at my patch?
Comment #17
Garrett Albright CreditAttribution: Garrett Albright commentedTest 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.
Comment #18
webchickComment #19
Garrett Albright CreditAttribution: Garrett Albright commentedThe patch was already made against D7 core, and also applies cleanly to HEAD of 8.x (I just tested). So removing the tag.
Comment #20
tstoecklerThe 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.
Comment #21
David_Rothstein CreditAttribution: David_Rothstein commentedSince 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.
Comment #22
geerlingguy CreditAttribution: geerlingguy commentedSubscribe. Annoying to have to do this dance every time I want to uninstall some of the core modules.
Comment #23
David_Rothstein CreditAttribution: David_Rothstein commentedBased 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?
Comment #24
aspilicious CreditAttribution: aspilicious commentedThis is now a duplicate, last patch in the other issue does this alrdeady.