Problem/Motivation

User roles are configuration items, therefore they should use machine names as identifiers and not serial integers. This is important for the exportability of roles. There is the Role Export module for Drupal 7 that works around the integer identifier in a hackish way. Drupal 8 should be fixed to use role machine names only.

Proposed resolution

Change all occurrences of role ids (rid) in Drupal from integers to strings (machine names).

Remaining tasks

User interface changes

  • When adding a new role a role ID machine name field has to be filled out also.
  • The role id is displayed as machine name on the role edit page.

API changes

The user/account object changes regarding roles: The array keys of the roles array are not numeric role ids anymore but machine name strings. The human readable role name (label) is kept as array value for now. Old role system:

<?php
$user
->roles = array(
 
2 => 'authenticated user',
 
3 => 'administrator',
 
4 => 'site editor',
);
?>

New system:
<?php
$user
->roles = array(
 
'authenticated' => 'Authenticated user',
 
'administrator' => 'Administrator',
 
'site_editor' => 'Site editor',
);
?>

The function user_role_load_by_name() has been removed, as the role ID is the machine readable name now.

Original report by sun

Same as #934050: Change format into string, for #933846: Better distro support

Files: 
CommentFileSizeAuthor
#98 935062-role-names-98.patch40.87 KBklausi
PASSED: [[SimpleTest]]: [MySQL] 36,847 pass(es).
[ View ]
#91 935062-role-names-91.patch40.87 KBklausi
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 935062-role-names-91.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#91 935062-role-names-91-interdiff.txt963 bytesklausi
#83 935062-role-names-83.patch40.16 KBklausi
PASSED: [[SimpleTest]]: [MySQL] 36,723 pass(es).
[ View ]
#83 935062-role-names-83-interdiff.txt1.3 KBklausi
#82 platform.roles_.82.patch40.88 KBsun
PASSED: [[SimpleTest]]: [MySQL] 36,708 pass(es).
[ View ]
#81 935062-role-names-81.patch40.09 KBklausi
PASSED: [[SimpleTest]]: [MySQL] 36,714 pass(es).
[ View ]
#81 935062-role-names-81-interdiff.txt6.68 KBklausi
#80 platform.roles_.80.patch37.99 KBsun
PASSED: [[SimpleTest]]: [MySQL] 36,706 pass(es).
[ View ]
#80 interdiff.txt12.08 KBsun
#79 platform.roles_.79.patch31.42 KBsun
PASSED: [[SimpleTest]]: [MySQL] 36,708 pass(es).
[ View ]
#76 platform.roles_.75.patch38.95 KBsun
PASSED: [[SimpleTest]]: [MySQL] 36,709 pass(es).
[ View ]
#73 platform.roles_.73.patch38.96 KBsun
FAILED: [[SimpleTest]]: [MySQL] 36,361 pass(es), 358 fail(s), and 0 exception(s).
[ View ]
#73 interdiff.txt16 KBsun
#73 role-list-add.png7.95 KBsun
#73 role-edit.png5.01 KBsun
#70 935062-role-names-70.patch38.72 KBklausi
PASSED: [[SimpleTest]]: [MySQL] 36,714 pass(es).
[ View ]
#70 935062-role-names-70-interdiff.txt4.11 KBklausi
#68 935062-role-names-68.patch38.27 KBklausi
FAILED: [[SimpleTest]]: [MySQL] 36,686 pass(es), 27 fail(s), and 8 exception(s).
[ View ]
#65 935062-role-names-65.patch116.45 KBklausi
PASSED: [[SimpleTest]]: [MySQL] 36,648 pass(es).
[ View ]
#65 935062-role-names-65-interdiff.txt9.64 KBklausi
#58 935062-role-names-58.patch116.96 KBklausi
PASSED: [[SimpleTest]]: [MySQL] 36,637 pass(es).
[ View ]
#58 935062-role-names-58-interdiff.txt1.32 KBklausi
#56 935062-role-names-56.patch117.25 KBklausi
PASSED: [[SimpleTest]]: [MySQL] 36,629 pass(es).
[ View ]
#55 935062-role-names-55.patch117.39 KBklausi
PASSED: [[SimpleTest]]: [MySQL] 35,094 pass(es).
[ View ]
#52 935062-role-names-52.patch117.42 KBklausi
PASSED: [[SimpleTest]]: [MySQL] 35,950 pass(es).
[ View ]
#52 935062-interdiff-52-do-not-test.patch6.87 KBklausi
#50 role-name-935062-50.patch112.37 KBoriol_e9g
PASSED: [[SimpleTest]]: [MySQL] 35,906 pass(es).
[ View ]
#44 935062-role-names.patch112.44 KBklausi
PASSED: [[SimpleTest]]: [MySQL] 35,717 pass(es).
[ View ]
#44 935062-interdiff-do-not-test.patch1.88 KBklausi
#42 935062-role-names.patch112.34 KBklausi
FAILED: [[SimpleTest]]: [MySQL] 35,681 pass(es), 7 fail(s), and 4 exception(s).
[ View ]
#42 935062-interdiff-do-not-test.patch5.51 KBklausi
#40 935062-40.role-names.patch112.37 KBksenzee
PASSED: [[SimpleTest]]: [MySQL] 35,683 pass(es).
[ View ]
#39 935062-role-names.patch112.39 KBklausi
PASSED: [[SimpleTest]]: [MySQL] 35,667 pass(es).
[ View ]
#39 935062-interdiff-do-not-test.patch4.07 KBklausi
#36 935062-role-names_36.patch108.67 KBwamilton
FAILED: [[SimpleTest]]: [MySQL] 34,618 pass(es), 1,337 fail(s), and 2,374 exception(s).
[ View ]
#34 935062-role-names_8.patch108.67 KBklausi
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/user/user.install.
[ View ]
#32 935062-role-names_8.patch108.64 KBklausi
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 935062-role-names_8_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#29 935062-role-names.patch108.64 KBklausi
PASSED: [[SimpleTest]]: [MySQL] 34,271 pass(es).
[ View ]
#29 935062-role-names-interdiff-d8.patch3.29 KBklausi
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 935062-role-names-interdiff-d8_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#27 935062-role-names.patch108.29 KBklausi
FAILED: [[SimpleTest]]: [MySQL] 33,236 pass(es), 40 fail(s), and 18 exception(s).
[ View ]
#27 935062-role-names-interdiff-d8.patch13.73 KBklausi
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 935062-role-names-interdiff-d8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#21 935062-role-names.patch108.14 KBklausi
PASSED: [[SimpleTest]]: [MySQL] 33,347 pass(es).
[ View ]
#20 935062-role-names.patch108.67 KBklausi
PASSED: [[SimpleTest]]: [MySQL] 33,354 pass(es).
[ View ]
#18 935062-role-names.patch105.23 KBklausi
FAILED: [[SimpleTest]]: [MySQL] 34,139 pass(es), 4 fail(s), and 2 exception(es).
[ View ]
#14 935062-role-names.patch102.1 KBklausi
FAILED: [[SimpleTest]]: [MySQL] 34,121 pass(es), 4 fail(s), and 2 exception(es).
[ View ]
#12 935062-role-names.patch92.85 KBklausi
FAILED: [[SimpleTest]]: [MySQL] 34,064 pass(es), 64 fail(s), and 148 exception(es).
[ View ]
#10 935062-role-names.patch74.56 KBklausi
FAILED: [[SimpleTest]]: [MySQL] 33,549 pass(es), 380 fail(s), and 355 exception(es).
[ View ]
#9 935062-role-names.patch53.88 KBklausi
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]
#8 935062-role-names.patch32.33 KBklausi
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#4 drupal.rid_.4.patch11.42 KBsun
FAILED: [[SimpleTest]]: [MySQL] 25,915 pass(es), 60 fail(s), and 7 exception(es).
[ View ]
#2 drupal.rid_.2.patch10.74 KBsun
FAILED: [[SimpleTest]]: [MySQL] 7,558 pass(es), 281 fail(s), and 649 exception(es).
[ View ]
drupal.rid_.0.patch9.3 KBsun
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Comments

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new10.74 KB
FAILED: [[SimpleTest]]: [MySQL] 7,558 pass(es), 281 fail(s), and 649 exception(es).
[ View ]

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new11.42 KB
FAILED: [[SimpleTest]]: [MySQL] 25,915 pass(es), 60 fail(s), and 7 exception(es).
[ View ]

