During hook_enable: Hooks in newly enabled module cannot be invoked

Indeed I omitted a clear_cache from module_implementations at the time of writing because back then we reloaded the page and that was it. The API change is somewhat internal so that should not be a problem. However, you should need to comment this change.

To clarify: I meant doxygen. Please mention that 'for internal use only'.

I'm not a big fan of $clear_cache parameters ...

I think $clear_cache may be a necessary evil. Maybe $refresh would be more consistent.

I think $clear_cache may be a necessary evil. Maybe $refresh would be more consistent.

Renamed to $refresh and fixed comment punctuation.

Hmmm, for some reason this addition to module_enable() is killing the "The configuration options have been saved." message when enabling a module.

Nevermind. This was my own fault testing the hook_disable.

Submitted a separate issue as per chx's request at http://drupal.org/node/97271 for hook_disable.

Clearing various caches for every individual module that gets enabled is going to cause serious performance problems. Can you imagine how inefficient it would be if a user enabled 20 modules at once (as it is perfectly possible to do on the 'admin/build/modules' page), and Drupal followed this procedure:

1. Load module A
2. Clear module_list() cache
3. Clear module_implements() cache
4. Clear the menu cache
5. Invoke hook_enable() for module A
6. Load module B
7. Clear module_list() cache
8. Clear module_implements() cache
9. Clear the menu cache
10. Invoke hook_enable() for module B
11. Etc...

This is what the proposed patch does, and there is no way that code as inefficient as this can go into core. Clearing all those caches is very expensive - it should only be done once, for all modules that are being enabled in a given operation. This is the procedure that should ideally be followed:

1. Load module A
2. Load module B
3. Etc...
4. Clear module_list() cache ONCE
5. Clear module_implements() cache ONCE
6. Clear the menu cache ONCE
7. Invoke hook_enable() for module A
8. Invoke hook_enable() for module B
9. Etc...

Attached patch implements the latter (more efficient) procedure. It changes module_enable() to accept an array of modules, instead of a single array, so that it can implement the procedure for all the affected modules. All functions in core that call this function have been updated to work with the new system, including drupal_install_modules() (the drupal_install_module() function has been removed, and its code placed in a foreach() inside drupal_install_modules()), and system_modules_submit().

Also, module_enable() no longer returns anything in this patch, whereas it currently returns TRUE if the specified module is not already enabled, and FALSE otherwise. I don't think that this is a big deal: none of the dependent functions in core use this return value.

This patch is much bigger and more complex than the previously proposed patch, but these changes are required if this cache-clearing is to be carried out in a sensible way. Hopefully the importance of the patch warrants the slight API change.

This patch has been tested for installing Drupal 5 fresh, and for installing / enabling / disabling modules on the admin/build/modules page. Works fine for me, but could use more eyeballs. If people think that this patch is OK, then http://drupal.org/node/97271 should be updated to implement similar stuff for disabling of modules.

Clearing the menu cache 10 times in a 10 milliseconds, or clearing it once in 10 milliseconds shouldn't affect performance. I'm not convinced that Jaza's argument holds. His patch performs better, but does it matter? And is it worth the added complexity? Either way, the patch look OK so I'd like to get a bit more feedback from other people.

@Dries- I'm not too familiar with the inner workings of the menu cache, but I assume it's like other Drupal caches in that something is only inserted into the cache when a page is viewed. So, this should be a trivial cost.

Obviously, clearing out the list of modules via module_list() and module_implements() is going to run a fixed cost (query and function calls) per enabled module in the patch in #10. However, with the patch in #10 installed on Drupal on a local host (Mac Os 10.3.9 dual G4, MySQL 4.1, PHP 4.4.1), enabling 18 modules at once took ~3 seconds of user time.

Since enabling modules is a rare event, I don't think the efficiency of this code has much bearing on anything unless it's so bad that PHP times out.

I have a question, however: should not the menu cache also be cleared in function module_disable()?

Looks okay to me. Committed to HEAD.

Cool. I have uploaded a new patch to http://drupal.org/node/97271 so that module_disable() uses the same approach.