Automatically install/uninstall schema
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | install system |
| Category: | task |
| Priority: | normal |
| Assigned: | Dave Reid |
| Status: | needs work |
This issue grew out of #306027: user_modules_uninistalled() is missing. Looking at drupal_uninstall_module(), it seems to me that the function is very non-Drupal. It calls a module-specific hook and then does some menu cleanup.
However, we also need to do permission, block, and other types of automated cleanup. Now we could force the developer to do these in hook_uninstall(), but it seems a better solution is a hook that lets modules act on their 'child' hooks.
That is, the current logic for menu cleanup should go into a menu.inc function.
Proposed hook would be hook_uninstall_module(), whith $module a string as parameter.
Some uses:
-- user.module user_uninstall_module($module) to remove permissions set by the module.
-- menu.inc menu_uninstall_module($module) to remove menu items set by the module.
-- block.module block_uninstall_module($module) to remove blocks set by the module.
-- node.module node_uninstall_module($module) to remove node types items created by the module.
This could also apply to trigger, filter, and other core modules. Making it a hook would also let contibuted modules (like Views) perform cleanups.
We could also use a similar function that fires a hook on module enable/disable, so that modules can react to changes in the module list.
#1
Actually this would be a perfect use for #253569: DX: Add hook_modules to act on other module status changes! Yay! Working on patch...
#2
Here's just some functions that would show what would be changed. Not a patch.
#3
I support hook_module(). Then we can use this issue for implementing the above list of implementations.
#4
+1 - subscribe
#5
That pseudo patch is quite nice. Anyone available to refine and re-submit?
#6
Trying to keep all these module uninstall issues in one post...
#306027: user_modules_uninistalled() is missing
#320303: DX: Remove module menus on uninstall
#335237: DX: Remove module filters on uninstall
#335240: DX: Remove module blocks on uninstall
#7
#8
Subscribing.
#9
Adding #420124: DX: Remove module actions on uninstall.
#10
Here is a test patch to get this rolling.
It appears that we can simply call the schema hooks directly, which removes an unneeded module_load_install(), since the schema function calls that.
Tested by removing hook_install() from comment module and removing drupal_uninstall_schema() from comment_uninstall().
I think this is the right approach -- it lets us remove cruft and streamline DX.
#11
The last submitted patch failed testing.
#12
Subscribing.
#13
Re-titling, interesting patch.
#14
@catch
This was a DX movement that Dave Reid and I were workin on and we got sidetracked. The idea is simple:
Module authors should not need to declare their own schema (un)installs, since Drupal is smart enough to do that for them. In fact, there are a number of related tasks that can be automated.
#15
Patch applies cleanly in dev. The problem is that it will break almost all tests, since we have to remove the duplicate install/uninstall calls.
#16
<?php// Uninstall the module.
- module_load_install($module);
+ drupal_uninstall_schema($module);
module_invoke($module, 'uninstall');
?>
(1) why removing the module_load_install() call?
(2) I would rather see the schema installation happen *after* we called the uninstall hook.
#17
1) Because drupal_uninstall_schema() calls module_load_install() already, so we don't need both. (It happens in http://api.drupal.org/api/function/drupal_get_schema_unprocessed/7)
2) This comment makes no sense. Installation after uninstallation? You mean reverse the order. Call the module's hook_uninstall() in case the module needs to act on its data?
// Uninstall the module.module_load_install($module);
module_invoke($module, 'uninstall');
drupal_uninstall_schema($module);
In that case, we would need the module_load_install(). But are there use-cases for modules that act to cleanup after their own data?
#18
Sure he meant "would rather see the schema uninstallation happen *after* we called the uninstall hook". I agree that this sounds safer. Some uninstall logic may depend on the state of the tables.
#19
The uninstall hook absolutely has to run while the tables are still there - if I need to call an API function for the uninstalled module to do clean up something in my module, the tables have to be there for that to work (say cleaning out vocabulary related tables when taxonomy module is uninstalled).
#20
I would love to see this fixed before code freeze. It's always embarrassing explaining to people that they need to explicitly install/uninstall their tables.
#21
@webchick
Does that mean you want to see a patch against all of core using the pattern defined in #17?
#22
For the record, there is an instance in book.install that validates Damien's correction.
#23
And here we go. For some reason, this patch breaks SimpleTest entirely, but I can't figure out why.
#24
Bumping to get some eyes on the testing problem.
#25
The last submitted patch failed testing.
#26
Bah. This fails because drupal_install_schema() -- used in system_install() -- doesn't update the schema version of a module, which makes no sense to me.
I account for that by wrapping db_create_table() in db_table_exists() check, which fixes the problem.
But the schema install functions are spread out all over the place. Some are in common.inc and some are in install.inc. I think they should all be centralized in database.inc, and the db_install_schema should update the {system} table.
But that is a different patch.
#27
The last submitted patch failed testing.