DX: Add hook_modules to act on other module status changes

This is sorely needed.

I think it has to be a little smarter when multiple modules are enabled/disabled at same time. The 'enableapi' calls should happen after all modules are enabled so that the new modules may also react accordingly. Hope that makes sense.

tested and this breaks nothing while adding sorely needed functionality. modules really do want to take action when their neighbors are enabled/disabled.

We shouldn't use the word 'api' in the name of a function/hook. It's not consistent with core and redundant information.

Anyone have a naming suggestion?

hook_module_(en|dis)able

we already have those hooks. the problem is that all those hooks will suddenly start getting called. perhaps thats not a big concern. we could make sure that the module being disabled gets called first? what do people think?

CCK currently also has a use case for hook_module_uninstall().

We recently fixed #292872: fields and data are deleted when a content-type module is disabled, which was not cool when people were simply disabling modules say, when preparing for a D5-D6 upgrade.
But that was the only chance we ever got to clear that information. Right now, you uninstall a module and you have stale CCK information that sits here until you manually remove it.

Ah, now I understand the name chx proposed. sounds good.

I'm also seeing a need for this hook so I can add a setting to FCKeditor to help disable a certain textfield in my module.

Another idea would to have hook_module($module, $op) where $op would be 'install', 'uninstall', 'enable', or 'disable'.

Here's my patch for hook_module($module_name, $op) against HEAD. Tested and ready for review. Also added a documentation patch against HEAD docs.

Note that the TestBot error is for the documentation patch.

Renaming for better description.

hook_module($module, $op) seemed backwards to me while hook_module($op, $module) makes more sense.

I thought, well, let's be consistent. So, I did a review of the hooks in core for 6.x and 7.x and here is what I found:

$op not first:
hook_comment
hook_nodeapi
hook_node_grants

$op first:
hook_block
hook_filter
hook_search
hook_taxonomy
hook_user

So...I feel like the $op should come first and that we should standardize on putting the $op first. $op has been second/last in some cases for a pretty long time so I was unable to find any discussion about changing the function signature.

For this patch the only question is whether $op should come first. If folks agree it should come first then moving it around in other hooks could be a separate issue.

I think I agree with you greggles. Having $op first is starting to grow on me. Re-patched for parameter change. It would be wonderful to get this in so we could work on #306151: Automatically install/uninstall schema!

Also, landing this hook would be essential to begin work on #306151: Automatically install/uninstall schema.

Patch applies cleanly and works as advertised. Speaking as a contrib developer, this patch is a Good Thing (tm), as it allows all kinds of goodness. It also, as mentioned, will make #306151: Automatically install/uninstall schema fairly simple.

Talked some with chx about this in #drupal. A few things:

1. There aren't any clear use cases that I see here, just a lot of "Yeah! This would be great!" It'd be helpful to get more info as to /why/ this would be so great. ;)

2. The registry needs hooks to be like hook_thingy_thinger_op(...) rather than hook_thingy_thinger($op, ...). So we'll need to rename the functions (again).

3. There aren't tests. Seems like it would be pretty easy to write a little .test module that implements these hooks and verifies things are working properly.

As was recommended, I split the hook_module($op, $module) into it's four different hooks:
- hook_module_install($module)
- hook_module_enable($module)
- hook_module_disable($module)
- hook_module_uninstall($module)

Attached is the patch for the hooks as well as a hook for the core hook documentation (which will fail TestingBot). I have also modified the EnableDisableCoreTestCase in system.test to test for these hooks being called correctly. The test passes 100% for me and is ready for review.

As for use cases, the biggest use case for this change is the proposal found in #306151: Automatically install/uninstall schema.

The filter module should remove filters from modules that are uninstalled:

function filter_module_uninstall($module) {
  if (module_hook($module, 'filter')) {
    db_delete('filters')->condition('module', $module)->execute();
  }
}

Similar cases can be made for removing module-provided user permissions, blocks, actions, menus (move current menu code from drupal_uninstall_module), etc. This will keep module-specific uninstall code out of drupal_uninstall_module and help prevent stale records in the database that will never be referenced again.

