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:

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

New system:

$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

CommentFileSizeAuthor
#98 935062-role-names-98.patch40.87 KBklausi
#91 935062-role-names-91.patch40.87 KBklausi
#91 935062-role-names-91-interdiff.txt963 bytesklausi
#83 935062-role-names-83.patch40.16 KBklausi
#83 935062-role-names-83-interdiff.txt1.3 KBklausi
#82 platform.roles_.82.patch40.88 KBsun
#81 935062-role-names-81.patch40.09 KBklausi
#81 935062-role-names-81-interdiff.txt6.68 KBklausi
#80 platform.roles_.80.patch37.99 KBsun
#80 interdiff.txt12.08 KBsun
#79 platform.roles_.79.patch31.42 KBsun
#76 platform.roles_.75.patch38.95 KBsun
#73 platform.roles_.73.patch38.96 KBsun
#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
#70 935062-role-names-70-interdiff.txt4.11 KBklausi
#68 935062-role-names-68.patch38.27 KBklausi
#65 935062-role-names-65.patch116.45 KBklausi
#65 935062-role-names-65-interdiff.txt9.64 KBklausi
#58 935062-role-names-58.patch116.96 KBklausi
#58 935062-role-names-58-interdiff.txt1.32 KBklausi
#56 935062-role-names-56.patch117.25 KBklausi
#55 935062-role-names-55.patch117.39 KBklausi
#52 935062-role-names-52.patch117.42 KBklausi
#52 935062-interdiff-52-do-not-test.patch6.87 KBklausi
#50 role-name-935062-50.patch112.37 KBoriol_e9g
#44 935062-role-names.patch112.44 KBklausi
#44 935062-interdiff-do-not-test.patch1.88 KBklausi
#42 935062-role-names.patch112.34 KBklausi
#42 935062-interdiff-do-not-test.patch5.51 KBklausi
#40 935062-40.role-names.patch112.37 KBksenzee
#39 935062-role-names.patch112.39 KBklausi
#39 935062-interdiff-do-not-test.patch4.07 KBklausi
#36 935062-role-names_36.patch108.67 KBwamilton
#34 935062-role-names_8.patch108.67 KBklausi
#32 935062-role-names_8.patch108.64 KBklausi
#29 935062-role-names.patch108.64 KBklausi
#29 935062-role-names-interdiff-d8.patch3.29 KBklausi
#27 935062-role-names.patch108.29 KBklausi
#27 935062-role-names-interdiff-d8.patch13.73 KBklausi
#21 935062-role-names.patch108.14 KBklausi
#20 935062-role-names.patch108.67 KBklausi
#18 935062-role-names.patch105.23 KBklausi
#14 935062-role-names.patch102.1 KBklausi
#12 935062-role-names.patch92.85 KBklausi
#10 935062-role-names.patch74.56 KBklausi
#9 935062-role-names.patch53.88 KBklausi
#8 935062-role-names.patch32.33 KBklausi
#4 drupal.rid_.4.patch11.42 KBsun
#2 drupal.rid_.2.patch10.74 KBsun
drupal.rid_.0.patch9.3 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

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

sun’s picture

Status: Needs work » Needs review
FileSize
10.74 KB

Status: Needs review » Needs work

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

sun’s picture

Status: Needs work » Needs review
FileSize
11.42 KB

Status: Needs review » Needs work

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

Damien Tournoud’s picture

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:

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

klausi’s picture

Title: Change rid to string » Change 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.

klausi’s picture

Assigned: sun » klausi
FileSize
32.33 KB

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

klausi’s picture

Status: Needs work » Needs review
FileSize
53.88 KB

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.
klausi’s picture

FileSize
74.56 KB

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.

klausi’s picture

Status: Needs work » Needs review
FileSize
92.85 KB

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.

klausi’s picture

Status: Needs work » Needs review
FileSize
102.1 KB

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

webchick’s picture

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.

klausi’s picture

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.

klausi’s picture

Status: Needs work » Needs review
FileSize
105.23 KB

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.

klausi’s picture

Status: Needs work » Needs review
FileSize
108.67 KB

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.

klausi’s picture

FileSize
108.14 KB

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.

klausi’s picture

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

amateescu’s picture

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.

tstoeckler’s picture

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.

klausi’s picture

Updated the issue summary.

fago’s picture

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:

$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()

klausi’s picture

