Clean up the Filter module upgrade path

Damien, is it really worth to fiddle with this? The filter* tables cannot contain much data either way, so those updates may not be perfect, but who cares, as long as they work?

I'm just trying to make sense of this - my gut feeling tells me that especially both your and my time would be much better spent on critical and major issues instead of beautifying unattractive functions that are invoked only once...? If I'm missing something important, please let me know.

+++ modules/user/user.install
@@ -360,6 +360,31 @@ function user_update_dependencies() {
+function _update_user_role_grant_permissions($rid, array $permissions, $module) {

Can we move this function into includes/update.inc?

Powered by Dreditor.

As far as I know there is nothing immediately broken here, so I'm downgrading from critical. Because filter.module is a required core module, it is always guaranteed to be enabled, and it is therefore safe to use user_role_grant_permissions() and other API functions.

That said, it is probably better not to (for the sake of setting a good example for other modules)...

And I also remember seeing a patch from @chx somewhere that would force all modules to act as disabled during the upgrade; if that gets in, then I guess we would absolutely need to do this.

user_role_grant_permissions call user_permission_get_modules(). We do not call modules as they can be disabled.

Committed to CVS HEAD. Thanks.

   // user_update_7006 relies on filter_update_7002.
   // TODO: move user_update_7006 down below in the upgrade process.
   $dependencies['user'][7006] = array(
-    'filter' => 7002,
+    'filter' => 7003,

It appears that first comment needs updating...?

Yes, there is that, plus a couple other small things I noticed. I'll see if I can whip up a quick patch.

But in terms of the larger changes, I think this could still use a little more discussion:

First of all, again, this is NOT a critical issue. user_role_grant_permissions() is broken when called for a disabled module's permissions, but the filter module can never be disabled, so there isn't a problem with it. Nevertheless, it does make sense to use the separate helper function here for consistency, since most other modules have to anyway.

But I am much less sure about this idea of never using any API functions at all - that I don't understand. Some we can't use, but lower-level ones such as filter_formats() and filter_permission_name(), I don't understand why the patch removed them? I think we should consider putting them back.

Here is a quick patch for the smaller issues. It should be mostly self-explanatory, except maybe for the cache clearing in _update_user_role_grant_permissions()... that is introduced simply because user_role_grant_permissions() has it, and if we are going to have two separate functions, we need to keep the behavior of those two as closely in sync as possible.

I think we should still discuss adding back filter_formats() and filter_permission_name() in the update functions, but I haven't done that in this patch.

looks RTBC to me if someone can do a re-roll.

However, I'm bumping down to minor as the latest patch is almost purely code sniffs on once-off code, which as @sun said in #4, is a low priority here.

Patch #16 re-rolled.

Doesn't look like the latest patch was tested manually... this won't work at all since the _update_user_role_grant_permissions() function no longer exists (it was changed to _update_7000_user_role_grant_permissions() a long time ago...)

Frankly I'm shocked that the automated tests didn't catch this; I guess we don't have any tests that deal with the Contact module update path :(

Done as suggested by @David.

See the Patch

Patch