user_access: permissions too general
hass - May 5, 2007 - 22:11
| Project: | Go - url redirects |
| Version: | 5.x-1.2 |
| Component: | Code |
| Category: | feature request |
| Priority: | minor |
| Assigned: | spiderman |
| Status: | closed |
Description
Permission names like:
1. "Administrative Settings"
2. "edit entries"
3. "view entry list"
are not so good names. Change them to be more gotwo specific, please. Have a look how other modules are doing...
Some much better examples:
1. "administer gotwo"
2. "edit gotwo entries"
3. "view gotwo list"
this names will not have side effects on other modules. if someone else using the same unspecific names like above, this will result in user permissions that a user couldn't get. I have never seen a module using mixed case permission names.
#1
Valid suggestion, +1
#2
"administer gotwo" = DONE!
The remainder still require work but it's a *minor* problem.
#3
attached is a quick patch i just rolled, since i'm on a roll with this module today anyway ;)
i believe it covers all the cases of these perms getting used, and also adds a definition for the 'administer gotwo' perm in the _perm hook. seems to be working on my test site, but please review for correctness & completeness ;)
#4
volunteering to follow this one through
#5
Should we build in an automated upgrade path from old names to new names via .install file?
#6
@hass: good idea- how about something like this (off the top of my head, totally untested):
<?phpIndex: gotwo.install
===================================================================
RCS file: /cvs/drupal-contrib/contributions/modules/gotwo/gotwo.install,v
retrieving revision 1.1
diff -u -p -r1.1 gotwo.install
--- gotwo.install 4 Jul 2006 18:55:59 -0000 1.1
+++ gotwo.install 30 Oct 2007 03:50:04 -0000
@@ -32,3 +32,20 @@ function gotwo_install() {
break;
}
}
+
+# update the permissions table, to reflect changes to hook_perm
+function gotwo_update_5100() {
+ $res = db_query("SELECT rid,perm FROM {permission}");
+ $perms = array();
+ while ($p = db_fetch_object($res)) {
+ $perm = $p->perm;
+ $perm = preg_replace('/Administrative Settings/', 'administer gotwo', $perm);
+ $perm = preg_replace('/view entry list/', 'view gotwo entry list', $perm);
+ $perm = preg_replace('/edit entries/', 'edit gotwo entries', $perm);
+ $perms[$p->rid] = $perm;
+ }
+
+ foreach (array_keys($perms) as $rid) {
+ db_query("UPDATE {permission} SET perm = '%s' WHERE rid = %d", $perms[$rid], $rid);
+ }
+}
?>
i'll re-roll this patch tomorrow and test the update process on my test site :)
#7
here's a new patch which includes a hook_update_N implementation in the .install file, and works great on my test site. i should note that the old "Administrative Settings" perm wasn't actually being set by hook_perm, so the first preg_replace in this function may only apply to very old installations of this module..
#8
i've gone ahead and committed this patch, since it seems to be working fine, and i'm eager to move onto other things :)
the latest module code is here
#9
Automatically closed -- issue fixed for two weeks with no activity.