It seems that when you upgrade forum.module from d6 to d7, the permission for creating forum topic nodes is being lost. For instance, if you have it set in d6 so that authenticated users can create forum nodes, in d7 you will lose that.

I'm inferring this from the d.o upgrade -- on the live d.o anyone can create forum topic nodes, but on a d7 dev site created by taking the d.o database (or a subset of it) and running the upgrades, everyone lost the ability to create new forum topic nodes.

The permission has apparently moved into the node module by the way, but I think there should be an update function that sets the permission on the appropriate roles.

Files: 
CommentFileSizeAuthor
#21 1685110.combined_20.patch5.93 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 39,359 pass(es).
[ View ]
#19 1685110.combined.patch5.47 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 39,268 pass(es).
[ View ]
#15 1685110.combined.patch2.6 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 39,268 pass(es).
[ View ]
#11 1685110.testonly.patch1.32 KBBTMash
FAILED: [[SimpleTest]]: [MySQL] 39,290 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#11 1685110.patch4.36 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 39,261 pass(es).
[ View ]
#10 1685110.patch3.04 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 39,265 pass(es).
[ View ]
#8 1685110.patch4.15 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 39,267 pass(es).
[ View ]

Comments

BTMash’s picture

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.

BTMash’s picture

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

BTMash’s picture

Assigned:Unassigned» BTMash

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

BTMash’s picture

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?

drumm’s picture

Issue tags:+Drupal.org D7, +porting

Adding tags since this affects Drupal.org.

jhodgdon’s picture

Issue tags:-Drupal.org D7, -porting

RE #4 - you are not losing your head -- that looks correct. preg_replace returns the fixed text, and doesn't change the passed-in string by reference or anything fancy like that. I just checked
http://us.php.net/manual/en/function.preg-replace.php
to be sure.

Sounds like we need a patch, and ... can/should we also make a new update function that would fix the ones that were missed the first time through?

jhodgdon’s picture

Issue tags:+Drupal.org D7, +porting

whoops, cross-post

BTMash’s picture

Status:Active» Needs review
StatusFileSize
new4.15 KB
PASSED: [[SimpleTest]]: [MySQL] 39,267 pass(es).
[ View ]

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.

jhodgdon’s picture

Hm. The changes in update_7000 look fine, but I don't understand the new update hook, so I guess I'll leave that for others to review (or can you explain what is going on there BTMash?).

Also this might need a test in the upgrade tests?

BTMash’s picture

StatusFileSize
new3.04 KB
PASSED: [[SimpleTest]]: [MySQL] 39,265 pass(es).
[ View ]

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.

BTMash’s picture

StatusFileSize
new4.36 KB
PASSED: [[SimpleTest]]: [MySQL] 39,261 pass(es).
[ View ]
new1.32 KB
FAILED: [[SimpleTest]]: [MySQL] 39,290 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

jhodgdon’s picture

I think we still need to figure out if there is a way to fix sites that already went through the broken upgrade hook... although as pointed out above, if they've visited and saved the Permissions page, they might have lost even the broken permissions... Is that true? I haven't looked at the permission page save code to verify that it loses anything that it doesn't recognize.

Other than that, assuming the test bot agrees (test-only fails, full patch passes), I think this is good to go!

BTMash’s picture

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.

jhodgdon’s picture

OK, sounds like fixing the permission with a second update hook is a good idea then... But what I still don't get is why the patch in #8 has the code in system_update_7000() being so different from the new code in system_update_7075()? And maybe it would be good to put the code that does the fixing into a helper function and call it from both update functions?

BTMash’s picture

StatusFileSize
new2.6 KB
PASSED: [[SimpleTest]]: [MySQL] 39,268 pass(es).
[ View ]

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

joachim’s picture

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

BTMash’s picture

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.

jhodgdon’s picture

RE #17 - Ah, that makes sense!

So, I think we should still go back and fix the broken update function as well as adding the new one. There is no sense keeping around a broken update function that I can see.

BTMash’s picture

Assigned:BTMash» Unassigned
StatusFileSize
new5.47 KB
PASSED: [[SimpleTest]]: [MySQL] 39,268 pass(es).
[ View ]

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

jhodgdon’s picture

Status:Needs review» Reviewed & tested by the community

This looks good to me -- thanks for all the work and explanations!

aspilicious’s picture

StatusFileSize
new5.93 KB
PASSED: [[SimpleTest]]: [MySQL] 39,359 pass(es).
[ View ]

Reroll to fix a whitespace thingie

webchick’s picture

Status:Reviewed & tested by the community» Fixed

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

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

David_Rothstein’s picture