I'm sure that there are smaller use cases for these new hooks that modules would find useful and helpful to have, as shown already by #292872: fields and data are deleted when a content-type module is disabled.

re webchick #27
1. There aren't any clear use cases that I see here, just a lot of "Yeah! This would be great!" It'd be helpful to get more info as to /why/ this would be so great. ;)

#292872: fields and data are deleted when a content-type module is disabled : CCK shouldn't delete field and field data when a module defining a content type is *disabled*, but then has no way to do it when the module is *uninstalled*, so stale fields and data tables rot in the db.

@webchick

This approach is a better, more Drupal approach to solving the problem defined here -- #306027: user_modules_uninistalled() is missing -- and leads to allowing the types of data cleanup that yched mentions. See #306151: Automatically install/uninstall schema for some further examples -- block cleanup, permission removal, etc. There are a number of things that developers might have to do in hook_uninstall() that we can (and should) do automatically in core.

We would also move the menu-specific code out of drupal_uninstall_module and into menu.inc. Think of it this way -- core modules should all have a chance to clean up after themselves after a module is uninstalled.

See Dave's notes here -- http://drupal.org/files/issues/306151.module_uninstall.txt

Adding the other operations adds additional flexibility. In the case of Domain Access, I would love to be able to act based on module activation, so I can test to see if a new node access module has been activated.

/me goes to test the new patch.

Patch http://drupal.org/files/issues/253569.hook_module_0.patch applies cleanly.

Tests pass and hook fires as expected.

Summary of discussion on IRC:
- My eye was drawn to the foreach loop in module_enable, since we already have a foreach loop up above. Some back and forth later, chx came up with the idea to pass the entire array of modules being enabled/disabled rather than just one at a time. This is much more powerful: check to see if a conflicting module is in the list, etc.
- (anal) Put some Doxygen on the hook implementations in system_test.module.

We had it that way before and it is less powerful. See my comment in #3. The new modules that are enabled need a chance to react as well. You really have to properly install the new modules and THEN fire the hooks. What is the point of saving a foreach loop? This is extremely infrequent code and we are talking about a millisecond here.

Moshe, your comment in #3 is not too clear. I interpreted it as you do not want module_invoke to happen inside the main module_enable foreach loop but outside. There is absolutely no point in iterating module_implements here, we want a module_invoke_all just the question is -- inside or outside?

I think I agree with webchick and chx with regards to passing hook_module_enable or hook_module_disable an array of the enabled/disabled modules. It would be easier to check for incompatibilities between more than just two modules. This has the same functionality as #3 (performing after all the modules have been enabled/disabled), but the parameter is now just an array form.

Revised patch with 100% system test pass.

I thought webchick proposed to collapse the two loops into one. Sorry about that. Just changing the arguments on the second loop is fine.

I should say though that you can't do much if you see a module that is incompatible in the list. The module is already enabled, and rightfully so. The right time to object is during form validation, not during the enable API.

Lost the last patch upload...

Here's another thought: It would be easy to have hook_module_install($module) to return a boolean. If that boolean is true, do not install the module specified by $module. Not sure if I like that or not... It would make hook_module_install special from the rest.

Dave, I don't quite get the last patch. It looks like _install and _uininstall are passing strings and _enable and _disable are passing arrays.

Don't we want consistency there? Or am I just missing the point of the last bit of discussion.

Consider if I am ImageAPI module.

I have a requirement that *either* ImageGD or ImageMagick module are /also/ enabled, but not both. Because it could be either of these modules, the requirements line in the .info file is useless to me.

I don't see any possible way to do this if the only context in hook_module_enable() I have is *one* of the modules that were enabled. I need the full list, so I can check to see if the modules I care about are in it.

@webchick et al - i personally don't think we want to do validation in this hook. thats gonna be terribly messy interacting with the dependancy validation. a rejection of a given module has to then reject the dependants. the module can do a dsm() or a runtime requirements error if it wants.

