Upgrade of forum.module d6-d7 loses permission

Interesting (I actually haven't worked with core forum module in a long while so the thought to see what is going on there completely escaped me). As of right now, I don't believe there is any test on creating forum content as part of the upgrade path. So having tests that do this would be a good idea.

So looking this through, forum used to have the following permissions in D6:

• create forum topics
• delete own forum topics
• delete any forum topics
• edit own forum topics
• edit any forum topics
• administer forums

'administer forums' is still around (is that working correctly?) but the others have changed to (as part of the node module from your observation):

• create forum content
• delete own forum content
• delete any forum content
• edit own forum content
• edit any forum content

So we need to see how the permissions are moved over and we can have an update function to change the forum permissions to be those (do those permissions exist in a table after the upgrade?).

Ok, this is very strange...looking at system_update_7000, this issue should have been handled...but its not :/ Will investigate further.

Wow...I looked at system_update_7000, and this is what is currently in the function:

    $renamed_permission = preg_replace('/(?<=^|,\ )create\ blog\ entries(?=,|$)/', 'create blog content', $role->perm);
    $renamed_permission = preg_replace('/(?<=^|,\ )edit\ own\ blog\ entries(?=,|$)/', 'edit own blog content', $role->perm);
    $renamed_permission = preg_replace('/(?<=^|,\ )edit\ any\ blog\ entry(?=,|$)/', 'edit any blog content', $role->perm);
    $renamed_permission = preg_replace('/(?<=^|,\ )delete\ own\ blog\ entries(?=,|$)/', 'delete own blog content', $role->perm);
    $renamed_permission = preg_replace('/(?<=^|,\ )delete\ any\ blog\ entry(?=,|$)/', 'delete any blog content', $role->perm);

    $renamed_permission = preg_replace('/(?<=^|,\ )create\ forum\ topics(?=,|$)/', 'create forum content', $role->perm);
    $renamed_permission = preg_replace('/(?<=^|,\ )delete\ any\ forum\ topic(?=,|$)/', 'delete any forum content', $role->perm);
    $renamed_permission = preg_replace('/(?<=^|,\ )delete\ own\ forum\ topics(?=,|$)/', 'delete own forum content', $role->perm);
    $renamed_permission = preg_replace('/(?<=^|,\ )edit\ any\ forum\ topic(?=,|$)/', 'edit any forum content', $role->perm);
    $renamed_permission = preg_replace('/(?<=^|,\ )edit\ own\ forum\ topics(?=,|$)/', 'edit own forum content', $role->perm);

    if ($renamed_permission != $role->perm) {
      db_update('permission')
        ->fields(array('perm' => $renamed_permission))
        ->condition('rid', $role->rid)
        ->execute();
    }

I might be losing my head, but does role->perm actually change with each preg_replace? It seems like the code should really be

    $renamed_permission = $role->perm;
    $renamed_permission = preg_replace('/(?<=^|,\ )create\ blog\ entries(?=,|$)/', 'create blog content', $renamed_permission);
    $renamed_permission = preg_replace('/(?<=^|,\ )edit\ own\ blog\ entries(?=,|$)/', 'edit own blog content', $renamed_permission);
    $renamed_permission = preg_replace('/(?<=^|,\ )edit\ any\ blog\ entry(?=,|$)/', 'edit any blog content', $renamed_permission);
    $renamed_permission = preg_replace('/(?<=^|,\ )delete\ own\ blog\ entries(?=,|$)/', 'delete own blog content', $renamed_permission);
    $renamed_permission = preg_replace('/(?<=^|,\ )delete\ any\ blog\ entry(?=,|$)/', 'delete any blog content', $renamed_permission);

    $renamed_permission = preg_replace('/(?<=^|,\ )create\ forum\ topics(?=,|$)/', 'create forum content', renamed_permission);
    $renamed_permission = preg_replace('/(?<=^|,\ )delete\ any\ forum\ topic(?=,|$)/', 'delete any forum content', $renamed_permission);
    $renamed_permission = preg_replace('/(?<=^|,\ )delete\ own\ forum\ topics(?=,|$)/', 'delete own forum content', $renamed_permission);
    $renamed_permission = preg_replace('/(?<=^|,\ )edit\ any\ forum\ topic(?=,|$)/', 'edit any forum content', $renamed_permission);
    $renamed_permission = preg_replace('/(?<=^|,\ )edit\ own\ forum\ topics(?=,|$)/', 'edit own forum content', $renamed_permission);

    if ($renamed_permission != $role->perm) {
      db_update('permission')
        ->fields(array('perm' => $renamed_permission))
        ->condition('rid', $role->rid)
        ->execute();
    }

Does that make sense?

Adding tags since this affects Drupal.org.

Here is a quick patch. This updates system_update_7000 by setting up renamed_permissions correctly. And system_update_7075 should now create new permission changes as well.

My line of thinking at the time was that any permissions that were left over that still had the old permission names would get taken care of with the newest upgrade function (instead of having the person run upgrade_7000). But in hindsight...if they resave the permissions page, those sets of permissions are gone, right? That part could probably be removed in that case.

For the upgrade tests...yikes. Do we have *any* upgrade related tests that test out access functionality? I'll try and look into this to see what's possible.

In the meanwhile, smaller patch.

Ok, I think I figured out a really simple way around this patch without needing to recreate a D6 dump with another user - add a permission for all roles to be able to create forum topics (We could do the same for the ability to delete own/any topics though that will require something much more expansive). The test seems to be working well (fail without patch, pass with). Time for review.

Very briefly looking at http://api.drupal.org/api/drupal/modules!user!user.module/function/user_..., it seems like the permissions may still linger around (I thought it rebuilt the entire permissions tree for a role by clearing out all the entries) so it seems like an update hook should bring it back. In which case #8 would make sense - the new update function in there is to update any permissions that didn't make their way through in the inital hook_update_7000 due to the weirdness that was going on (it seems to me like whatever issue was going on with forum was also happening with blog - really strange to not have heard about it since launch?) We could just have the update hook be the thing that is in there instead of modifying the system_7000 update.

Here is a patch with just a new update function + tests.

Should we maybe consider adding a helper function to rename permissions?

The reason the code is so different is in D6, the permissions for a given role are stored in 1 row so the permission was essentially in this format: 1|1|create node, add user, so on, so forth. But in D7, the permission table split out a given permission for each role into a row so it is now:
1|1|create node
1|1|add user
1|1|so on
1|1|so forth

So the update function can't really be shared between the two.

Ok, this is #8 plus the tests from #11. Unassigning.

Reroll to fix a whitespace thingie

Nice catch. Committed and pushed to 7.x. Thanks!

Possible issue here: #1816148: Error in upgrade due to system_update_7075()