Three roles now, not two -> explain the Administrator role on the Roles page!

The new administrator role is created in default.install as an extra role, so it's not hardcoded. I agree it might seem confusing, maybe, we should add more documentation on the install screen explaining what will happen during a default install (ie, 2 content types, new administrator role, some default links ..)

I think this was sufficiently cleaned up in the help text revamp.

Please forgive me for reopening this, but are we really unable to count to three?

admin/people/permissions/roles says

By default, Drupal comes with two user roles:

* Anonymous user: this role is used for users that don't have a user account or that are not authenticated.
* Authenticated user: this role is automatically granted to all logged in users.

Can't we detect whether the Administrator role was created, and add some helpful information here, if it is present? For example:

* Administrator: this role automatically receives all new permissions when you install a new module.
Warning: Obviously, this role should only be given to people you trust at least as much as yourself!

We take pains to point out dangerous permissions on the admin/people/permissions page, but we can't be bothered to explain the Administrator role? That doesn't seem right.

For reference: #480660: Add an 'administrator' role to core

If the Administrator role was not created during installation, but/or another role was subsequently assigned on admin/config/people/accounts, the text should be adjusted accordingly. This is a core feature, even if the role is installed "only" by the default (!) profile.

Oh, and no, I don't think the blob of text at the top of admin/people/permissions is sufficient. A role that automatically receives all new permissions is a time bomb, if the site maintainer is not constantly aware of the implications of that role. Ideally, it should be highlighted somehow wherever it appears, so that it really stands out.

BTW, in the current text "logged in" should be "logged-in".

This issue may be mostly out of date; the current help text on the Permissions page says this:

Permissions let you control what users can do and see on your site. You can define a specific set of permissions for each role. (See the Roles page to create a role). Two important roles to consider are Authenticated Users and Administrators. Any permissions granted to the Authenticated Users role will be given to any user who can log into your site. You can make any role the Administrator role for the site, meaning this will be granted all new permissions automatically. You can do this on the User Settings page. You should be careful to ensure that only trusted users are given this access and level of control of your site.

However, I think I agree with the suggestion in #4 that the administrator role should be highlighted somehow wherever it appears, so that it stands out. There may actually be another issue for that somewhere already.... can't remember.

Oops, sorry, I was looking at the wrong page...

The Permissions page text is good, but the above comments were actually about the Roles page (admin/people/permissions/roles), not the Permissions page :) And that one does indeed look like it could still use some work.

It still has to go in D8 first and then be backported. However, the hurdle for it to get in D7 is that it will break strings...

How about this?

I added a bit of logic to see if the administrator role is installed. Even though administrator role via default.install, I think it's an important enough role to deserve some explanation on admin/people/permissions/roles. I've changed it to say "By default, Drupal comes with three user roles:" if the administrator exists.

Thanks for working on this!

