Issue #935062 by klausi, sun, wamilton, ksenzee, oriol_e9g: Change role id to machine name.

core/includes/bootstrap.inc

@@ -150,12 +150,12 @@
 /**
  * Role ID for anonymous users; should match what's in the "role" table.
  */
-const DRUPAL_ANONYMOUS_RID = 1;
+const DRUPAL_ANONYMOUS_RID = 'anonymous';
 
 /**
  * Role ID for authenticated users; should match what's in the "role" table.
  */
-const DRUPAL_AUTHENTICATED_RID = 2;
+const DRUPAL_AUTHENTICATED_RID = 'authenticated';
 
 /**
  * The number of bytes in a kilobyte.

core/modules/block/block.install

@@ -118,8 +118,8 @@ function block_schema() {
         'description' => "The block's unique delta within module, from {block}.delta.",
       ),
       'rid' => array(
-        'type' => 'int',
-        'unsigned' => TRUE,
+        'type' => 'varchar',
+        'length' => 64,
         'not null' => TRUE,
         'description' => "The user's role ID from {users_roles}.rid.",
       ),
@@ -128,6 +128,12 @@ function block_schema() {
     'indexes' => array(
       'rid' => array('rid'),
     ),
+    'foreign keys' => array(
+      'role' => array(
+        'table' => 'role',
+        'columns' => array('rid' => 'rid'),
+      ),
+    ),
   );
 
   $schema['block_custom'] = array(
@@ -222,6 +228,17 @@ function block_install() {
  * @{
  */
 
+/**
+ * Implements hook_update_dependencies().
+ */
+function block_update_dependencies() {
+  // Convert role IDs after User module converted {role}.
+  $dependencies['block'][8002] = array(
+    'user' => 8002,
+  );
+  return $dependencies;
+}
+
 /**
  * Block cache is always enabled in 8.x.
  *
@@ -268,6 +285,32 @@ function block_update_8001() {
   db_create_table('block_language', $schema);
 }
 
+/**
+ * Replace serial role IDs with machine name strings.
+ *
+ * @see user_update_8002()
+ */
+function block_update_8002() {
+  // Change serial rid column into string.
+  $column = array(
+    'type' => 'varchar',
+    'length' => 64,
+    'not null' => TRUE,
+    'description' => "The user's role ID from {users_roles}.rid.",
+  );
+  db_change_field('block_role', 'rid', 'rid', $column);
+
+  // Rename the built-in serial role IDs into the hardcoded machine names.
+  db_update('block_role')
+    ->fields(array('rid' => DRUPAL_ANONYMOUS_RID))
+    ->condition('rid', 1)
+    ->execute();
+  db_update('block_role')
+    ->fields(array('rid' => DRUPAL_AUTHENTICATED_RID))
+    ->condition('rid', 2)
+    ->execute();
+}
+
 /**
  * @} End of "addtogroup updates-7.x-to-8.x".
  * The next series of updates should start at 9000.

core/modules/field/modules/text/text.test

@@ -215,7 +215,9 @@ class TextFieldTestCase extends WebTestBase {
     $format = filter_format_load($edit['format']);
     $format_id = $format->format;
     $permission = filter_permission_name($format);
-    $rid = max(array_keys($this->web_user->roles));
+    $roles = $this->web_user->roles;
+    unset($roles[DRUPAL_AUTHENTICATED_RID]);
+    $rid = key($roles);
     user_role_grant_permissions($rid, array($permission));
     $this->drupalLogin($this->web_user);
 

core/modules/filter/lib/Drupal/filter/Tests/FilterFormatAccessTest.php

@@ -106,8 +106,10 @@ function testFormatPermissions() {
   }
 
   function testFormatRoles() {
-    // Get the role ID assigned to the regular user; it must be the maximum.
-    $rid = max(array_keys($this->web_user->roles));
+    // Get the role ID assigned to the regular user.
+    $roles = $this->web_user->roles;
+    unset($roles[DRUPAL_AUTHENTICATED_RID]);
+    $rid = key($roles);
 
     // Check that this role appears in the list of roles that have access to an
     // allowed text format, but does not appear in the list of roles that have

core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php

@@ -400,17 +400,24 @@ protected function drupalCreateUser(array $permissions = array()) {
   /**
    * Internal helper function; Create a role with specified permissions.
    *
-   * @param $permissions
+   * @param array $permissions
    *   Array of permission names to assign to role.
-   * @param $name
-   *   (optional) String for the name of the role.  Defaults to a random string.
-   * @return
+   * @param string $rid
+   *   (optional) The role ID (machine name). Defaults to a random name.
+   * @param string $name
+   *   (optional) The label for the role. Defaults to a random string.
+   *
+   * @return string
    *   Role ID of newly created role, or FALSE if role creation failed.
    */
