String freeze: "Access" versus "View" in Permissions
In admin/user/permissions I can set permissions to:
access news feeds
access printer-friendly version
access comments
access site-wide contact form
access content
view revisions
access statistics
view post access counter
access administration pages
view uploaded files
access user profiles
As far as I can tell, all these except access site-wide contact form (and perhaps access administration pages?) give permission to look at different parts of the site. I suggest using the same word for all these. When we use different words, users expect them to describe different kinds of permissions.
To me, "access" sounds like it gives more than viewing permissions, it sounds like it gives permission to do something. Therefore, I suggest using "view". Admittedly, English isn't my native language, but after looking at "access" in a few dictionaries, I think my understanding of the word is pretty mainstream.
#1
#2
#3
In cases like the contact form, view sounds quite bad, access seems more appropriate. While I agree that these can be improved, since these are permission names, it is past the point we can modify them (they need update functions, contrib modules might depend on them etc). We already did some permission modifications late, and now we are past the point where this would be workable. This would be a great improvement in Drupal 7 though.
#4
Interesting improvement. I agree that there should be consistency here. I think that 'access' should be used when the user can do something with the permission while view should be used when the user can only see information. The following is a list of permissions using 'access' or 'view' (and the odd 'inspect' from poll.module) with a few modifications based on that definition.
AccessView news feedsAccessView printer-friendly versionAccessView contentInspectView all votesAccessView statisticsAccessView site reportsAccessView user profilesIf others agree that these changes are good, I'll put together a quick patch.
#5
+1 from me :-)
#6
tagging for novice queue
#7
Since the last person mentioning a patch was at the end of January, I took a shot at creating a patch with the necessary changes.
P.S. - Please be kind as this is my first patch :)
#8
The last submitted patch failed testing.
#9
I tweaked my GREPs to find some missing permissions and tests.
#10
#11
The last submitted patch failed testing.
#12
#13
Nice work so far. Definitely a solid start. I believe you have successfully changed all of the perms to the new wording throughout the code, but there is still a bit of work to do.
@@ -87,7 +87,7 @@ function book_update_6000() {This was probably just an overlooked changed by a bulk change, you should not be touching old update functions.
Secondly, you need to account for updates. When a site is being updated from Drupal 6, it is going to have the old permission strings stored in the database. Any permission string you change also needs an associated update in the .install file in hook_update_7000().
See the book_update_6000() referenced above for an example of this. Permissions for book were changed a bit from D5=>D6, so you can use this as a starting point for all of these changes.
#14
Added Update functions and fixed an update which I should not have touched...
#15
The last submitted patch failed testing.
#16
Very good, especially for a first patch! Few minor things:
+ * Update permissions to use new namesNeeds a period at the end.
+ // Fix role permissions to account for the changed names+ // Setup the array holding strings to match and the corresponding
+ // strings to replace them with.
Also needs a period. Newlines for new sentences are not necessary. You can just keep on going until you hit the 80 characters.
+ // Replace all the old permissions with the corresponding new permissions.+ $fixed_perm = strtr($role->perm, $replace);
+ // Only save if the permissions have changed.
+ if ($fixed_perm != $role->perm) {
+ $ret[] = update_sql("UPDATE {permission} SET perm = '$fixed_perm' WHERE rid = $role->rid");
+ }
This can be done simpler:
// Only save if the permissions have changed.if (isset($replace[$role->term])) {
$ret[] = update_sql("UPDATE {permission} SET perm = '$replace[$role->term]' WHERE rid = $role->rid");
}
Keep up the good work! :)
#17
It seems that the only two from that list that weren't changed were:
Contact: Access site-wide contact form
System: Access administration pages
I personally don't see a problem with using 'View' for these. As English is my native tongue, I don't see these as being clunky to say.
Contact: View site-wide contact form
System: View administration pages
While I can understand the OP position with using "Access" to describe an action (i.e it sounds like it gives permission to do something). I would opt for renaming "Access" to "Use", but thats another issue, I suppose.
Contact: Use site-wide contact form
System: Use administration pages
#18
Doubleplusone to "use" in those two cases! That's a lot clearer and more informative than access, at least to my non-native-English ears :-)
Just to be clear: It's not the clunkyness of the phrases I'm worried about, it's the information contained in them. Both "view" and "use" are easy to understand.
#19
Should we open another issue for these two cases? Or should the discussion stay within this issue?
#20
I'm not sure, although I have already rolled the "use" changes into the next patch...
#21
status to needs review then
#22
The last submitted patch failed testing.
#23
Full updated, applied the patch, fixed issues, created new patch, and *remembered to change status*. :)
#24
Correction, I forgot to change the status...
#25
The wording change seems solid, it is a good thing we are using more clear language for this - acces sounds to heavy for that cause.
#26
The last submitted patch failed testing.
#27
#28
The last submitted patch failed testing.
#29
This is a good idea, but I think it's too late for 7.x code slush.
If we re-rolled and applied this patch now, it would break the api that modules are relying on these access permissions.
I'm all for intelligent naming conventions. The notes about "use" and "view" are good. use/access imply read/write. view implies read.
Let's do this in 8.x first thing, so that it doesn't get any bigger (103k patch is alot). Or, break into smaller issues. Maybe module by module?
Moving to 8.x
#30
We can still do this for the permission titles.