In admin/user/permissions and admin/user/roles, the two system roles "anonymous user" and "authenticated user" usually are shown in front of any other custom roles. However this is not the case if either there a custom role that comes alphabetically before "anonymous user" or the role names are translated.

We should make sure these two special roles are shown first.
The best way would be to add to the "role" table a column "system" with 1 for the two system roles and 0 for any custom roles.
As this theoretically might affect contrib modules, it could be postponed to D7.
In this case we could manually extract the two roles from the query and add them as the first two entries. Wouldn't be a nice approach though.

I'll provide a patch as soon as I get any response on how to proceed.

CommentFileSizeAuthor
#11 system_roles-11.patch2.48 KBpancho
#6 system_roles-6.patch2.51 KBpancho
#4 system_roles-4.patch2.51 KBpancho
#3 system_roles-3.patch2.07 KBtraxer
#2 system_roles.patch2.06 KBpancho
roles.png31.57 KBpancho

Comments

traxer’s picture

traxer commented

Sounds like a good idea. There are two constants defined in bootstrap.inc:

/**
 * Role ID for anonymous users; should match what's in the "role" table.
 */
define('DRUPAL_ANONYMOUS_RID', 1);

/**
 * Role ID for authenticated users; should match what's in the "role" table.
 */
define('DRUPAL_AUTHENTICATED_RID', 2);

Theses make a "system" column redundant.

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented
Assigned: Unassigned » pancho
Status: Active » Needs review
StatusFileSize
new system_roles.patch2.06 KB

Thanks traxer for your comment,

you made me reconsider this, and I came up with an elegant and generic solution without changing db.

As this issue doesn't only apply to admin/user/permissions and admin/user/roles but also to admin/settings/filter/n and probably one or the other contrib use case, the only way to go was a generic one.

I therefore implemented the sort order in user_roles(), where I reserve the first two array entries for the two system roles.

In the end, all my patch does is changing the order of the roles array that is being returned by user_roles(): anonymous comes first, authenticated second, and all the other roles follow in alphabetical order.

One more change is involved: user_admin_perm() now uses the generic API function user_roles() instead of retrieving roles "manually" as it did before.

I tested my patch in all three use cases and it seems to be perfectly sane. As far as contrib is concerned, while this is a minimal behavior change of an API function, I can't think of any situation where this would be problematic. In the contrary, it should be favorable in all cases.

For testing, please take a look at http://api.drupal.org/api/function/user_roles/6/references where all functions using user_roles() are listed.

traxer’s picture

traxer commented
StatusFileSize
new system_roles-3.patch2.07 KB

Great idea. It's nice to keep the order of roles consistent accross the UI.

I cleaned up user.module a bit and changed to strict comparison (===) when "Unset[ting] system roles if not matched". I also fixed a bug in user_admin_perm(): when a $rid was passed, the $role_names array was not build properly.

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented
StatusFileSize
new system_roles-4.patch2.51 KB

Thanks for the fix, I just replaced "=== NULL" by "!isset()" as this is the more common style here. Also removing some excessive lines.
Next tester maybe wants to set this rtbc?

traxer’s picture

traxer commented

$roles[DRUPAL_ANONYMOUS_RID] is initialized to NULL explicitly. So this is what should be tested against (explicitly) to decide whether to unset roles. Also, to the casual reader, a line like if (! isset($var)) unset($var); makes absolutely no sense.

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented
StatusFileSize
new system_roles-6.patch2.51 KB

You're absolutely right - I return to strict comparison as suggested by you in #3.
Would be great if we had at least one more review...

dropcube’s picture

dropcube commented

It works. Effectively, after applying the patch System roles are showed before custom added roles.

catch’s picture

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

Works for me too.

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented

Don't wanna be too pushy, but this one directly blocks #18954. Would be great to move forward asap...

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 don't like how complicate this is done. I'd suggest using array_filter() without a callback parameter instead at the last part, which would be a one line solution to remove the unwanted roles. See http://php.net/array_filter I doubt we need to support role names like '' or '0', which could be the only two problematic role names with array_filter().

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented
Status: Needs work » Needs review
StatusFileSize
new system_roles-11.patch2.48 KB

Thanks for your valuable suggestion. Using array_filter is much better indeed.
I thought about ruling out '0' as role name in user_admin_role_validate(), but was not sure whether it would really be necessary.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Fixed

Committed this one, thanks.

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented

Thanks for committing! Now I can proceed to #18954...

traxer’s picture

traxer commented

I doubt we need to support role names like '' or '0', which could be the only two problematic role names with array_filter().

Such role names are indeed ruled out by user_admin_role_validate().

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented

You are right! So we don't have to worry about that.

Anonymous’s picture

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

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