-  protected function drupalCreateRole(array $permissions, $name = NULL) {
-    // Generate random name if it was not passed.
-    if (!$name) {
-      $name = $this->randomName();
+  protected function drupalCreateRole(array $permissions, $rid = NULL, $name = NULL) {
+    // Generate a random, lowercase machine name if none was passed.
+    if (!isset($rid)) {
+      $rid = strtolower($this->randomName(8));
+    }
+    // Generate a random label.
+    if (!isset($name)) {
+      $name = $this->randomString(8);
     }
 
     // Check the all the permissions strings are valid.
@@ -420,14 +427,29 @@ protected function drupalCreateRole(array $permissions, $name = NULL) {
 
     // Create new role.
     $role = new stdClass();
+    $role->rid = $rid;
     $role->name = $name;
-    user_role_save($role);
+    $result = user_role_save($role);
+
+    $this->assertIdentical($result, SAVED_NEW, t('Created role ID @rid with name @name.', array(
+      '@name' => var_export($role->name, TRUE),
+      '@rid' => var_export($role->rid, TRUE),
+    )), t('Role'));
+
+    if ($result === SAVED_NEW) {
+      // Grant the specified permissions to the role, if any.
+      if (!empty($permissions)) {
         user_role_grant_permissions($role->rid, $permissions);
 
-    $this->assertTrue(isset($role->rid), t('Created role of name: @name, id: @rid', array('@name' => $name, '@rid' => (isset($role->rid) ? $role->rid : t('-n/a-')))), t('Role'));
-    if ($role && !empty($role->rid)) {
-      $count = db_query('SELECT COUNT(*) FROM {role_permission} WHERE rid = :rid', array(':rid' => $role->rid))->fetchField();
-      $this->assertTrue($count == count($permissions), t('Created permissions: @perms', array('@perms' => implode(', ', $permissions))), t('Role'));
+        $assigned_permissions = db_query('SELECT permission FROM {role_permission} WHERE rid = :rid', array(':rid' => $role->rid))->fetchCol();
+        $missing_permissions = array_diff($permissions, $assigned_permissions);
+        if (!$missing_permissions) {
+          $this->pass(t('Created permissions: @perms', array('@perms' => implode(', ', $permissions))), t('Role'));
+        }
+        else {
+          $this->fail(t('Failed to create permissions: @perms', array('@perms' => implode(', ', $missing_permissions))), t('Role'));
+        }
+      }
       return $role->rid;
     }
     else {

core/modules/system/system.info

@@ -36,3 +36,4 @@ files[] = tests/uuid.test
 files[] = tests/xmlrpc.test
 files[] = tests/upgrade/upgrade.test
 files[] = tests/upgrade/upgrade.language.test
+files[] = tests/upgrade/upgrade.roles.test

core/modules/system/tests/upgrade/drupal-7.roles.database.php

+<?php
+
+/**
+ * @file
+ * Database additions for role tests. Used in upgrade.roles.test.
+ *
+ * This dump only contains data and schema components relevant for role
+ * functionality. The drupal-7.bare.database.php file is imported before
+ * this dump, so the two form the database structure expected in tests
+ * altogether.
+ */
+
+db_insert('role')->fields(array(
+  'rid',
+  'name',
+  'weight',
+))
+// Adds a role with an umlaut in it.
+->values(array(
+  'rid' => '4',
+  'name' => 'gärtner',
+  'weight' => '3',
+))
+// Adds a very long role name.
+->values(array(
+  'rid' => '5',
+  'name' => 'very long role name that has exactly sixty-four characters in it',
+  'weight' => '4',
+))
+// Adds a very similar role name to test edge cases.
+->values(array(
+  'rid' => '6',
+  'name' => 'very_long role name that has exactly sixty-four characters in it',
+  'weight' => '5',
+))
+->execute();
+
+// Add the "Edit own comments" permission to the gärtner test role.
+db_insert('role_permission')->fields(array(
+  'rid',
+  'permission',
+  'module',
+))
+->values(array(
+  'rid' => '4',
+  'permission' => 'edit own comments',
+  'module' => 'comment',
+))
+->execute();
+
+// Adds some role visibility settings on the who's online block for the long
+// role.
+db_insert('block_role')->fields(array(
+  'module',
+  'delta',
+  'rid',
+))
+->values(array(
+  'module' => 'user',
+  'delta' => 'online',
+  'rid' => '5',
+))
+->execute();

core/modules/system/tests/upgrade/upgrade.roles.test

+<?php
+
+/**
+ * @file
+ * Upgrade tests for the conversion of serial role IDs to role machine names.
+ */
+
+/**
+ * Tests upgrading a bare database with user role data.
+ *
+ * Loads a bare installation of Drupal 7 with role data and runs the
+ * upgrade process on it.
+ */
+class UserRoleUpgradePathTestCase extends UpgradePathTestCase {
+  public static function getInfo() {
+    return array(
+      'name'  => 'Role upgrade test',
+      'description'  => 'Upgrade tests with role data.',
+      'group' => 'Upgrade path',
+    );
+  }
+
+  public function setUp() {
+    $this->databaseDumpFiles = array(
+      drupal_get_path('module', 'system') . '/tests/upgrade/drupal-7.bare.standard_all.database.php.gz',
+      drupal_get_path('module', 'system') . '/tests/upgrade/drupal-7.roles.database.php',
+    );
+    parent::setUp();
+  }
+
+  /**
+   * Tests expected role ID conversions after a successful upgrade.
+   */
+  public function testRoleUpgrade() {
+    $this->assertTrue($this->performUpgrade(), 'The upgrade was completed successfully.');
+
+    // Check that "gärtner" has been converted to "4" and that the role
+    // edit page for it exists.
+    $this->drupalGet('admin/people/permissions/roles/edit/4');
+    $this->assertResponse(200, 'Role edit page for "gärtner" was found.');
+
+    // Check that the anonymous user role ID has been converted from "1" to
+    // "anonymous".
+    $this->drupalGet('admin/people/permissions/' . DRUPAL_ANONYMOUS_RID);
+    $this->assertResponse(200, 'Permission edit page for "anonymous" was found.');
+
+    // Check that the authenticated user role ID has been converted from "2" to
+    // "authenticated".
+    $this->drupalGet('admin/people/permissions/' . DRUPAL_AUTHENTICATED_RID);
+    $this->assertResponse(200, 'Permission edit page for "authenticated" was found.');
+
+    // Check that the permission for "gärtner" still exists.
+    $this->drupalGet('admin/people/permissions/4');
+    $this->assertFieldChecked('edit-4-edit-own-comments', 'Edit own comments permission for "gärtner" is set correctly.');
+
+    // Check that the role visibility setting for the who's online block still
+    // exists.
+    $this->drupalGet('admin/structure/block/manage/user/online/configure');
+    $this->assertFieldChecked('edit-roles-5', "Who's online block visibility setting is correctly set for the long role name.");
+    // Check that the role name is still displayed as expected.
+    $this->assertText('gärtner', 'Role name is displayed on block visibility settings.');
+    $this->assertText('very long role name that has exactly sixty-four characters in it', 'Role name is displayed on block visibility settings.');
+    $this->assertText('very_long role name that has exactly sixty-four characters in it', 'Role name is displayed on block visibility settings.');
+
+    // The administrative user role must still be assigned to the
+    // "administrator" role (rid 3).
+    $this->drupalGet('admin/config/people/accounts');
+    $this->assertFieldByName('user_admin_role', 3);
+  }
+}

core/modules/user/lib/Drupal/user/Tests/UserAdminTest.php

@@ -55,7 +55,9 @@ function testUserAdmin() {
     $this->assertText($user_c->name, t('Found user C on filtered by perm admin users page'));
 
     // Filter the users by role. Grab the system-generated role name for User C.
-    $edit['role'] = max(array_flip($user_c->roles));
+    $roles = $user_c->roles;
+    unset($roles[DRUPAL_AUTHENTICATED_RID]);
+    $edit['role'] = key($roles);
     $this->drupalPost('admin/people', $edit, t('Refine'));
 
     // Check if the correct users show up when filtered by role.

core/modules/user/lib/Drupal/user/Tests/UserPermissionsTest.php

@@ -26,10 +26,10 @@ function setUp() {
 
     $this->admin_user = $this->drupalCreateUser(array('administer permissions', 'access user profiles', 'administer site configuration', 'administer modules', 'administer users'));
 
-    // Find the new role ID - it must be the maximum.
-    $all_rids = array_keys($this->admin_user->roles);
-    sort($all_rids);
-    $this->rid = array_pop($all_rids);
+    // Find the new role ID.
+    $all_rids = $this->admin_user->roles;
+    unset($all_rids[DRUPAL_AUTHENTICATED_RID]);
+    $this->rid = key($all_rids);
   }
 
   /**

core/modules/user/lib/Drupal/user/Tests/UserRoleAdminTest.php

@@ -37,38 +37,40 @@ function testRoleAdministration() {
     // correspond to an integer, to test that the role administration pages
     // correctly distinguish between role names and IDs.)
     $role_name = '123';
-    $edit = array('name' => $role_name);
+    $edit = array('role[name]' => $role_name, 'role[rid]' => $role_name);
     $this->drupalPost('admin/people/permissions/roles', $edit, t('Add role'));
     $this->assertText(t('The role has been added.'), t('The role has been added.'));
-    $role = user_role_load_by_name($role_name);
+    $role = user_role_load($role_name);
     $this->assertTrue(is_object($role), t('The role was successfully retrieved from the database.'));
 
     // Try adding a duplicate role.
     $this->drupalPost(NULL, $edit, t('Add role'));
-    $this->assertRaw(t('The role name %name already exists. Choose another role name.', array('%name' => $role_name)), t('Duplicate role warning displayed.'));
+    $this->assertRaw(t('The machine-readable name is already in use. It must be unique.'), t('Duplicate role warning displayed.'));
 
     // Test renaming a role.
     $old_name = $role_name;
     $role_name = '456';
-    $edit = array('name' => $role_name);
+    $edit = array('role[name]' => $role_name);
     $this->drupalPost("admin/people/permissions/roles/edit/{$role->rid}", $edit, t('Save role'));
     $this->assertText(t('The role has been renamed.'), t('The role has been renamed.'));
-    $this->assertFalse(user_role_load_by_name($old_name), t('The role can no longer be retrieved from the database using its old name.'));
-    $this->assertTrue(is_object(user_role_load_by_name($role_name)), t('The role can be retrieved from the database using its new name.'));
+    $new_role = user_role_load($old_name);
+    $this->assertEqual($new_role->name, $role_name, 'The role name has been successfully changed.');
 
     // Test deleting a role.
     $this->drupalPost("admin/people/permissions/roles/edit/{$role->rid}", NULL, t('Delete role'));
     $this->drupalPost(NULL, NULL, t('Delete'));
     $this->assertText(t('The role has been deleted.'), t('The role has been deleted'));
     $this->assertNoLinkByHref("admin/people/permissions/roles/edit/{$role->rid}", t('Role edit link removed.'));
-    $this->assertFalse(user_role_load_by_name($role_name), t('A deleted role can no longer be loaded.'));
+    $this->assertFalse(user_role_load($role_name), t('A deleted role can no longer be loaded.'));
 
-    // Make sure that the system-defined roles cannot be edited via the user
+    // Make sure that the system-defined roles can be edited via the user
     // interface.
     $this->drupalGet('admin/people/permissions/roles/edit/' . DRUPAL_ANONYMOUS_RID);
-    $this->assertResponse(403, t('Access denied when trying to edit the built-in anonymous role.'));
+    $this->assertResponse(200, 'Access granted when trying to edit the built-in anonymous role.');
+    $this->assertNoText(t('Delete role'), 'Delete button for the anonymous role is not present.');
     $this->drupalGet('admin/people/permissions/roles/edit/' . DRUPAL_AUTHENTICATED_RID);
-    $this->assertResponse(403, t('Access denied when trying to edit the built-in authenticated role.'));
+    $this->assertResponse(200, 'Access granted when trying to edit the built-in authenticated role.');
+    $this->assertNoText(t('Delete role'), 'Delete button for the authenticated role is not present.');
   }
 
   /**

core/modules/user/user-rtl.css

@@ -4,12 +4,6 @@
   padding-right: 1.5em;
 }
 
-#user-admin-roles .form-item-name {
-  float: right;
-  margin-left: 1em;
-  margin-right: 0;
-}
-
 /**
  * Password strength indicator.
  */

core/modules/user/user.admin.inc

@@ -653,7 +653,7 @@ function user_admin_permissions($form, $form_state, $rid = NULL) {
 
   // Retrieve role names for columns.
   $role_names = user_roles();
-  if (is_numeric($rid)) {
+  if (isset($rid)) {
     $role_names = array($rid => $role_names[$rid]);
   }
   // Fetch permissions for all roles or the one selected role.
@@ -822,47 +822,62 @@ function theme_user_permission_description($variables) {
  * @see theme_user_admin_roles()
  */
 function user_admin_roles($form, $form_state) {
-  $roles = user_roles();
+  $roles = db_select('role', 'r')
+    ->addTag('translatable')
+    ->fields('r')
+    ->orderBy('weight')
+    ->orderBy('name')
+    ->execute();
 
   $form['roles'] = array(
     '#tree' => TRUE,
   );
-  $order = 0;
-  foreach ($roles as $rid => $name) {
-    $form['roles'][$rid]['#role'] = (object) array(
-      'rid' => $rid,
-      'name' => $name,
-      'weight' => $order,
+  $max_weight = 0;
+  foreach ($roles as $role) {
+    $max_weight = max($max_weight, $role->weight);
+    $form['roles'][$role->rid]['#role'] = $role;
+    $form['roles'][$role->rid]['#weight'] = $role->weight;
+    $form['roles'][$role->rid]['name'] = array(
+      '#markup' => check_plain($role->name),
     );
-    $form['roles'][$rid]['#weight'] = $order;
-    $form['roles'][$rid]['weight'] = array(
+    $form['roles'][$role->rid]['weight'] = array(
       '#type' => 'textfield',
-      '#title' => t('Weight for @title', array('@title' => $name)),
+      '#title' => t('Weight for @title', array('@title' => $role->name)),
       '#title_display' => 'invisible',
       '#size' => 4,
-      '#default_value' => $order,
+      '#default_value' => $role->weight,
       '#attributes' => array('class' => array('role-weight')),
     );
-    $order++;
+    $form['roles'][$role->rid]['edit'] = array(
+      '#type' => 'link',
+      '#title' => t('edit role'),
+      '#href' => 'admin/people/permissions/roles/edit/' . $role->rid,
+    );
+    $form['roles'][$role->rid]['permissions'] = array(
+      '#type' => 'link',
+      '#title' => t('edit permissions'),
+      '#href' => 'admin/people/permissions/' . $role->rid,
+    );
   }
 
-  $form['name'] = array(
-    '#type' => 'textfield',
-    '#title' => t('Name'),
-    '#title_display' => 'invisible',
-    '#size' => 32,
-    '#maxlength' => 64,
-  );
-  $form['add'] = array(
-    '#type' => 'submit',
-    '#value' => t('Add role'),
-    '#validate' => array('user_admin_role_validate'),
-    '#submit' => array('user_admin_role_submit'),
+  // Embed the role add form.
+  $add_role = (object) array(
+    'rid' => NULL,
+    'name' => NULL,
+    'weight' => $max_weight + 1,
   );
-  $form['actions'] = array('#type' => 'actions');
+  $add_form = user_admin_role(array(), $form_state, $add_role);
+  $add_form['actions']['submit']['#submit'] = array('user_admin_role_submit');
+  $add_form['role']['actions'] = $add_form['actions'];
+  unset($add_form['actions']);
+  $form += $add_form;
+
+  $form['actions']['#type'] = 'actions';
   $form['actions']['submit'] = array(
     '#type' => 'submit',
     '#value' => t('Save order'),
+    // Do not validate the add form when saving the order.
+    '#limit_validation_errors' => array(array('roles')),
     '#submit' => array('user_admin_roles_order_submit'),
   );
 
@@ -895,23 +910,27 @@ function theme_user_admin_roles($variables) {
 
   $header = array(t('Name'), t('Weight'), array('data' => t('Operations'), 'colspan' => 2));
   foreach (element_children($form['roles']) as $rid) {
-    $name = $form['roles'][$rid]['#role']->name;
     $row = array();
-    if (in_array($rid, array(DRUPAL_ANONYMOUS_RID, DRUPAL_AUTHENTICATED_RID))) {
-      $row[] = t('@name <em>(locked)</em>', array('@name' => $name));
-      $row[] = drupal_render($form['roles'][$rid]['weight']);
-      $row[] = '';
-      $row[] = l(t('edit permissions'), 'admin/people/permissions/' . $rid);
-    }
-    else {
-      $row[] = check_plain($name);
-      $row[] = drupal_render($form['roles'][$rid]['weight']);
-      $row[] = l(t('edit role'), 'admin/people/permissions/roles/edit/' . $rid);
-      $row[] = l(t('edit permissions'), 'admin/people/permissions/' . $rid);
+    foreach (element_children($form['roles'][$rid]) as $column) {
+      $row[] = drupal_render($form['roles'][$rid][$column]);
     }
     $rows[] = array('data' => $row, 'class' => array('draggable'));
   }
-  $rows[] = array(array('data' => drupal_render($form['name']) . drupal_render($form['add']), 'colspan' => 4, 'class' => 'edit-name'));
+
+  // Distribute the role add form into table columns.
+  $form['role']['name']['#title_display'] = 'invisible';
+  unset($form['role']['name']['#description']);
+  unset($form['role']['rid']['#description']);
+
+  $actions = $form['role']['actions'];
+  unset($form['role']['actions']);
+  unset($form['role']['weight']);
+  $row = array();
+  $row[] = drupal_render($form['role']);
+  // Empty placeholder for the weight column.
+  $row[] = '';
+  $row[] = array('data' => drupal_render($actions), 'colspan' => 2);
+  $rows[] = array('data' => $row);
 
   drupal_add_tabledrag('user-roles', 'order', 'sibling', 'role-weight');
 
@@ -925,90 +944,74 @@ function theme_user_admin_roles($variables) {
  * Form to configure a single role.
  *
  * @ingroup forms
- * @see user_admin_role_validate()
  * @see user_admin_role_submit()
  */
 function user_admin_role($form, $form_state, $role) {
-  if ($role->rid == DRUPAL_ANONYMOUS_RID || $role->rid == DRUPAL_AUTHENTICATED_RID) {
-    drupal_goto('admin/people/permissions/roles');
-  }
+  $form['role'] = array(
+    '#tree' => TRUE,
+    '#parents' => array('role'),
+  );
 
-  // Display the edit role form.
-  $form['name'] = array(
+  $form['role']['name'] = array(
     '#type' => 'textfield',
     '#title' => t('Role name'),
     '#default_value' => $role->name,
     '#size' => 30,
     '#required' => TRUE,
     '#maxlength' => 64,
-    '#description' => t('The name for this role. Example: "moderator", "editorial board", "site architect".'),
+    '#description' => t('The name for this role. Example: "Moderator", "Editorial board", "Site architect".'),
   );
-  $form['rid'] = array(
-    '#type' => 'value',
-    '#value' => $role->rid,
+  $form['role']['rid'] = array(
+    '#type' => 'machine_name',
+    '#default_value' => $role->rid,
+    '#required' => TRUE,
+    '#disabled' => !empty($role->rid),
+    '#size' => 30,