+++ b/modules/user/user.module
@@ -58,10 +58,17 @@ function user_help($path, $arg) {
+      $number_roles = $is_administrator_installed ? 'three' : 'two';

This needs to be a number to allow translation.

tagging

@salvis, Oh good call! I overlooked that. Here's the patch re-rolled with "two" and "three" translatable.

+++ b/modules/user/user.module
@@ -58,10 +58,20 @@ function user_help($path, $arg) {
+      if ($is_administrator_installed) {
+        $output .= '<p>' . t('By default, Drupal comes with three user roles:') . '</p>';
+      } else {
+        $output .= '<p>' . t('By default, Drupal comes with two user roles:') . '</p>';
+      }

now we have two almost identical messages for translation. Far from ideal, I suggest to just pass in the number as salvis suggested.

And: the else{} part has to start on a new line, see the Drupal coding standards.

3 days to next Drupal core point release.

@klausi, I don't think I understand what you and salvis mean by passing in the number. Could you explain?

Also, sorry about not putting the else on a new line. I'll get that next time around.

Make it a number rather than a word...

$number_roles = $is_administrator_installed ? 3 : 2;

... and pass that to t(), avoiding the need for translation.

-      $output .= '<p>' . t('By default, Drupal comes with two user roles:') . '</p>';
+      $is_administrator_installed = in_array('administrator', user_roles());
+      if ($is_administrator_installed) {
+        $output .= '<p>' . t('By default, Drupal comes with three user roles:') . '</p>';
+      } else {
+        $output .= '<p>' . t('By default, Drupal comes with two user roles:') . '</p>';
+      }

This will be misleading if role with name "administrator" is created in the minimal or custom installation profile.

+      if ($is_administrator_installed) {
+        $output .= '<li>' . t('Administrator: this role is automatically granted all new permissions when you install a new module.') . '</li>';
+      }

From "Account settings" page it is configurable to set which role to receive permissions automatically on module install. If this is not set to "administrator", again the above code will be misleading.

Attached is the patch that tries to address these two issues.

That makes sense that you want numerals like 3 and 2 to avoid translation, but it's generally more grammatically correct to spell out numbers less than 10.

This is displayed only to user 1. I think we can count on some tolerance.

+      $role_count = in_array('administrator', $roles) && (variable_get('install_profile') == 'standard') ? t('3') : t('2');

This looks problematic. The admin role might not be named 'administrator', and it might be installed by a different profile, so why are we checking for those?

Also, I don't believe there's any reason to translate '2' and '3' (or at least, Drupal never does that).

---

Perhaps this whole issue can be made a lot simpler just by rewording "By default, Drupal comes with two user roles" to something more like "Drupal has three special user roles" and go from there...? Then there would be no need to worry about install profiles or defaults or whether the admin role is present or anything.

Or alternatively, just kill this text entirely (it's too long) and move to something like help links or tooltips on the roles themselves to communicate the same information. That could be a different issue though.

How about this patch which looks for the user_admin_role variable?

Oops wrong status. Setting to needs review.

Tested patch in #21 and the text is now correct on admin/people/permissions/roles

I tried adding more roles and the number stayed at 3 as expected.
I renamed the administrator role to superuser and the number stayed at 3 as expected.
I deleted the administrator role and the number decreased to 2 as expected.

So before I use the patch I had the default help message "By default, Drupal comes with two user roles:" and after installed the patch the errors message change and becomes; "By default, Drupal comes with 3 user roles:".
You will see my screenshots.

@nattyseydi Thanks for the review. I'm not sure if you are stating the change from "two" to "3" as a problem with the patch or not. But to clarify, this was a requested change from @salvis in #10 and then @David_Rothstein pointed out in #20 that Drupal doesn't usually translate numbers like "2" and "3".

#21 needs work

+      $rid = variable_get('user_admin_role', 0);
+      $role_count = '2';
+      if ($rid) {
+        $admin_role = user_role_load($rid);
+        if (!empty($admin_role)) {
+          $role_count = '3';
+        }
+      }
+      $output .= '<p>' . t('By default, Drupal comes with @count user roles:', array('@count' => $role_count)) . '</p>';

As per the patch any custom created role set as "Administrator role" in "admin/config/people/accounts" is counted as default role. This is not true.

At #20

+      $role_count = in_array('administrator', $roles) && (variable_get('install_profile') == 'standard') ? t('3') : t('2');

This looks problematic. The admin role might not be named 'administrator', and it might be installed by a different profile, so why are we checking for those?

For instance, minimal install profile doesn't come with administrator role by default. Manually created role that takes the name "administrator" will be counted as default roles that's the reason I added install profile check.

@sivaji I think you are looking at an older version of the patch. #21 doesn't include those lines of code.

Also tested and it indeed changes to 3.

This looks better, but it still doesn't quite make sense to me.

Suppose I start off with the minimal profile, and the page tells me "By default, Drupal comes with 2 user roles". Then I add some roles manually and configure one to be the admin role, come back to the page and now it says "By default, Drupal comes with 3 user roles"... So what happened? Did my actions cause Drupal to go back in time and change its default behavior? :)

Again, I think the problem is with the phrase "By default". We simply don't know enough about how a particular installation of Drupal was set up initially to be able to meaningfully use that phrase.

If we instead say "Drupal has three special user roles" it becomes a whole lot simpler. The exact description of the admin role could change depending on whether the site is currently using it, but I think it makes sense to either always be there or not. Help text that can disappear isn't usually very helpful.

Suppose I start off with the minimal profile, and the page tells me "By default, Drupal comes with 2 user roles". Then I add some roles manually and configure one to be the admin role, come back to the page and now it says "By default, Drupal comes with 3 user roles"...

This is what I tried to address in my patch, eventually it adds more logic into function user_help() and makes it a bit hard to handle all the cases. I agree to revise the help text.

Ok this patch edits out the "By default" language and uses @David_Rothstein's suggestion of "Drupal has three special user roles" instead.

I left the admin user role description as a conditional based on whether it was configured or not. I think that it makes more sense for the admin help text to disappear if the user has deleted the admin user role than to include it and have the help text be incorrect.

Ah, I agree that #31 makes more sense.

+++ b/modules/user/user.moduleundefined
@@ -58,10 +58,23 @@ function user_help($path, $arg) {
+        $output .= '<li>' . t('@admin_role_name: this role is automatically granted all new permissions when you install a new module.', array('@admin_role_name' => ucwords($admin_role->name))) . '</li>';

This is a trivial point, but let's use ucfirst() here instead for consistency with the other role names.

Also, note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

I agree #31 is much better, but...

I think that it makes more sense for the admin help text to disappear if the user has deleted the admin user role than to include it and have the help text be incorrect.

Why would the help text have to be incorrect? (The idea would be to word it slightly differently in each case.)

@David_Rothstein I think my concern is that if the administrator help text is always there even if the user accidentally or intentionally deletes the administrator role, it would be incorrect.

It may also lead to a situation where the user then wants to re-assign the administrator role having been reminded by the help text that they could have a role that "is automatically granted all new permissions when you install a new module." I don't think there is a user interface for re-assigning that role, is there?

There is. You can reassign the admin role (or assign it for the first time if your install profile didn't start with one) at admin/config/people/accounts.

So my thought was that in the case where the admin role is off, the help text could point people to that page and tell them it's available to turn on, something like that. Not sure if it's worth the extra words or not, but it seemed better to me than having the help text disappear (at which point anyone who was relying on it won't be able to see it anymore :)

AH! I think that's a much better idea than having the help text disappear. :) I'll re-roll the patch with some amended language to that effect.

Re-rolled patch taking into account @David_Rothstein's suggestion in #35. I've changed the help text to be static at all times with a link to configure which role is the administrator role. I think this is much better because it doesn't require any conditional logic and is more grammatically correct (i.e. 'three' instead of '3').

This looks very good - I like how you managed to word it so it works in either case.

Some minor nitpicks/questions:

• Capitalization of "Account Settings" looks wrong (only the first word should be capitalized). To make that look reasonable I think it would be better like this: <a href="@account_settings">Account settings</a> page
• "Configure which custom role"... do we really need 'custom' there? I think I know why you added it (since it's not available to assign to anonymous/authenticated) but I wonder if it will confuse people in this context. It's probably the job of the Account settings page to explain the limitations on who this role can be assigned to, not the help text here (whose main purpose is just to inform the user of the admin role's existence)?

Thanks @David_Rothstein. I made the capitalization change. I also agree about the "custom" part in that I think it's the job of the Account settings page help text to explain how to assign the administrator role, but I snuck it in there just in case ;) I've removed it in the attached patch.

This is definitely better.

This changes one string and introduces another. However, a case could be made for backporting it since the current text is wrong. I'm leaving the backport tag on for now for the D7 branch maintainer to review the change and decide one way or another.

This needs to be bound to if(variable_get('user_admin_role')) it's not always there.

chx: See #35. We had it that way before and decided to change it. The idea is that the text explains how to set the role if it is not set.

OK, then

Committed to 8.x. Moving to 7.x for webchick to consider as it is a string change.

This was originally done deliberately because you only get 3 roles if you install Drupal 7 with the default profile.

However, the text change looks good, because it outlines that this third role is there, but also denotes that it's configurable and groups all of these under the general "special roles" group, so it remains factually accurate regardless of the profile used. Nicely done!

My gut feeling though is that this isn't really suitable for D7, even though it mostly qualifies as a bug fix. It seems like this change would require an awful lot of work for translators. We're not talking about fixing a small typo or a minor word change here or there, we're talking about brand spanking new strings with different meanings and ideas communicated that would require re-translation from scratch. :\

So moving back to 8.x and 'fixed' and removing the backport tag. If you feel this is a grave mistake, then let's get some representation here from some translator teams to explain why this would be OK to break in a minor point release.

Thanks @webchick. I think it makes sense not to port this back to D7 because of the amount of work it would require from translators.