It happens sometimes when I'm tired that I click on "Access rules" instead of "Access control"...

How about renaming "Access control" to "Permissions"?

CommentFileSizeAuthor
#23 permissions_10.patch13.37 KBJirkaRybka
#15 permissions_9.patch13.9 KBanders.fajerson
#12 permissions_8.patch12.38 KBanders.fajerson
#8 permissions_7.patch11.06 KBcatch
#6 permissions_6.patch5.61 KBcatch
permissions_5.patch688 bytesanders.fajerson

Comments

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

I agree. I've clicked the wrong one before as well, and "Permissions" is standard across a lot of systems. Tentatively marking RTBC.

catch’s picture

catch
he/him
Primary language English
commented

Hit submit too fast, sorry.

This would also make for less confusion with node_access modules.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Needs review

I agree, but I'd love to see some more people chime in about this.

dries’s picture

dries commented

I agree 300% but I think we want to change the path as well (admin/user/access should become admin/user/permissions).

anders.fajerson’s picture

anders.fajerson commented
Status: Needs review » Needs work

Agree, I'll roll a patch. This then needs to go into the D5 -> D6 documentation since a lot of modules have that link in their INSTALL.txt.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new permissions_6.patch5.61 KB

In which case we should change the permission to "administer permissions" for consistency.

anders.fajerson’s picture

anders.fajerson commented
Status: Needs review » Needs work

There should be more links to /access in core.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new permissions_7.patch11.06 KB

that's true.

Updated patch changes all admin/user/access to admin/user/permissions

it also changes "Access control" to "Permissions" where this was relevant (i.e. not in relation to file handling etc. where access control seems fine as general terminology)

anders.fajerson’s picture

anders.fajerson commented
Status: Needs review » Needs work

There probably has to be an upgrade path if a permission is renamed. I looked in system.install but couldn't find an example of this.

catch’s picture

catch
he/him
Primary language English
commented

also true. Requires a regexp or MySQL REPLACE on the permissions table.

update permissions set perm = replace(perm,'administer access control','administer permissions');

will work, although presumably not on pgsql.

I can't find any such permission update referenced on drupal.org though so hopefully someone will come along who knows.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

Instead of an SQL level regex, grab the permisisons to PHP, and replace there. It is more portable SQL wise.

anders.fajerson’s picture

anders.fajerson commented
Status: Needs work » Needs review
StatusFileSize
new permissions_8.patch12.38 KB

Here is a shot at it. Tested on MySQL.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Needs work

- Even after clearing the cache, permissions are still in admin/user/access, title is still 'Access control'.

- If I enable 'access administration pages', 'administer users' and 'administer permissions' for a role (auth users) and login with a user in that role, 'Access Control' does not appear in the menu (below 'Administer').

JirkaRybka’s picture

JirkaRybka commented
Status: Needs work » Needs review

I just tested the patch, and it works fine for me. The above problems were probably caused by patching an existing installation without necessary updates - I was able to reproduce these problems by patching on existing install, and I solved them as follows:

- The first bullet above just needed a menu-rebuild, to recognize new changes. Solved by enabling/disabling a module.

- The second is caused by renamed permission. Solved by update.php run.

Neither exists on a new install. So I can confirm that the patch works well, including the upgrade path. I'm not setting RTBC only just because I haven't time to check all the other changes in detail.

anders.fajerson’s picture

anders.fajerson commented
StatusFileSize
new permissions_9.patch13.9 KB

Found a leftover link in upload.module. Unless my search is failing me, this should be the last. The upgrade path still needs a code review and there needs to be a note about this change in http://drupal.org/node/114774 (Converting 5.x modules to 6.x). Here is a start:

"administer access control" permission renamed to "administer permissions"
The "administer access control" permission has been renamed to "administer permissions".

5.x:

if (user_access('administer access control')) {
  // ...
}

6.x:

if (user_access('administer permissions')) {
  // ...
}

"Access control" page renamed to "Permissions" and new URL

The page "Access control" (found at Administer > User management > Access control) has been renamed to "Permissions". The new URL to this page is /admin/user/permissions instead of /admin/user/access. Any documentation (e.g. INSTALL.txt) pointing to this page should be updated.

[Example needed?]

anders.fajerson’s picture

anders.fajerson commented

Here is a list with modules that uses the "administer access control" permissions in contrib. Even though it's a bit late in the release cycle to break modules it's not that many that will be affected...

accounttypes.module 1
level1.module 1
netforum_authentication.module 2
nf_handshake.module 1
phpids.module 1
roleassign.module 9
role_delegation 3
taxonomy_access 1

heine’s picture

heine commented
Version: 6.x-dev » 7.x-dev

We are already past the first beta and I don't think it is such a huge usability bug (isn't even marked as such) that it warrants a patch for 6.

nicholasthompson’s picture

nicholasthompson
Primary language English
commented

I +1 this idea too - I've always wondered why it was never called Permissions!

hass’s picture

hass commented

+1 - Very good usability change...

catch’s picture

catch
he/him
Primary language English
commented
Version: 7.x-dev » 6.x-dev

Since both Gabor and Dries have chimed in positively on this thread, moving back to 6.x for them to make the decision on whether it gets postponed or not.

JirkaRybka’s picture

JirkaRybka commented

Just to join the recent movement - +1 from me too, and rather a "big" one (as far as I can say that): For me, this is one of the things, which were difficult to figure out when I first started with Drupal.

anders.fajerson’s picture

anders.fajerson commented

Could we please stop voting on this and instead get a code review/test so we can set it to "ready to be commited"? - So core commiters can decide whether it's for Drupal 6 or 7.

JirkaRybka’s picture

JirkaRybka commented
StatusFileSize
new permissions_10.patch13.37 KB

OK, I tested this again - didn't apply after recent changes to comment module, so attaching the same patch re-rolled.

Otherwise I see no significant changes from the version I already examined: Page and relevant links renamed to 'permissions', one permission renamed with a working upgrade path provided, otherwise no change and everything works fine. I also can't find any more links to be updated (not that I tried too hard, though), and the whole seems good to me.

So if core commiters decide for D6, I would vote for RTBC.

JirkaRybka’s picture

JirkaRybka commented
Status: Needs review » Reviewed & tested by the community

Patch still applies with just one offset, and I hope there's nothing wrong if I set it to RTBC, as it's really how I feel about it. We were waiting just for a commiter's decision between 6.x and 7.x, which will be done prior to commit anyway I believe.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Needs work

I discussed this with Dries and he said this should be fixed, so looked over the patch line by line and as everything looked nice, committed.

Marking "patch (code needs work)" so the update docs will be updated.

Thanks.

anders.fajerson’s picture

anders.fajerson commented

Thanks. I don't have access to add documentation, but I started with some at #15.

catch’s picture

catch
he/him
Primary language English
commented

I do have access to docs, and have updated with text from #15 with minor additions. Thanks!

Marking back to fixed. If any more needs to be done to docs, please bump this issue/mark back to needs work.

This will also need to go into the new versioned core documentation which I believe is targeted at 6.x right?

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs work » Fixed
Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)