DX: Add hook_modules to act on other module status changes

oops, forgot the status

obviously, i wrote the invocation incorrect. was writing quickly. a better example:

there might be a better name for the hook as well, to integrate the functionality (using $op), but might be confusing. this makes the most sense to me, considering we have hook_enable and hook_disable already, and hook_node/hook_nodeapi have set a precedent.

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.

ah yes, i see what you're saying. we'll just need to run through the loop after they're all enabled. i'll rewrite the patch with that in mind today or tomorrow.

thanks,
aaron

this creates a new loop after enabling all new modules for enableapi. disableapi is the same.

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.

chx's naming convention sounds good. i'm neutral to the name, so long as we have the functionality. i'll reroll a patch in the next day or two.

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.

Patch failed to apply. More information can be found at http://testing.drupal.org/node/14069. If you need help with creating patches please look at http://drupal.org/patch/create

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.

Patch failed to apply. More information can be found at http://testing.drupal.org/node/14238. If you need help with creating patches please look at http://drupal.org/patch/create

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:

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.

subscribe

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:

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.

@webchick: my original use case was to add a table to the db if another specific module had been installed (and to remove that table in turn if the module is uninstalled). I agree with moshe that this shouldn't be a step for validation.

without this hook, i'm stuck having to insert myself into form_alter on the modules admin page, which can get a bit hairy. from other comments on this thread, seems as though there are other use cases as well. in fact when i needed the functionality, i was surprised it wasn't already there, as it seems to me an easy call that can have many valid uses.

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()?

#37 applies cleanly. looks sound to me. it also takes care of my filtered modules on disable concern, so i'm now agnostic about whether to include the entire array or not. personally, i like consistency, and most cases i could think of just want to act on a single module's status. plus it's slightly easier to deal with a single string than an array. however, if there are some edge cases that could use them, then sure, throw the whole array at them.

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:

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:

Then you have to use an opt-out empty.

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

Automatically closed -- issue fixed for two weeks with no activity.

Automatically closed -- issue fixed for two weeks with no activity.

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. :)