Wouldn't that mean we need a pre-enable check? We could throw a hook during the form validate step.

Psuedo-code:

// validating the module list form...
  module_invoke_all('module_validate', $form_state);

Hm. Then I guess I'm not clear on what the point of hook_module_enable/disable is, if it's not for validation?

It's for performing actions/cleanups based on what your module (or core) knows about other modules.

The menu wipe is the obvious core implementation, but it can apply to lots of things in the contrib world. In my modules, there are certain settings that I may want to enable if OG is enabled, for instance.

That's why I think this is a DX (Developer Experience) patch, in line with bjaspan's notes on the subject.

Ok cool. Then the only question remaining is whether hook_enable() / hook_disable() are more useful when passed in the entire array of enabled/disabled modules, or whether they should only pass them in one at a time, for consistency with hook_install() and hook_uninstall()?

Also agnostic on the string v. array debate. I suppose it presents the most flexibility if the hook passes as an array, so that the responding functions can deal with the entire data set at once. On the other hand, strings are easier to deal with for me.

But we usually come done on the side of "more flexibility in the API," so I suppose that's a vote for passing the whole array of status changes to each hook. The only warning there is that the target modules still need to be in memory so their functions can be accessed during each phase -- loading hook_perm() from a disabled module, for instance.

Another good use case that arose in #drupal was if your module (mymodule) needs to have a higher or lower weight than a conflicting module (lousymodule), there's really no good place to check for that currently besides when your module is being installed with mymodule_install().

With this patch, mymodule could implement:

function mymodule_module_install($module) {
  if ($module == 'lousymodule') {
    // Set mymodule weight
  }
}

another use case: bulletproof Admin Role module
automatically enable permissions when "other modules" are enabled..

I think we're good on use-cases (see http://drupal.org/node/253569#comment-1008955).

We just need a new patch that settles the string/array question.

+1 for extra flexibility (array)

Revised patch attached that uses arrays for all four hook implementations. Tests pass.

It looks to me like there is some redundant code in system_modules_submit and in _drupal_install_modules, both of which try to check if a module has been installed previously.

Since the routine in system_modules_submit effectively filters out modules that need installation from those that need to be enabled, this patch simply fires the new hooks straight from the submit function.

Also note that hook_module_disable fires _before_ we flush the registry, in case we need to access any hooks from the modules being disabled (such as hook_menu) in order to perform actions.

Ah, sorry. -1 to doing this in submit functions. This means that an install profile, for example, calling drupal_uninstall_modules() wouldn't fire the proper hook.

@webcheck -- that also explains what I took to be redundant code.

drupal_uninstall_module($module) accepts a string, not an array, which seems pretty odd, so that needs to be cleaned up. Bringing that in line with drupal_install_modules() should allow us to use arrays in all cases.

New patch changes drupal_uninstall_module($module) to drupal_uninstall_modules($module_list).

Tests pass -- http://testing.drupal.org/node/14661

1. Changing drupal_uninstall_module($module) to drupal_uninstall_modules($module_list) would be a great change for consistency with drupal_install_modules($module_list). I did a core search for calls to drupal_uninstall_module (besides the call in system_modules_uninstall_submit), but there were no other results, so everything looks good to change this.
2. Since we're always going to be using module name arrays, should we change the hook name from hook_module_action to hook_modules_action?
3. I disagree with relocating the hook_module_install until after module_enable is called, because now hook_module_enable is called before hook_module_install.
4. Ran tests and passed.

A quick note, when we get the documentation for hook_modules_action written, we'll need to make a note that hook_module_install and hook_module_enable will need to call drupal_load('module', $module) if they need to get information from the modules being installed/enabled (registry hasn't been rebuilt). Maybe the registry needs to be rebuilt after all the modules have been installed in drupal_install_modules instead of when each module is enabled?

Patch below fixes the order of hook_module_install and hook_module_enable.

Part of the logic to running hook_modules_install() after module_enable was so we don't have to load the code twice. But it is probably best to run that routine first.

