user_modules_uninistalled() is missing
agentrickard - September 9, 2008 - 14:32
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | user.module |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed |
Description
This is a small patch inspired by bjaspan's call for better developer experience.
The patch adds a small function module_permission_uninstall($module) that is automatically invoked when a module is uninstalled. This has the benefit of automatically cleaning the {role_permission} table when modules are uninstalled.
| Attachment | Size |
|---|---|
| permission_uninstall.patch | 1.71 KB |
| Testbed results | ||
|---|---|---|
| permission_uninstall.patch | failed | Failed: Failed to apply patch. Detailed results |
#1
TODO: Convert to the DBTNG format for the delete.
#2
Should this also be expanded to deleting node types, blocks, and actions (things that are automatically defined by the module and not the module.install?)
#3
Possibly, but not in this patch. Generally, things like node types are handled by the module's own implementation of hook_uninstall().
However, since permissions are handled through a universal hook_perm(), it seems natural to do this automatically instead of forcing developers to delete them with extra code.
#4
Actually since node types are handled through hook_node_info, it would be smart to uninstall those automatically. Same with hook_block().
#5
Good idea about perms (and blocks) ... We should only issue one delete query with an IN clause. For example, DELETE WHERE perm IN ('administer foo', 'manage foo') ...
#6
Argh! Why is there no Drupal function for creating an IN query?!?
As to node types and blocks -- separate patches for those, please. Unless we want to turn this into a function that checks all the "known" uninstall items.
#7
ah, but now we do. from one of your favorite functions:
<?php$query = db_delete('node_access')->condition('nid', $node->nid);
if ($realm) {
$query->condition('realm', array($realm, 'all'), 'IN');
}
$query->execute();
?>
#8
OK, I take that last rant back a bit. The new db_query accepts arrays. Nifty.
New patch attached.
Let's open other issues for blocks and such.
#9
@Dave Reid
See #306151: Automatically install/uninstall schema
#10
@moshe
Cool! Here's a new version using DBTNG!
#11
Hah! I actually submitted an issue for a new hook that would help with exactly that: #253569: DX: Add hook_modules to act on other module status changes. Would need to get that into core first.
#12
I am perfectly ok with rolling this patch into #306151: Automatically install/uninstall schema after #253569: DX: Add hook_modules to act on other module status changes lands.
In that case, this patch would go into user.module as user_module() with $op uninstall.
#13
subscribe
#14
New function would be:
function user_modules_uninstalled($module_list) {$perms = array();
foreach ($module_list as $module) {
// Get the module permissions.
$perms = array_merge($perms, module_invoke($module, 'perm'));
if (!empty($perms)) {
$perms = array_keys($perms);
$placeholders = db_placeholders($perms, 'text');
db_query("DELETE FROM {role_permission} WHERE permission IN ($placeholders)", $perms);
}
}
#15
i think this should be postponed pending #253569: DX: Add hook_modules to act on other module status changes
#16
Yes. It should. The function in #14 is dependent on that patch.
Setting to CNW.
#17
Alright! We can finally get this in now! Although we should probably get some tests written for this.
#18
That issues a delete query for each module. We can do a single delete for all modules by moving the query below the foreach().
#19
Dave - see the patch in #10 for the syntax that Moshe refers to. DBTNG lets us pass arrays of deletes.
The drupal_load() is not needed either, since those modules get loaded by
drupal_uninstall_modules()during the same page load cycle. So the functions are all still in memory.#20
Nope, #10 isnt it. That does a query per module as well. You can do this with one big IN clause.
#21
Moshe is right. :-(
#22
was this more what you had in mind? we should probably add a test to ensure this works as advertised.
#23
The drupal_load() there is redundant. This hook is invoked at the end of http://api.drupal.org/api/function/drupal_uninstall_modules/7
#24
Actually, we don't need to have the drupal_load in drupal_uninstall modules anymore. That was only used for the menu paths uninstalling, but that would depend on #320303: DX: Remove module menus on uninstall.
#25
well either of you could have re-rolled it without that ;) so tag now you guys get to write the tests ;)
#26
Sorry I've been only on to just check a few update statuses. I'm actually getting the hang at writing tests, so I'll get started on that.
But I have a question I wanted to ask everyone about these newtests for hook_modules_uninstalled: should they be done by the respective module or should we create/use a simpletest dummy module that implements one block, filter, permission, etc and be able to combine all these new tests in one test, much like the module list test in system.test. Ideas or thoughts?
#27
Well, I think we want to keep the drupal_load() in drupal_uninstall, otherwise, we have to fire it every time we write a hook_modules_uninstalled().
No opinion about testing.
#28
Progress, anyone?
#29
The last submitted patch failed testing.
#30
Re-rolled. Any suggestions for which module could host the appropriate test files? Build off of the module enable/disable test?
#31
i think we could add a module.test and module_test.module to handle this. once there's an obvious place for it i think we'd be well served to add have some unit tests for module.inc.
#32
i added tests to this patch, not sure wether this is the right way to check it
#33
I think the test could be a little simpler:
<?php// Checks whether the permissions where removed.
$count = db_query("SELECT COUNT(rid) FROM {role_permission} WHERE permission = :perm", array(':perm' => 'module_test perm'))->fetchField();
$this->assertEqual(0, $count, t('Permissions were all removed.'));
?>
#34
Also all the comments should be "Proper sentences."
#35
thx for the review!
here is a better patch, i hope :)
#36
+ // Uninstalles the module_test module, so hook_modules_uninstalled is executed.should be:
+ // Uninstalls the module_test module, so hook_modules_uninstalled() is executed.The ModuleUninstallTest class needs a PHPDoc block.
Other than that this is looking good.
#37
thx for taking the time to review
so there is another patch
#38
Any reason this testcase isn't in ModuleUnitTest?
#39
Probably because the ModuleUnitTest has a different setUp requirement than the uninstall test case. Will do further review shortly.
#40
not really, i thought because of user module, but this is silly
because user module is loaded every time
just wonders whether the function should be under or above assertModuleList()
#41
damn this cannot work!
#42
sry for the 3 posts
so this time with a setup function
#43
didn't test it but visually it's fine by me.
#44
sry but this cannot work
$this>assertModuleList()gets all active modules, and check with expected modules, (but there are path module_test and user, and not just path)so we have to add module_test and user to this expected values or we have to add another testclass
#45
then modify assertModuleList() to add module_test and user in there.
#46
You don't need to call setUp with the user module. It is automatically installed since it's a required core module.
#47
IMHO I think this should be in a separate test class, since eventually we're going to be adding more uninstall tests. The current ModuleUnitTest tests "low-level API functions" which is not what we're testing. We should have a new "class ModuleUninstallUnitTest" to fit the core test class name standards.
#48
i tryed to solve this, but i couldn't do it, anyone else know know how to solve it?
at least this patch reduces the fails to 4
#49
#47 sounds logical for me
so i took #37 and fixed a code style problem at the beginning of the file and added a better comment
#50
#51
The last submitted patch failed testing.
#52
Attached is an updated patch, but the tests all fail because there is no 'module_test.module' file.
#53
arg, i created this module before, but i didn't added it to the patch file :(
#54
Excellent. Having the module_test.module will help with all the related DX patches, too, since we can dump dummy hooks there.
#55
so what other things could be removed automatically
sry for not checking them, whether they get removed
#56
Dave Reid has a list over at #306151: Automatically install/uninstall schema
#57
whats the best way to handle the waiting to get something like this commited?
should i wait to create the other testspatches untill this is commited?
#58
Or note the dependency in the other issues, which may get more attention on this one.
#59
Patch review:
class ModuleUninstallTest extends DrupalWebTestCasePlease rename to ModuleUninstallUnitTest as per the testing naming standards.
parent::setUp('module_test', 'user');Please exclude 'user' since it is automatically installed (it's a required core module).
We need to test to make sure the permissions are actually found in the {role_permission} table so that we know for sure the uninstall deletion works. Something like:
$count = db_query("SELECT COUNT(rid) FROM {role_permission} WHERE permission = :perm", array(':perm' => 'module_test perm'))->fetchField();$this->assertTrue($count, t('Module permissions were found.'));
+name = "module test"+description = "Support module for module system testing."
+package = Testing
+version = VERSION
+core = 7.x
+files[] = module_test.module
+hidden = TRUE
The user_modules_uninstalled() should have a call to drupal_load('module', $module) because the functions in module_test.module will not be loaded into the function registry during uninstall since the modules have been disabled.
+ foreach ($modules as $module) {+ drupal_load('module', $module);
+ $permissions = array_merge($permissions, array_keys(module_invoke($module, 'perm')));
+ }
#60
This is now a critical bug since user_modules_installed() automatically adds permissions to the admin role when modules are installed, but doesn't remove them - leading to PDO errors when you uninstall then reinstall a module.
Attached patch fixes bugs in the most recent one here, and adds additional tests by pp from #503554: user_modules_uninstalled() is missing
#61
The last submitted patch failed testing.
#62
Can't reproduce locally either running in the UI or via the command line. Trying once more.
#63
The last submitted patch failed testing.
#64
Forgot the -N flag when creating the patch, whoops.
#65
You do not need to run drupal_load() on each module. drupal_uninstall_modules() does this for you before firing the hook.
http://api.drupal.org/api/function/drupal_uninstall_modules/7
#66
doh, fixed.
#67
Sorry. Typos:
+ // Are the perms definced by module_test removed from {role_permission}.
+ * Implementation of hook_modules_uninstalled().
These should be fixed in the attached.
I am not sure what the aggregator bit is doing in the test function. Is that left over from another patch? If it's needed here, just say so.
#68
The aggregator test hunk is from another issue which was a straight bug report, but this issue is the proper fix for it. It tests that you can safely uninstall and reinstall a module (at the moment it throws a lot of PHP warnings due to user_modules_installed() trying to insert permissions for the admin role which were never removed.
#69
OK then!
#70
Committed to CVS HEAD. Thanks all!
I'd like to see us rename all instances of 'perm' to 'permission' so marking this 'code needs work'.
#71
This issue is already pretty long - I've opened #506976: Stop abbreviating permissions to perm so we can continue there.
Also added a short note about this to the 6/7 update documentation.
#72
The committed patch results in a couple of notices when you uninstall a module that doesn't implement hook_perm eg profile.module.
* Warning: array_keys(): The first argument should be an array in user_modules_uninstalled() (line 955 of modules\user\user.admin.inc).* Warning: array_merge(): Argument #2 is not an array in user_modules_uninstalled() (line 955 of modules\user\user.admin.inc).
#73
ack, untested patch.
#74
Is there a core module with no hook_perm() or maybe one of the test ones? We could add that to the uninstall test.
#75
The last submitted patch failed testing.
#76
And the path that was supposed to come with #53.
#77
The last submitted patch failed testing.
#78
Patch needs to update to hook_permission()
#79
#80
i think the code would be a tiny bit briefer if you use foreach (module_implements('permission') as $module). you won't loop over unnecessary modules and you can omit the drupal_function_exists().
#81
We don't want to remove permissions for every module which implements hook_permission(), only those ones which are being uninstalled and given as the parameter to hook_modules_uninstalled() - so module_implements() isn't an option here.
There's other ways to check if we get any permissions from a module before running array_keys() on it (that original hunk of code wasn't mine - I just re-rolled it into the patch which was committed), but they're all fairly equally verbose.
#82
ah right. makes sense now.
#83
Committed to CVS HEAD. Thanks.
#84
Automatically closed -- issue fixed for 2 weeks with no activity.