Status: Needs work » Needs review
FileSize
13.73 KB
108.29 KB

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.

klausi’s picture

Issue summary: View changes

issue summary template

Status: Needs review » Needs work

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

klausi’s picture

Status: Needs work » Needs review
FileSize
3.29 KB
108.64 KB

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

interdiff is against #27.

gdd’s picture

Issue tags: +Configuration system

tagging

Status: Needs review » Needs work

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

klausi’s picture

Status: Needs work » Needs review
FileSize
108.64 KB

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.

klausi’s picture

Status: Needs work » Needs review
FileSize
108.67 KB

Rerolled.

Status: Needs review » Needs work

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

wamilton’s picture

FileSize
108.67 KB

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.

wamilton’s picture

Status: Needs work » Needs review

...and flipping status.

Status: Needs review » Needs work

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

klausi’s picture

Status: Needs work » Needs review
FileSize
4.07 KB
112.39 KB

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

ksenzee’s picture

FileSize
112.37 KB
ksenzee’s picture

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?

klausi’s picture

Status: Needs work » Needs review
FileSize
5.51 KB
112.34 KB
  • 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.

klausi’s picture

Status: Needs work » Needs review
FileSize
1.88 KB
112.44 KB

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.

joachim’s picture

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?

tstoeckler’s picture

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

klausi’s picture

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.

tstoeckler’s picture

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.

xjm’s picture

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

oriol_e9g’s picture

FileSize
112.37 KB

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

Remaining task: #4

oriol_e9g’s picture

Status: Needs work » Needs review

Only for testbot.

klausi’s picture

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.

klausi’s picture

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

klausi’s picture

Issue tags: +API change

Tagging.

klausi’s picture

FileSize
117.39 KB

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

klausi’s picture

Priority: Normal » Major
FileSize
117.25 KB

Rerolled to account for the user entity namespace changes.

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

bojanz’s picture

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.

klausi’s picture

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.

klausi’s picture

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

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

bojanz’s picture

Status: Needs review » Reviewed & tested by the community

Let's do this!

sun’s picture

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)

ksenzee’s picture

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.

klausi’s picture

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

klausi’s picture

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.

sun’s picture

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

klausi’s picture

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.

klausi’s picture

Status: Needs work » Needs review
FileSize
38.27 KB

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

sun’s picture

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.

klausi’s picture

Status: Needs work » Needs review
FileSize
4.11 KB
38.72 KB

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.

klausi’s picture

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?

klausi’s picture

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

klausi’s picture

Issue summary: View changes

updated new role system to TRUE flags.

klausi’s picture

Issue summary: View changes

updated user interface changes.

sun’s picture

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)
sun’s picture

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.

sun’s picture

Status: Needs work » Needs review
FileSize
38.95 KB

Sorry, fixed that.

klausi’s picture

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.

klausi’s picture

Status: Needs work » Needs review

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

sun’s picture

sun’s picture

FileSize
12.08 KB
37.99 KB

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

klausi’s picture

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

sun’s picture

FileSize
40.88 KB

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

klausi’s picture

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.

klausi’s picture

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

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

sun’s picture

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

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

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

Dave Reid’s picture

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

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

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

klausi’s picture

Status: Needs work » Needs review
FileSize
963 bytes
40.87 KB

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?

sun’s picture

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

sun’s picture

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

klausi’s picture

Cool, it even survived the kernel patch :-)

sun’s picture

Status: Needs review » Reviewed & tested by the community

Excellent. So back to RTBC.

catch’s picture

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

klausi’s picture

Status: Needs work » Needs review
FileSize
40.87 KB

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
tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

catch’s picture

Title: Change role id to machine name » Change 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.

catch’s picture

Issue summary: View changes

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

klausi’s picture

Issue summary: View changes

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

klausi’s picture

Issue summary: View changes

role array values are not TRUE right now.

klausi’s picture

Status: Active » Needs review

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

klausi’s picture

Issue summary: View changes

Removed task

sun’s picture

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

Thanks, I've revised and rewritten that :)

Tor Arne Thune’s picture

Priority: Critical » Major

Status: Fixed » Closed (fixed)

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

DuaelFr’s picture

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

klausi’s picture

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.

DuaelFr’s picture

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

YesCT’s picture

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

dawehner’s picture

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

dawehner’s picture

Issue summary: View changes

added change record link