Automatically install/uninstall schema
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | install system |
| Category: | task |
| Priority: | normal |
| Assigned: | Dave Reid |
| Status: | closed |
| Issue tags: | Needs Documentation |
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.
#28
This seems to work fine now. Let's see what testbot says!
#29
The last submitted patch failed testing.
#30
This works, but testbot hates it. I have no idea how to make the bot happy.
#31
Found out why the tests fail, I think. Trying again.
#32
About time we used the schema properly...
#33
This patch would make it impossible to uninstall a whole bunch of modules via the Drupal user interface - since only modules which implement hook_uninstall() show up on the list.
I've leaving this RTBC because that can probably be fixed in a very simple followup, but it's critical that it be fixed if this patch goes in.
#34
Should be easy to add. Just show any module with an uninstall hook OR a schema hook. Problem solved.
#35
@David_Rothstein
Then why do all tests pass? Can you prove this? Happy to fix if you can provide an error.
#36
Hm, it's a good question why there is no test that catches this. I checked, and we do have a test that uninstalls modules via the UI, but it uses the aggregator module when doing so, and that module still has a hook_uninstall() even after this patch, so that's why it doesn't catch it. I suppose we could change it to use a different module that would catch this case, but it would be somewhat fragile to rely on that anyway, so maybe not worth doing.
Anyway, as was said above, this is easy to fix. The attached patch is identical to the previous one, except for the addition of this:
Index: modules/system/system.admin.inc
===================================================================
RCS file: /cvs/drupal/drupal/modules/system/system.admin.inc,v
retrieving revision 1.197
diff -u -p -r1.197 system.admin.inc
--- modules/system/system.admin.inc 26 Aug 2009 10:53:45 -0000 1.197
+++ modules/system/system.admin.inc 9 Sep 2009 03:56:17 -0000
@@ -1051,10 +1051,10 @@ function system_modules_uninstall($form_
// Grab the module info
$info = unserialize($module->info);
- // Load the .install file, and check for an uninstall hook.
+ // Load the .install file, and check for an uninstall or schema hook.
// If the hook exists, the module can be uninstalled.
module_load_install($module->name);
- if (module_hook($module->name, 'uninstall')) {
+ if (module_hook($module->name, 'uninstall') || module_hook($module->name, 'schema')) {
$form['modules'][$module->name]['name'] = array('#markup' => $info['name'] ? $info['name'] : $module->name);
$form['modules'][$module->name]['description'] = array('#markup' => t($info['description']));
$options[$module->name] = '';
Pretty trivial, so still RTBC.
To see the difference between this patch and the previous one, try disabling the database logging module and then going to the uninstall page. The previous patch would not give you an option to uninstall the module via the UI (and therefore you'd be stuck with the watchdog table around forever), but this one does.
#37
Excellent catch!
#38
Great little improvement. Committed this to CVS HEAD. This needs to be documented in the upgrade instructions so marking this as 'code needs work'. Set to 'fixed' after it has been properly documented. Thanks!
#39
Needs Documentation
#40
http://drupal.org/node/224333#install-schema
#41
Awesome to the max!
#42
Automatically closed -- issue fixed for 2 weeks with no activity.