user_modules_uninistalled() is missing

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

Actually since node types are handled through hook_node_info, it would be smart to uninstall those automatically. Same with hook_block().

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

ah, but now we do. from one of your favorite functions:

   $query = db_delete('node_access')->condition('nid', $node->nid);
    if ($realm) {
      $query->condition('realm', array($realm, 'all'), 'IN');
    }
    $query->execute();

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.

i think this should be postponed pending #253569: DX: Add hook_modules to act on other module status changes

Alright! We can finally get this in now! Although we should probably get some tests written for this.

That issues a delete query for each module. We can do a single delete for all modules by moving the query below the foreach().

Nope, #10 isnt it. That does a query per module as well. You can do this with one big IN clause.

was this more what you had in mind? we should probably add a test to ensure this works as advertised.

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: Remove module menus on uninstall.

well either of you could have re-rolled it without that ;) so tag now you guys get to write the tests ;)

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?

Progress, anyone?

Re-rolled. Any suggestions for which module could host the appropriate test files? Build off of the module enable/disable test?

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.

i added tests to this patch, not sure wether this is the right way to check it

I think the test could be a little simpler:

   // 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.'));

Also all the comments should be "Proper sentences."

thx for the review!
here is a better patch, i hope :)

+    // 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.

thx for taking the time to review

so there is another patch

Any reason this testcase isn't in ModuleUnitTest?

Probably because the ModuleUnitTest has a different setUp requirement than the uninstall test case. Will do further review shortly.

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

damn this cannot work!

sry for the 3 posts

so this time with a setup function

didn't test it but visually it's fine by me.

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

then modify assertModuleList() to add module_test and user in there.

You don't need to call setUp with the user module. It is automatically installed since it's a required core module.

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.

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

#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

arg, i created this module before, but i didn't added it to the patch file :(

so what other things could be removed automatically

• Blocks defined by hook_block_list | not sure whether this is in core from the {block} table
• Block roles in {block_role}, same as before
• filter in {filter}

sry for not checking them, whether they get removed

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?

Patch review:

class ModuleUninstallTest extends DrupalWebTestCase
Please 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 module name should be "Module test"

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')));
+  }

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

Can't reproduce locally either running in the UI or via the command line. Trying once more.

Forgot the -N flag when creating the patch, whoops.

doh, fixed.

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.

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'.

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.

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

ack, untested patch.

Is there a core module with no hook_perm() or maybe one of the test ones? We could add that to the uninstall test.

And the path that was supposed to come with #53.

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

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.

ah right. makes sense now.

Committed to CVS HEAD. Thanks.