Improve documentation about install/enable/etc. hook invocation order

Do 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?

I don't see the relationship to #569326: Add hook_taxonomy_vocabulary_info() - could you please explain?

+++ includes/module.inc	24 Jun 2010 15:29:14 -0000
@@ -393,11 +393,6 @@ function module_enable($module_list, $en
-  // If any modules were newly installed, invoke hook_modules_installed().
-  if (!empty($modules_installed)) {
-    module_invoke_all('modules_installed', $modules_installed);
-  }
-
   // If any modules were newly enabled, invoke hook_modules_enabled().
   if (!empty($modules_enabled)) {
     module_invoke_all('modules_enabled', $modules_enabled);

I 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.

d'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.

The 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.

When 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.

yched 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?

You 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.

Well, 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?

Also, a minor point about the patch:

-  if (!empty($modules_installed)) {
-    module_invoke_all('modules_installed', $modules_installed);
-  }
+  module_invoke_all('modules_installed', $modules_installed);

Why remove that if() statement?

This 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.

Excellent 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().

#20: 836748_module_enable_hook_installed_enabled_documentation.patch queued for re-testing.

A 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:

  * The hook is called everytime module is enabled.
+ *
+ * @see module_enable
  */
 function hook_enable() {

That line should be:
This hook is called every time the module is enabled.

Fixed a couple of typographical/grammatical errors and added some more @see lines (repatch is faster than explaining...).

Thanks!

+++ modules/system/system.api.php	25 Jun 2010 19:13:54 -0000
@@ -2050,15 +2050,18 @@
+ * hook_install() is only called on the module actually being installed. See
+ * module_enable() for a detailed description of the order in which install and
+ * enable hooks are performed.

(and elsewhere) "...are invoked."

+++ includes/module.inc	25 Jun 2010 19:13:53 -0000
@@ -286,8 +286,25 @@
+ * - Invoke hook_modules_installed() on the list of installed modules.
+ * - Invoke hook_modules_enabled() on the list of enabled modules.

s/on the list of/in all/

49 critical left. Go review some!

I'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...

yup, 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 ;)

Nice improvements. Committed to CVS HEAD. Thanks.