A reasonable assumption, IMO, is that hook_modules_installed
should fire after a module is installed but before it's enabled. Currently that's not the case: hook_modules_installed
and hook_modules_enabled
run only after modules have been both installed and enabled.
In other words, with these two hooks it's impossible to inject logic between the module install and module enable phases.
In the original patch for this feature (#253569: DX: Add hook_modules to act on other module status changes), hook_modules_installed
was invoked before module_enable
. Since then, drupal_install_modules
has gone away, module_enable
has been rewritten, and install.inc
has been refactored. At some point, hook_modules_installed
was moved.
In this patch:
The invocation of hook_modules_installed
is moved back to where it was, directly after module installation, before module enabling.
Advantages:
Allow developers to inject logic between install and enable phase.
Disadvantages:
The number of module_invoke_all('modules_installed')
calls will increase from 1 to N, where N is the number of modules being installed for the current request.
Comment | File | Size | Author |
---|---|---|---|
#29 | 836748-29.patch | 7.67 KB | jhodgdon |
#28 | 836748-cleanup_1.patch | 8.17 KB | AaronBauman |
#26 | 836748-cleanup.patch | 7.74 KB | jhodgdon |
#24 | 836748_module_enable_hook_installed_enabled_documentation_1.patch | 6.32 KB | AaronBauman |
#20 | 836748_module_enable_hook_installed_enabled_documentation.patch | 9.26 KB | AaronBauman |
Comments
Comment #1
AaronBaumanComment #2
jhodgdonDo you have a use case for wanting it to be this way? I mean, I agree that it's somewhat reasonable, but is there a reason for thinking that you'd want to do something between install and enable, that would justify the overhead?
Comment #3
sunI don't see the relationship to #569326: Add hook_taxonomy_vocabulary_info() - could you please explain?
Comment #4
AaronBaumanforum.module defines a vocabulary via hook_taxonomy_vocabulary_info.
taxonomy.module reads the vocabularies defined thusly during its implementation of hook_modules_installed.
During taxonomy_modules_installed, any newly-defined vocabularies are inserted into the database.
In forum.install, forum_enable depends on the vocabulary defined in forum_taxonomy_vocabulary_info.
Therefore, unless taxonomy_modules_installed is called before forum_enable, the vocabulary it expects to find will not have been created yet.
Did that make sense?
Comment #5
AaronBaumanFollowing up, maybe somewhat tangentially:
ctools gets around this bootstrapping problem by implementing a system to enable/disable/override/clone/etc. code-based settings and content.
For this system, when loading an entity or setting via ctools, ctools gathers everything it needs from code as well as everything it needs from the database. The database takes precedence, and anything defined in both places is called "overridden". Anything defined in code only is called "default". Anything defined in the database only is "custom" more or less. Moreover, a setting or entity defined in code is a first-class citizen that doesn't need to be duplicated to the database and referenced with an auto-incrementing primary key.
The ctools system hinges on having unique identifiers for vocabularies across modules, e.g. forum_nav_vocabulary, and using these as primary identifiers. Drupal uses an arbitrarily assigned integer, e.g. vocabulary's vid, that could easily result in conflicts between modules and across sites. Such a change would represent a pretty fundamental shift in The Drupal Way™, so for now it seems like the best solution is to process some x_info hooks and dump the results into the database...
Comment #6
sunI still don't understand - hook_modules_installed() is invoked before hook_modules_enabled() right now already, as visible in the copied context.
Powered by Dreditor.
Comment #7
AaronBaumanThe patch is for
module_invoke_all('modules_installed')
to be invoked beforemodule_invoke('enable')
.Updating title for clarification.
Comment #8
sund'oh.
Looks good then. The passed $modules "array" doesn't make much sense then, but stays consistent with hook_modules_enabled(), and consistency is good.
Not sure whether we need David Rothstein or catch to double-confirm this tweak. I don't think so.
Comment #9
sunThe only issue might be expensive operations being done in http://api.drupal.org/api/search/7/modules_installed implementations... contrary to now, they wouldn't know when the last module has been installed, so as to be able to do an expensive cache/data clearing and rebuilding, considering any new information provided by all newly installed modules.
Comment #10
AaronBaumanGood point - field_modules_installed calls field_cache_clear.
(For that matter, so does field_modules_enabled -- #836962: field_cache_clear called twice sequentially when installing modules.)
However, it's impossible to install a module without enabling it.
So,
hook_modules_enabled
would always be called afterhook_modules_installed
was called.If an implementation needs to invoke an expensive operation after the module installation is complete, it should not be done in
hook_modules_installed
anyway. Right? Can you think of a counter example? The only one I can think of is a convoluted case in which an implementation ofhook_modules_installed
makes changes that invalidate fields' cache, and an implementation ofhook_modules_enabled
relies on those specific changes. In that case, maybehook_modules_enabled
should be responsible for setting its system weight higher than fields', or for clearing the fields cache itself?Comment #11
yched CreditAttribution: yched commentedWhen 10 modules are enabled, I don't think replacing one call to hook_modules_installed() notifying of all modules by 10 separate calls is a good idea.
Comment #12
jhodgdonyched has a point. But the point of this issue is that sometimes you need/want to react to the fact that a module has been installed, before the hook_enable() for that module gets called. So you need the hook_modules_installed() to run before that.
I think?
Comment #13
AaronBaumanRe #11: I agree that it's not ideal.
Re #12: bingo.
So, the attached patch refactors module_enable() in the following ways to strike a balance between the two concerns:
By doing this, hook_modules_installed() gets invoked between "install" and "enable" phases, addressing the OP; and it's invoked only once, addressing the concern in #11.
Comment #14
jhodgdonYou have some whitespace issues in that patch - tabs vs. spaces I think. When I look at the patch in my browser, the indentation is wonky. No tabs.
Other than that, this seems like a good approach to me.
Comment #15
David_Rothstein CreditAttribution: David_Rothstein commentedWell, this is a bit tricky actually.
The current behavior of waiting to invoke the hook is a result of #620298: Schema not available in hook_install(). I can see why we might want to reverse that and not wait, but the counterargument is that in Drupal "installing a module" generally refers to the whole process of starting it from nothing and getting it fully ready to be used. As someone implementing hook_modules_installed(), I think I would reasonably expect that the list of modules I receive are fully installed, enabled, and ready to be treated like first-class module citizens.
I don't think we have to look very far to see an example of that:
http://api.drupal.org/api/function/user_modules_installed/7
This function receives the list of newly-installed modules and goes on to invoke hook_permission() in each of them; in doing so, it expects to get back a complete list of that module's permissions. But many modules in Drupal have dynamic permissions that depend, e.g., on items stored in the database - items which may very well have been initially inserted in hook_enable(). So if hook_enable() hasn't run yet, you'd get an incomplete permission list back, no?
Comment #16
David_Rothstein CreditAttribution: David_Rothstein commentedAlso, a minor point about the patch:
Why remove that if() statement?
Comment #17
AaronBaumanRe: #15
OK, that makes perfect sense.
Thanks for the background and explanation.
Unless anyone else has a counter argument, I think this is now "won't fix".
I will attack #569326: Add hook_taxonomy_vocabulary_info() from a different angle.
Comment #18
sunThis question and topic is sufficiently complex that we should put the considerations and reasoning from this issue into code.
i.e. into doxygen description of module_enable(), which the patches in here kept on touching.
Comment #19
jhodgdonExcellent idea, and at least put a sentence like
See module_enable() for a description of blah blah blah
in both hook_modules_enabled() and hook_modules_installed().
Comment #20
AaronBaumanOK, in this patch:
There were also discrepancies in the placement of @see notations in system.api.php, and some trailing whitespace which I couldn't help fixing.
Comment #22
jhodgdon#20: 836748_module_enable_hook_installed_enabled_documentation.patch queued for re-testing.
Comment #23
jhodgdonA few thoughts:
- All of your @see lines need to have () after the function names, so they can properly turn into links on api.drupal.org.
- Since you've gone to the trouble to define the difference between "enable" and "install", maybe the one-line description of module_enable() should say that it installs and/or enables modules?
- If you think that's a good idea, you could bring the first line into compliance with our standards (start with 3rd person verb) -- see
http://drupal.org/node/1354
- I appreciate the effort you went to to move the @see lines to the end of docblocks, but please do this on a separate issue for functions unrelated to this issue, or we'll get accused of killing kittens.
- It seems like hook_disable() and hook_uninstall() should maybe have @see to each other, and maybe hook_enable and hook_install should reference each other too?
- If you're in there fixing hook_enable(), you could correct the grammar/spelling on the line above too:
That line should be:
This hook is called every time the module is enabled.
Comment #24
AaronBaumanThanks for the feedback.
Agreed on all points except I'm not a huge fan of "and/or" because I find it's often abused.
In this case it's unnecessary since the definition of "Installing" a module includes "Enabling" it, so we only need "or".
So, changes from the previous patch:
Comment #25
AaronBaumanComment #26
jhodgdonFixed a couple of typographical/grammatical errors and added some more @see lines (repatch is faster than explaining...).
Comment #27
sunThanks!
(and elsewhere) "...are invoked."
s/on the list of/in all/
49 critical left. Go review some!
Comment #28
AaronBaumanLast one for me today until Monday...
Comment #29
jhodgdonI'm not fond of some of the wording you changed since my last patch, such as:
+ * - Invoke hook_modules_installed() in all installed modules.
vs.
+ * - Invoke hook_modules_installed() on the list of installed modules.
Hmmm. I guess that was sun's idae. However, I think my version (the second one) is more accurate, since the list is the argument to the hook, which is invoked once, passing in the array of all the modules as the argument.
Or maybe we should just say "Invoke hook_modules_installed()." and leave it to hook_modules_installed() to document what its arguments are?
Here's yet another patch...
Comment #30
sunyup, my request. "Invoke hook on the list of ..." sounded like we'd want to call functions on a list, but we are calling functions in certain modules, and "invoke foo in all bar things" is quite often used throughout core and contrib, to my knowledge.
Anyway, I guess we have more pressing issues than these nitpicks ;)
Comment #31
Dries CreditAttribution: Dries commentedNice improvements. Committed to CVS HEAD. Thanks.