Status:Needs review» Needs work

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

Version:7.x-dev» 8.x-dev

This is undoubtedly Drupal 8 material.

Relying on the role to do *anything* is a bug by itself, as the role is (and has always been) a mere technical artifact. Core provides only one access control system and it is based on *permissions* (whose names are unique), not roles. [I'm aware that the block system rely on roles, but it's another story.

Moreover, {role}.name is *already* a machine name:

<?php
     
'name' => array(
       
'type' => 'varchar',
       
'length' => 64,
       
'not null' => TRUE,
       
'default' => '',
       
'description' => 'Unique role name.',
       
'translatable' => TRUE,
      ),
?>

[The fact that it is marked as 'translatable' is a bug, unique things cannot be translated]

As a consequence it is a bug for any module to rely on {role}.rid instead of {role}.name on its exportable data structure.

True, role names can change, but any module that rely on it for import/export can easily prevent its modification with a simple hook_form_alter().

Title:Change rid to stringChange role id to machine name

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.

Assigned:sun» klausi
StatusFileSize
new32.33 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Here is a first raw, untested, incomplete patch that shows some directions how I think it should be.

Status:Needs work» Needs review
StatusFileSize
new53.88 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

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.

StatusFileSize
new74.56 KB
FAILED: [[SimpleTest]]: [MySQL] 33,549 pass(es), 380 fail(s), and 355 exception(es).
[ View ]

Another iteration of this patch.

  • Cleaned up user.module tests.
  • Fixed the role UI: roles are now added on the same form that is used for editing/deleting.
  • Added a MENU_LOCAL_ACTION for adding roles.

TODO:

  • Fix other simpletests.
  • Fix other core modules.
  • Upgrade path.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new92.85 KB
FAILED: [[SimpleTest]]: [MySQL] 34,064 pass(es), 64 fail(s), and 148 exception(es).
[ View ]

Replaced all occurrences of DRUPAL_ANONYMOUS_RID and DRUPAL_AUTHENTICATED_RID. Replaced all occurences of "rid" variables. Let's see what is left to do in the Simpletests.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new102.1 KB
FAILED: [[SimpleTest]]: [MySQL] 34,121 pass(es), 4 fail(s), and 2 exception(es).
[ View ]

Fixed all tests but the upgrade tests. This will be next.

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.

Good question, I'm not sure. A constant has the advantage of IDE autocompletion, so you do not make typing mistakes as easy as with a raw string.

Also, you could redefine the constant to declare a completely different role to be the anonymous user role, but I doubt that this is a valid use case.

We could change it to be shorter, i.e. ANONYMOUS_ROLE and AUTHENTICATED_ROLE (do we need the Drupal prefix for core constants?).

Anyway, this sounds a bit like a follow-up issue. The patch will be big enough, so I suggest to focus first on porting the crap and then fixing it up in followups.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new105.23 KB
FAILED: [[SimpleTest]]: [MySQL] 34,139 pass(es), 4 fail(s), and 2 exception(es).
[ View ]

Ah excellent, simpletests are all passing except for the upgrade tests. Dumping my most recent work here which contains a largely untested update function for user.install.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new108.67 KB
PASSED: [[SimpleTest]]: [MySQL] 33,354 pass(es).
[ View ]

New patch, now the bare upgrade test case should pass.

Adds a preparation function to the upgrade process because the new role name schema is needed for session bootstrapping.

StatusFileSize
new108.14 KB
PASSED: [[SimpleTest]]: [MySQL] 33,347 pass(es).
[ View ]

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.

+++ b/core/includes/bootstrap.inc
@@ -2110,8 +2110,7 @@ function drupal_anonymous_user() {
-  $user->roles[DRUPAL_ANONYMOUS_RID] = 'anonymous user';
+  $user->roles = array(DRUPAL_ANONYMOUS_ROLE => DRUPAL_ANONYMOUS_ROLE);

amateescu said on IRC that this does not look right and that we are loosing the human readable name there. The reasoning behind it:
  • We should not store role data in the user object, only references to roles. If i want to print information about the role a user has, then I need to load the role first.
  • Storing the roles as array values only would mean that I would have to use in_array('administrator', $user->roles) instead of isset($user->roles['administrator']) to check if the user has a role, which is a bit harder to read, IMO.
  • The Drupal ecosystem currently relies on the array keys of the roles array, so we should not break that (at least not in this patch).
  • The unique role name was available as array value in previous Drupal versions, so I also think we should not break that.

Therefore I would argue that we keep an array key value mirror for the roles array on the user object.

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.

I agree with #15. And that should be a follow-up issue. The point of the constant is that $user->roles[0] is less clear than $user->roles[DRUPAL_ANONYMOUS_RID]. But the latter and $user->roles['anonymous_user'] is equally clear, so there really is no point.

Updated the issue summary.

Status:Needs review» Needs work

I agree with #15. And that should be a follow-up issue. The point of the constant is that $user->roles[0] is less clear than $user->roles[DRUPAL_ANONYMOUS_RID]. But the latter and $user->roles['anonymous_user'] is equally clear, so there really is no point.

I agree too. $roles['anonymous_user'] is the best readable variant.

+++ b/core/includes/bootstrap.inc
@@ -2110,8 +2110,7 @@ function drupal_anonymous_user() {
-  $user->roles[DRUPAL_ANONYMOUS_RID] = 'anonymous user';
+  $user->roles = array(DRUPAL_ANONYMOUS_ROLE => DRUPAL_ANONYMOUS_ROLE);

I don't think the array values are constantly set to labels currently either. E.g. I'm pretty sure that in hook_user_presave() no array values are available. That said, I'd agree that we want to keep the array keyed as it doesn't unnecessarily invalidate current checks and allows for faster checks compared to using in_array()..

For setting a role I'd see the code to just be:

<?php
$user
->roles['admin'] = TRUE;
$user->save();
?>

Thus, array values should be just TRUE.

+++ b/core/modules/block/block.admin.inc
@@ -399,7 +399,7 @@ function block_admin_configure($form, &$form_state, $module, $delta) {
-  $default_role_options = db_query("SELECT rid FROM {block_role} WHERE module = :module AND delta = :delta", array(
+  $default_role_options = db_query("SELECT role FROM {block_role} WHERE module = :module AND delta = :delta", array(

Well, all the other db_tables are converted to use 'role_name' - we should be consistent here.

According to pattern outlined in #1233394: [Policy, no patch] Agree on a property naming pattern the column should be role_name, but the used property too. However, in the case of roles we usually don't need to deal with role-objects anyway. Thus I think it's fine to keep the current $object->roles or $object['roles'], while using the same array structure as for $user->roles.

+++ b/core/modules/user/user.module
@@ -3217,8 +3197,8 @@ function user_multiple_role_edit($accounts, $operation, $rid) {
+        if ($account !== FALSE && isset($account->roles[$role_name])) {
+          $roles = array_diff($account->roles, array($role_name => $role_name));

This should look at the keys not values, i.e. use array_diff_key()

Status:Needs work» Needs review
StatusFileSize
new13.73 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 935062-role-names-interdiff-d8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new108.29 KB
FAILED: [[SimpleTest]]: [MySQL] 33,236 pass(es), 40 fail(s), and 18 exception(s).
[ View ]

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.

Issue summary:View changes

issue summary template

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new3.29 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 935062-role-names-interdiff-d8_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new108.64 KB
PASSED: [[SimpleTest]]: [MySQL] 34,271 pass(es).
[ View ]

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

interdiff is against #27.

Issue tags:+Configuration system

tagging

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new108.64 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 935062-role-names_8_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Reuploading patch from #29 to not confuse the testbot.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new108.67 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/user/user.install.
[ View ]

Rerolled.

Status:Needs review» Needs work

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

StatusFileSize
new108.67 KB
FAILED: [[SimpleTest]]: [MySQL] 34,618 pass(es), 1,337 fail(s), and 2,374 exception(s).
[ View ]

Applied the patch to a freshly pulled d8, incremented the update hook number to 8002.

for file in $(git ls-files -m); do php -l $file; done

passes for me.

Status:Needs work» Needs review

...and flipping status.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new4.07 KB
new112.39 KB
PASSED: [[SimpleTest]]: [MySQL] 35,667 pass(es).
[ View ]

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

StatusFileSize
new112.37 KB
PASSED: [[SimpleTest]]: [MySQL] 35,683 pass(es).
[ View ]

Status:Needs review» Needs work

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

+++ b/core/includes/session.inc
@@ -110,8 +110,8 @@ function _drupal_session_read($sid) {
+    $user->roles += db_query("SELECT r.name, r.name FROM {role} r INNER JOIN {users_roles} ur ON ur.role_name = r.name WHERE ur.uid = :uid", array(':uid' => $user->uid))->fetchAllKeyed(0, 1);

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

+++ b/core/includes/update.inc
@@ -200,6 +203,93 @@ function update_prepare_d8_language() {
+          $new_name = $new_name . '_';

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.

+++ b/core/modules/block/block.install
@@ -199,6 +199,41 @@ function block_update_8000() {
+  // Update the pimary key of the block_role table.

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

+++ b/core/modules/comment/comment.module
@@ -1354,16 +1354,16 @@ function comment_node_update_index($node) {
+      if (!isset($perms['access comments'][$role_name]) && ($role_name == DRUPAL_ANONYMOUS_ROLE || !isset($perms['access comments'][DRUPAL_AUTHENTICATED_ROLE]))) {

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.

+++ b/core/modules/field/modules/text/text.test
@@ -212,8 +212,10 @@ class TextFieldTestCase extends DrupalWebTestCase {
-    $rid = max(array_keys($this->web_user->roles));

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.

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

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.

+++ b/core/modules/user/user.admin.inc
@@ -821,22 +820,22 @@ function theme_user_permission_description($variables) {
-  $roles = user_roles();

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?

Status:Needs work» Needs review
StatusFileSize
new5.51 KB
new112.34 KB
FAILED: [[SimpleTest]]: [MySQL] 35,681 pass(es), 7 fail(s), and 4 exception(s).
[ View ]
  • Fixed db_query() in session.inc
  • We can't do much about long role names in the upgrade path that are 64 characters and happen to match exactly another role with the same name. This is an extreme edge case and I suggest to just ignore it. I'm open to suggestions how we could solve that, though.
  • Fixed "pimary" spelling everywhere.
  • Fixed access check for search indexing in comment.module, it now matches exactly the old logic.
  • Fixed role name retrieval in tests.
  • I do not want to lowercase passed role names in simpletest, this is the responsibility of the caller. We could do it in user_role_save(), but that also feels wrong.
  • Added translatable query tag in user_admin_roles().
  • Where should we return 'machine_name' => 'Human-readable name'?

Interdiff is against #40.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new1.88 KB
new112.44 KB
PASSED: [[SimpleTest]]: [MySQL] 35,717 pass(es).
[ View ]

Reverted the role name retrieval in the tests, as the newly added role name is not necessarily at the end of the roles array anymore.

Interdiff is against #42.

Status:Needs review» Needs work

+++ b/core/includes/bootstrap.inc
@@ -146,14 +146,14 @@ const DRUPAL_BOOTSTRAP_LANGUAGE = 6;
+const DRUPAL_ANONYMOUS_ROLE = 'anonymous_user';

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

I agree (#24), and so does @fago (#25).

Status:Needs work» Needs review

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

Oh, I guess I missed that. I also think it's weird, since we're already changing the constants here, but I don't really mind.
In order to not forget again, I opened #1500140: Remove DRUPAL_ANONYMOUS_ROLE and DRUPAL_AUTHENTICATED_ROLE constants.

Status:Needs review» Needs work
Issue tags:+Needs tests

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.)

StatusFileSize
new112.37 KB
PASSED: [[SimpleTest]]: [MySQL] 35,906 pass(es).
[ View ]

Fixed: #1, #2, #3, #5, #6, #7 and #8.

Remaining task: #4

Status:Needs work» Needs review

Only for testbot.

Issue tags:-Needs tests
StatusFileSize
new6.87 KB
new117.42 KB
PASSED: [[SimpleTest]]: [MySQL] 35,950 pass(es).
[ View ]

Now with upgrade path tests :-) The following things are checked:

  • Upgrading a role name with an umlaut in it.
  • Upgrading a very long role name (64 characters).
  • Upgrading another very long role name with an almost identical name as the previous one.
  • Upgrading permissions for a specific role.
  • Upgrading block visibility settings per role (this revealed an actual upgrade path bug, yay!).

It was a bit tedious to get the upgrade test database working locally as http://drupal.org/node/1429136 lacks information about that. I will followup with a writeup about that later.

Interdiff is against #50.

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

Issue tags:+API change

Tagging.

StatusFileSize
new117.39 KB
PASSED: [[SimpleTest]]: [MySQL] 35,094 pass(es).
[ View ]

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

Priority:Normal» Major
StatusFileSize
new117.25 KB
PASSED: [[SimpleTest]]: [MySQL] 36,629 pass(es).
[ View ]

Rerolled to account for the user entity namespace changes.

Also changing the priority to major, since this really has to be fixed in D8.

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.

StatusFileSize
new1.32 KB
new116.96 KB
PASSED: [[SimpleTest]]: [MySQL] 36,637 pass(es).
[ View ]

Thanks Bojan!

Fixed user_admin_roles() to use user_roles() as it originally was. I don't think we have to put the special code from update_prepare_d8_role_names() into a dedicated function. It is targeted at a very special use case: to convert unique pseudo machine names to real machine names. I don't see where we would have to re-use that code.

Interdiff is against #56.

Status:Needs review» Needs work
Issue tags:-API change, -Configuration system

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

Status:Needs work» Needs review
Issue tags:+API change, +Configuration system

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

Status:Needs review» Reviewed & tested by the community

Let's do this!

Status:Reviewed & tested by the community» Needs review

Thanks for your hard work on this.

I have one major issue with this patch though. To explain it, I need to provide history:

For D7, we introduced machine names for two configuration objects, taxonomy vocabularies and text formats. And we chose two entirely different ways to convert the two of them:

For vocabularies, we introduced a new 'machine_name' column and auto-generated the machine name for each existing vocabulary, pretty much identical to as being proposed for roles in this patch. This caused major headaches down the line, because the new machine names were not really predictable (and it caused even more confusion, because we retained the old vocabulary IDs in parallel). Not only all runtime code had to be massively updated, but also the upgrade path for all modules depending on or dealing with vocabularies in any way turned out to be very cumbersome.

For text formats, however, we chose a much simpler route: We simply converted the format ID into a string (whereas the format (ID) is equivalent to the rid here). That way, the required changes to runtime code and the upgrade path for all modules were kept minimal.

There's absolutely nothing wrong with retaining the name "rid" as the key/property that holds the machine name identifier. Existing custom roles that are upgraded simply get a numeric machine name. But of course, newly created custom roles on upgraded as well as new sites get a "regular" alphanumeric machine name assigned.

I'd really prefer to go the simple route. We did the same for image styles in the configuration system conversion for D8.

Speaking of, this issue is tagged with Configuration system, but the patch does not seem to convert the basic user role configuration to the configuration system? (the role/permissions association should be a separate issue in any case)

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.

@sun: Maybe you are right. I thought about this, but it seemed ridiculous to replace the existing unique role name with the numeric ID. On the other hand we don't lose the name, it is just moved to the label column. And as you said, it makes the upgrade path easier as we can just drop the rid column altogether.

Still, it is ugly that we keep such broken machine names around. Anyway, I will rework this based on your suggestion and we will see what we like best.

StatusFileSize
new9.64 KB
new116.45 KB
PASSED: [[SimpleTest]]: [MySQL] 36,648 pass(es).
[ View ]

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.

Hm. Actually, I also meant to retain the name "rid" everywhere, denoting the role identifier (string).

That's what we've done with text formats, and also what we're going to do with vocabulary IDs ("vid") in #1552396: Convert vocabularies into configuration (dropping the separate/parallel 'machine_name' property instead).

Status:Needs review» Needs work

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.

Status:Needs work» Needs review
StatusFileSize
new38.27 KB
FAILED: [[SimpleTest]]: [MySQL] 36,686 pass(es), 27 fail(s), and 8 exception(s).
[ View ]

Work in progress patch attached, this will not pass all tests.

Status:Needs review» Needs work

+++ b/core/includes/bootstrap.inc
@@ -149,12 +149,12 @@ const DRUPAL_BOOTSTRAP_FULL = 7;
+const DRUPAL_ANONYMOUS_RID = 'anonymous user';
...
+const DRUPAL_AUTHENTICATED_RID = 'authenticated user';

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.

Status:Needs work» Needs review
StatusFileSize
new4.11 KB
new38.72 KB
PASSED: [[SimpleTest]]: [MySQL] 36,714 pass(es).
[ View ]

Ah, the space caused the test fails, copy & paste error.

Shortened the default role IDs as suggested by sun and extended the upgrade path test a bit.

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?

Short discussion with xjm on IRC: the path collision is followup stuff, so we leave that as is for now. See also #1262812: Make "Roles" a tab of "admin/people" instead of "admin/people/permissions".

Issue summary:View changes

updated new role system to TRUE flags.

Issue summary:View changes

updated user interface changes.

StatusFileSize
new5.01 KB
new7.95 KB
new16 KB
new38.96 KB
FAILED: [[SimpleTest]]: [MySQL] 36,361 pass(es), 358 fail(s), and 0 exception(s).
[ View ]

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:

role-list-add.png

  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.

role-edit.png

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

All of those constant replacements in tests should be backported, so I split that off into #1600892: Tests use magic numbers 1 and 2 instead of user role constants

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new38.95 KB
PASSED: [[SimpleTest]]: [MySQL] 36,709 pass(es).
[ View ]

Sorry, fixed that.

Status:Needs review» Needs work

+++ b/core/modules/user/user.install
@@ -81,7 +81,7 @@ function user_schema() {
     'foreign keys' => array(
       'role' => array(
-        'table' => 'roles',
+        'table' => 'role',

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.

Status:Needs work» Needs review

Didn't mean to change the status, I blame dreditor.

StatusFileSize
new31.42 KB
PASSED: [[SimpleTest]]: [MySQL] 36,708 pass(es).
[ View ]

StatusFileSize
new12.08 KB
new37.99 KB
PASSED: [[SimpleTest]]: [MySQL] 36,706 pass(es).
[ View ]

- Adjusted user_roles() to account for the custom role labels (built-in roles are no longer translated).

- Adjusted user_admin_roles() to properly embed the role configuration form into the listing/table without duplicating it.

StatusFileSize
new6.68 KB
new40.09 KB
PASSED: [[SimpleTest]]: [MySQL] 36,714 pass(es).
[ View ]
  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...

StatusFileSize
new40.88 KB
PASSED: [[SimpleTest]]: [MySQL] 36,708 pass(es).
[ View ]

- Minor addition to the role administration test to verify that the delete router paths for built-in roles cannot be accessed.

- Added user_admin_role_validate() form validation callback to prevent rids that currently clash with other User module router paths and UI behavior, using the same logic as we do for node types already. As mentioned in #71 and #72, we'll likely change the router paths entirely in a separate follow-up issue, but I wanted this patch to be complete.

StatusFileSize
new1.3 KB
new40.16 KB
PASSED: [[SimpleTest]]: [MySQL] 36,723 pass(es).
[ View ]

Fixed role name capitalization for built-in roles.

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

Status:Needs review» Needs work
Issue tags:-API change, -Configuration system

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

Status:Needs work» Needs review
Issue tags:+API change, +Configuration system

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

Also RTBC from my perspective. But I've added too much to this patch, so not eligible to mark as RTBC.

Status:Needs review» Reviewed & tested by the community

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

This seems to be missing coverage for the user_admin_role variable and administrative role functionality?

Status:Reviewed & tested by the community» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new963 bytes
new40.87 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 935062-role-names-91.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled the patch against the recent PSR-0 conversions of simpletests. Added a check for the administrative user role in the role upgrade path test. I think that should cover it?

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

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

Cool, it even survived the kernel patch :-)

Status:Needs review» Reviewed & tested by the community

Excellent. So back to RTBC.

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

Status:Reviewed & tested by the community» Needs work
Issue tags:+API change, +Configuration system

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

Status:Needs work» Needs review
StatusFileSize
new40.87 KB
PASSED: [[SimpleTest]]: [MySQL] 36,847 pass(es).
[ View ]

Rerolled.

git fetch origin
git fetch platform
git checkout platform-roles-935062-klausi
git merge origin/8.x
git diff origin/8.x > 935062-role-names-98.patch

Status:Needs review» Reviewed & tested by the community

Back to RTBC

Title:Change role id to machine nameChange notification for: Change role id to machine name
Priority:Major» Critical
Status:Reviewed & tested by the community» Active

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

Issue summary:View changes

We keep rid, we just change it to be a string

Issue summary:View changes

Corrected the new layout of $user->roles, we still have the labels

Issue summary:View changes

role array values are not TRUE right now.

Status:Active» Needs review

Basic change notification created: http://drupal.org/node/1619504

Issue summary:View changes

Removed task

Title:Change notification for: Change role id to machine nameChange role id to machine name
Status:Needs review» Fixed

Thanks, I've revised and rewritten that :)

Priority:Critical» Major

Status:Fixed» Closed (fixed)

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

Would it be possible to backport this to D7 ?
How is the process in this case ? Do we have to open another issue ?

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.

Thank you for the advice. I didn't know this module (but it will now be a full part of my drush make file !)

+++ b/core/modules/system/tests/upgrade/drupal-7.roles.database.phpundefined
@@ -0,0 +1,63 @@
+// Adds a role with an umlaut in it.
+->values(array(
+  'rid' => '4',
+  'name' => 'gärtner',
+  'weight' => '3',

this might be not displaying correctly in some places.

It seems to be the case that using umlauts was actually intended to test some of the upgrade functionality, sorry.

Issue summary:View changes

added change record link