Change role id to machine name

The last submitted patch, drupal.rid_.0.patch, failed testing.

The last submitted patch, drupal.rid_.2.patch, failed testing.

The last submitted patch, drupal.rid_.4.patch, failed testing.

This issue is tackled in D7 by the Role Export module.

For Drupal 8 we should fix this properly:

• rename what is now "name" to "label"
• introduce a machine_name
• remove rid

I believe we did something similar to filter formats late in the D7 development cycle, so we should consider looking into it for best practices, drawbacks and impact on (contrib) modules.

New patch, now at least the Drupal minimal profile installation succeeds.

Discussion points:

• How should the machine name column in the role table be called? Currently: "name"
• What structure should user_roles() return? Currently: "An associative array with the role name as both the key and value." I think that is a good idea for better compatibility with existing code.

TODO:

• Fix all simpletests.
• Fix the role UI.
• Fix other core modules.
• Set this back to needs review when the testbot has failed.

The last submitted patch, 935062-role-names.patch, failed testing.

The last submitted patch, 935062-role-names.patch, failed testing.

Question: Do we still need the DRUPAL_ANONYMOUS_ROLE et al constants if we go this route? It's more typing than just the machine name, and 'anonymous_user' seems fairly clear to me.

The last submitted patch, 935062-role-names.patch, failed testing.

The last submitted patch, 935062-role-names.patch, failed testing.

Yay, tests pass now. I moved more code from the update function in user.install to update_prepare_d8_role_names() to always have a consistent state of new role names.

Now I wonder what we should do with the remaining rid column in the role table? I cannot remove it immediately during the upgrade because other modules might need the data to update their role references.

I took a closer look at the patch, and aside from the comment in #22 for which I'm still not sure/convinced, this looks like a good (and much needed) improvement.

Updated the issue summary.

Thanks fago. I think the TRUE flag for user roles makes sense, updated the patch to address that. Also renamed the "role" column in the block_role table to "role_name". And fixed the array_diff_key() thing.

New patch attached, there is also a diff to the previous patch in #21.

One question: how should the role machine name column be named in the role table? Currently it is just "name". Should that be "role_name", too?

Will also update the issue summary to address the changes.

Thanks testbot, fixed some array key handling in the simpletests and the wrong column in block.module.

interdiff is against #27.

The last submitted patch, 935062-role-names-interdiff-d8.patch, failed testing.

The last submitted patch, 935062-role-names_8.patch, failed testing.

The last submitted patch, 935062-role-names_8.patch, failed testing.

...and flipping status.

Fixed some more role constants that were introduced in the meantime. Interdiff is against #36.

Whew, this is a long patch to review. :) Overall I think it looks great. A few comments:

No need to select r.name twice; just call fetchAllKeyed(0, 0).

The role name column is length 64. I'm not sure what happens here if I have two identical 64-character names, but it's probably bad.

s/pimary/primary/g (several instances of it throughout)

If I'm reading this right, we should be testing whether the role is either anon or auth, and this is testing whether the role is anon.

I believe it's important to get the most recently assigned role (judging from the comments in #11218: Allow default text formats per role, and integrate text format permissions, where this was added). If we use end(array_keys($array)) it should preserve the current behavior.

Lowercase only isn't being enforced at the code level as far as I can tell - we should test with the full range of characters or limit the input to lowercase only.

If we're not going to use user_roles() here, I think we should at least addTag('translatable') so the labels get translated.

Also, one general question: Would it make sense to return an array structure of 'machine_name' => 'Human-readable name' instead of 'machine_name' => TRUE?

The last submitted patch, 935062-role-names.patch, failed testing.

Why do we need these constants now since we have machine names? Would anything cause them to change?

Yes, but we agreed that removing the constants is a followup issue. This issue is just about porting/removing role IDs.

This is totally awesome. I think this patch is close to ready, but I also think it needs upgrade path tests. Details (plus an armada of nitpicks) follow.

