Give that it deals with module installation, it seems reasonable to assume that hook_modules_uninstalled() should go in an install file rather than a module file. (And assumptions are all one has to go on: #633332: hook API documentation files have no information about where hooks are to go.)
This is in fact not the case. I've just encountered this, and a read of http://api.drupal.org/api/drupal/includes--install.inc/function/drupal_u... confirms it.
I would suggest we move its expected location to the install file, by adding a module_load_all_includes('install')l to drupal_uninstall_modules().
Comments
Comment #1
jhodgdonI disagree with this idea.
hook_modules_uninstalled is a way for a module to respond when other modules are installed/uninstalled, so it is not in the same group as enable/disable/install/etc. Yes it is related to installation, but it is not part of that particular module being installed, so IMO it doesn't belong in the .install file.
Comment #2
joachim commentedAs I've always understood it, the install file exists for optimization: code that is only run when big system changes such as code updates or module addition/removal take place.
By that criterion, these hooks belong in there too. The rest of the time, during normal site operation, having them sit in .module is a waste of memory and clutters the code.
Comment #3
jhodgdonWell, your understanding is different from mine. My understanding of .install files is that it is for code relating to installing/enabling/uninstalling/disabling the module itself. That should be clarified...
Comment #4
joachim commentedWell the thing is that the hook_modules_*() hooks are new to D7. Prior to that, 'relating to changing the system status of the module itself' was all there was. I guess nobody's had the discussion about whether install files are for relating to changing the system status of other modules too.
And so here we are :)
Comment #5
jhodgdonOK. Well that isn't the only new hook that would be moved, if your idea is the one that is adopted.... There is also hook_schema_alter() and possibly others that relate to these processes? They should definitely all be consistent. And I would guess some code change would be needed in order to move them?
Comment #6
sven.lauer commentedI concur with jhodgdon's understanding of the current intention of .install files. To my mind, if we want to optimize / minimize loading of code that is only used rarely, we should use a hook_hook_info()-group and rely on the autoloading mechanism for those. Then, hook_modules_*() could (but need not) go into a MODULE_NAME.modules.inc (or MODULE_NAME.module_install.inc or something). The autoload mechanism is there for exactly this purpose, from the hook_hook_info() doc:
.install files, on the other hand, are special as they collect hooks that are intended to be run only on the implementing module's (de)installation and (de|en)abling. Actually, the optimization argument cuts both ways: Loading the .install files of ALL modules if any given module is installed/uninstalled/enabled/disabled is overkill. Having a separate include file for hook_module_*() implementations to go in would be more efficient (also, easier to implement, as the include file would be autoloaded).
Comment #8
dawehnerComment #21
nicxvan commentedI think we can close this, these hooks should be converted to a class with the attribute and can be organized in their own file.
Comment #22
catchAgreed, closing as outdated.