I think this patch adds useful functionality, however, we now have both foo_install and foo_module_install and I think that is a little confusing. Same with foo_uninstall and foo_module_uninstall. The difference is subtle and might confuse developers, and could result in inconsistent code. I wonder if there are things we could do to make the difference more prominent, and to better document those hooks. What might be a little better is to use foo_module_installed vs foo_module_install?

It is a minor point but I wanted to bring it up to see if we have any ideas. Ah, semantics!

Some ideas:

- hook_modules_installed($module_list)
- hook_modules_uninstalled($module_list)
- hook_modules_enabled($module_list)
- hook_modules_disabled($module_list)

-Or-

- hook_install_batch($module_list)
- hook_uninstall_batch($module_list)
- hook_enable_batch($module_list)
- hook_disable_batch($module_list)

[The above could be reversed to hook_batch_install($module_list)]

-Or-

- hook_install_all($module_list)
- hook_uninstall_all($module_list)
- hook_enable_all($module_list)
- hook_disable_all($module_list)

This mirrors module_invoke_all() but could cause confusion because developers might assume it means "enalbe all the modules available on my site."

-Or-

- hook_install_respond($module_list)
- hook_uninstall_respond($module_list)
- hook_enable_respond($module_list)
- hook_disable_respond($module_list)

_respond here could also be _response or _reply or _react.

I also thought about hook_install_alter($module_list), but I don't think that is consistent with the user of _alter() hooks elsewhere.

I think my favorite is:

- hook_process_install($module_list)
- hook_process_uninstall($module_list)
- hook_process_enable($module_list)
- hook_process_disable($module_list)

Where 'process' clearly indicates that we are acting on provided information, as in drupal_process_form().

I think hook_modules_(installed|past-tense-verb) has enough distinction from hook_(install|etc) but yet still close enough to an accurate description of the hook that it isn't confusing. I provided an initial docs patch in #25 that I'm working on updating to distinguish these new hooks from hook_(install|etc).

Works for me.

+1 to hook_modules_(past-tense verb)

Updated patch with the change to hook names to hook_modules_past-tense-verb. Ran all system tests and had 100% pass.

Hm, I think that if:

function drupal_uninstall_modules($module_list = array()) {
  foreach ($module_list as $module) {

Then you have to use an opt-out empty.

function drupal_uninstall_modules($module_list = array()) {
  if (empty($module_list)) {
    return;
  }
  foreach ($module_list as $module) {

This function should never be called with an empty array, but I understand that the array() is there for symmetry with module_install_modules().

The past tense verb version seems like a sensible choice i.e. hook_modules_enabled - things to do when modules are enabled etc.

Found a logic error that would fire hook_modules_installed for an enabled module that has been previously installed, enabled, then disabled. Also added a check for an empty array in drupal_uninstall_modules (thanks agentrickard).

Patch had a typo on line 488 of install.inc. New patch corrects. Test implementations of the hooks fire as expected. System tests pass.

Re-rolled from #69 and added some test documentation. The tests pass and I firmly believe this is RTBC. I can haz thiz in unstable-2?

No longer applies with the other crazy stuff that went in today. Probably needs a quick re-roll.

Re-rolled patch, tests pass. No major changes from last patch, so marking as RTBC.

Ok, looked this over one last time, and I believe it has Dries's stamp of approval as well, so committed to HEAD. :)

Docs, please! :D

YAY! :D Will get docs posted shortly. I assume we'd like a note in 6.x -> 7.x module upgrade doc?

Yes please! Also some API docs for api.drupal.org.

api.drupal.org docs and module upgrade docs finished. The links to the api docs on the module upgrade page currently do not exist. Just waiting for the api to regenerate.

Yay!

The following patches follow from this hook and need work

#306027: user_modules_uninistalled() is missing
#306151: Automatically install/uninstall schema

Subscribing—it's closed but I want to come back and get my head around this one and haven't got time to read it all now. :)

Note that hook_modules_disabled() no longer seems to be working as the code comments introduced here suggested it should work. An issue for that is here: #727876: Enabling modules one at a time works differently than enabling them all at once