1. +++ b/core/includes/update.incundefined
@@ -200,6 +203,93 @@ function update_prepare_d8_language() {
+ * Prepare a minimal working D8 role name system. This is required for session
+ * bootstrapping during an upgrade from D7.

   The second sentence here should be moved to its own paragraph.

2. +++ b/core/includes/update.incundefined
@@ -200,6 +203,93 @@ function update_prepare_d8_language() {
+      //'not null' => TRUE,
...
+      //'not null' => TRUE,

   Shouldn't we delete these lines rather than commenting them out?

3. +++ b/core/modules/block/block.installundefined
@@ -199,6 +199,41 @@ function block_update_8000() {
+ * Role ids are replaced with role machine names.
...
+++ b/core/modules/user/user.installundefined
@@ -398,5 +383,38 @@ function user_update_8001() {
+ * Role ids are replaced with role machine names.

   "IDs" should have capitalization.

4. +++ b/core/modules/block/block.installundefined
@@ -199,6 +199,41 @@ function block_update_8000() {
+function block_update_8001() {
...
+++ b/core/modules/user/user.installundefined
@@ -398,5 +383,38 @@ function user_update_8001() {
+function user_update_8002() {

   Let's add upgrade path tests for both these updates. See: http://drupal.org/node/1429136

5. +++ b/core/modules/simpletest/drupal_web_test_case.phpundefined
@@ -1146,12 +1146,13 @@ class DrupalWebTestCase extends DrupalTestCase {
+      // We only want lower case machine names.

   "Lowercase" should be one word.

6. +++ b/core/modules/user/user.installundefined
@@ -398,5 +383,38 @@ function user_update_8001() {
+  // We do not need to add role_name database columns because they were already
+  // created earlier during the upgrade process.
+  // @see update_prepare_d8_role_names().

   Can someone confirm that the updates will run in this order? Edit: Never mind, I found the answer further up in the patch. However, I think this information is important enough to move up to the docblock. The inline comment can be its own paragraph, and the @see should be at the end after a blank line.

7. +++ b/core/modules/user/user.moduleundefined
@@ -2796,29 +2804,28 @@ function user_roles($membersonly = FALSE, $permission = NULL) {
+      // We only translate the built in role labels.

   "Built-in" should be hyphenated here.

8. +++ b/core/modules/user/user.testundefined
@@ -1900,41 +1902,38 @@ class UserRoleAdminTestCase extends DrupalWebTestCase {
+    $this->assertText(t('The role label has been renamed.'), t('The role has been renamed.'));
...
+    $this->assertEqual($new_role->label, $role_label, t('The role label has been successfully changed.'));

   t() is not needed on the assertion message texts because it is not going to be translated; it just adds overhead. Reference: http://drupal.org/simpletest-tutorial-drupal7#t

   (The same also applies to the other assertion messages changed by this patch, but I thought i'd point it out for these in particular since they're new.)

Only for testbot.

Here is the writeup about my upgrade path test struggle: http://klau.si/41

Rerolled to account for the most recent core changes (moving simpletests around).

1) Why did user_admin_roles() stop using user_roles() and switch to a query? Seems unnecessary (but if it is necessary, then it deserves a code comment).
2) Maybe the role name generating code from update_prepare_d8_role_names() should be its own function? Don't feel very strongly about it, but it crossed my mind.

Generally, this feels ready to me. Needs a review from someone nitpickier though.

The last submitted patch, 935062-role-names-58.patch, failed testing.

Let's do this!

The Configuration system tag is there because this issue is a blocker for converting user roles to the config system, so CMI needs to track it. It's not filed against the configuration system because it doesn't attempt to convert anything.

Alright, updated patch attached.

Note: for the anonymous and authenticated user roles we cannot take the old rid as machine name. Those two roles are hardcoded into Drupal and their machine name must be right. So contrib module upgrade paths will also have to special case those two roles, but as the machine names are constants and the mapping is always crystal clear I see no problem here.

The "administrator" role is a different story, with this patch it gets the machine name "3" in installations that base on the standard install profile. Ugly, but I guess that's ok.

Oh. I should have read your post more carefully, sorry.

My brain connects anything Drupal related ending in "id" automatically to serial integers, so I didn't think of actually keeping rid. But I agree, this might be a good idea and would drastically reduce the size of this patch as we would not have to change the two role constants.

I hope to find time tomorrow to work on this.

Upfront: I'm fairly sure we want to restrict machine names of any kind to [a-zA-Z0-9_]. Optionally also "-", but that's very rare (only used by menu names currently).

I'd recommend to simply drop the " user" suffixes.

Just noticed that we have now a URL path collision:

admin/people/permissions/roles (Roles overview page)
admin/people/permissions/% (Permissions for one specified role)
admin/people/permissions/list (Permissions overview page)

Previously the rid had to be an integer, so we could easily differentiate them from "roles" and "list", but now this is a problem.

Personally I think we should move the roles pages one level up and we would get rid of the problem. I never quite understood why the roles page is so deeply nested anyway. My suggestion:

admin/people/roles (Roles overview page)
admin/people/permissions/role/% (Permissions for one specified role)
admin/people/permissions/list (Permissions overview page)

What do you think?

Alright, this code lives in the platform sandbox now: http://drupalcode.org/sandbox/sun/1255586.git

I took the opportunity to fix, clarify, and clean up a couple of things. From a technical perspective, this patch looks pretty much ready to me.

However, this patch inherently introduces the notion of human-readable role labels. I already checked whether that could be split into a separate issue somehow, but that doesn't seem to be possible due to the D7 upgrade path being involved, which migrates the D7 role names into D8 role labels, while the role IDs are converted and retained as-is as respective machine names. So splitting that ain't possible.

Thus, we also need to cater for the resulting UI a bit. It might be possible to defer larger UI changes to a separate issue, but I guess this patch at least has to ensure some level of sanity.

On the role listing:

1. We likely want proper human-readable labels for the built-in roles. (i.e., not all-lowercase)
2. It seems wrong that I'm not able to edit the labels of the built-in roles.
3. The machine name widget looks very odd in this table UI. I wonder how we solved that for Field UI's Manage fields table?

Also, a technical note: the form structure for the new role form elements was changed to make the fields not required and use a custom form validation handler instead, in order to allow for the tabledrag/role weighting that's baked into the form in parallel here. That does not seem correct to me; we rather want to change the form structure and potentially leverage #limit_validation_errors to prevent the role addition fields from being validated when pressing the "Save order" button. I'll look into that later.

1. Similarly, we want to adjust the help text to contain examples for human-readable labels. (i.e., properly capitalized)

The last submitted patch, platform.roles_.73.patch, failed testing.

unrelated change, but I just pretend that I have not seen that.

So the patch looks good, thanks for the finetuning! (I'm not going to RTBC it as I wrote most of it)

Regarding the open points from #73: That is all non-critical followup stuff and should not stop this from being committed.

#1600892: Tests use magic numbers 1 and 2 instead of user role constants landed.

1. Fixed access denied error when editing built-in role names, and the simpletests.
2. Separeted operations into their own table columns.
3. Fixed capitalization of role name examples.

Also pushed to platform-roles-935062-klausi http://drupalcode.org/sandbox/sun/1255586.git/shortlog/refs/heads/platfo...

Fixed role name capitalization for built-in roles.

This looks RTBC to me, waiting for someone else to double check and change the status.

#83: 935062-role-names-83.patch queued for re-testing.

I read through the patch, the code looks good and so does the grammar :)

Yep, it's definitely missing the user_admin_role variable upgrade.

That looks sufficient. One more excellent reason for why the in-place serial ID to string conversion is just simply KISS :)

Cool, it even survived the kernel patch :-)

#91: 935062-role-names-91.patch queued for re-testing.

Rerolled.

Thanks for the quick re-roll. Committed/pushed to 8.x.

Thanks, I've revised and rewritten that :)

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

No, this cannot be backported to D7 as it is quite a heavy API change. Use role_export if you need stable role IDs in D7.

this might be not displaying correctly in some places.