- Patch #619584 by sivaji, mr.baileys, David_Rothstein, gcopenhaver: deleting a user role throws PHP notices and prevents delete operation.

modules/user/user.admin.inc

@@ -803,19 +803,19 @@ function theme_user_permission_description($variables) {
 /**
  * Menu callback: administer roles.
  *
+ * @param $role
+ *   A user role object, as returned from user_role_load(). This represents the
+ *   role which will be edited. If not set, a new role will be added instead.
+ *
  * @ingroup forms
+ * @see user_role_load()
  * @see user_admin_role_validate()
  * @see user_admin_role_submit()
  * @see theme_user_admin_new_role()
  */
-function user_admin_role() {
+function user_admin_role($form, &$form_state, $role = NULL) {
-  $rid = arg(5);
+  if (!empty($role)) {
-  if ($rid) {
-    if ($rid == DRUPAL_ANONYMOUS_RID || $rid == DRUPAL_AUTHENTICATED_RID) {
-      drupal_goto('admin/people/permissions/roles');
-    }
     // Display the edit role form.
-    $role = db_query('SELECT * FROM {role} WHERE rid = :rid', array(':rid' => $rid))->fetchObject();
     $form['name'] = array(
       '#type' => 'textfield',
       '#title' => t('Role name'),
@@ -827,7 +827,7 @@ function user_admin_role() {
     );
     $form['rid'] = array(
       '#type' => 'value',
-      '#value' => $rid,
+      '#value' => $role->rid,
     );
     $form['actions'] = array('#type' => 'container', '#attributes' => array('class' => array('form-actions')));
     $form['actions']['submit'] = array(
@@ -855,16 +855,19 @@ function user_admin_role() {
   return $form;
 }
 
+/**
+ * Form validation handler for the user_admin_role() form.
+ */
 function user_admin_role_validate($form, &$form_state) {
-  if ($form_state['values']['name']) {
+  if (!empty($form_state['values']['name'])) {
     if ($form_state['values']['op'] == t('Save role')) {
-      $role = user_role_load($form_state['values']['name']);
+      $role = user_role_load_by_name($form_state['values']['name']);
       if ($role && $role->rid != $form_state['values']['rid']) {
         form_set_error('name', t('The role name %name already exists. Choose another role name.', array('%name' => $form_state['values']['name'])));
       }
     }
     elseif ($form_state['values']['op'] == t('Add role')) {
-      if (user_role_load($form_state['values']['name'])) {
+      if (user_role_load_by_name($form_state['values']['name'])) {
         form_set_error('name', t('The role name %name already exists. Choose another role name.', array('%name' => $form_state['values']['name'])));
       }
     }
@@ -881,7 +884,7 @@ function user_admin_role_submit($form, &$form_state) {
     drupal_set_message(t('The role has been renamed.'));
   }
   elseif ($form_state['values']['op'] == t('Delete role')) {
-    user_role_delete($form_state['values']['rid']);
+    user_role_delete((int) $form_state['values']['rid']);
     drupal_set_message(t('The role has been deleted.'));
   }
   elseif ($form_state['values']['op'] == t('Add role')) {

modules/user/user.module

@@ -1529,10 +1529,11 @@ function user_menu() {
     'type' => MENU_LOCAL_TASK,
     'weight' => -5,
   );
-  $items['admin/people/permissions/roles/edit'] = array(
+  $items['admin/people/permissions/roles/edit/%user_role'] = array(
     'title' => 'Edit role',
-    'page arguments' => array('user_admin_role'),
+    'page arguments' => array('user_admin_role', 5),
-    'access arguments' => array('administer permissions'),
+    'access callback' => 'user_role_edit_access',
+    'access arguments' => array(5),
     'type' => MENU_CALLBACK,
   );
 
@@ -2532,22 +2533,45 @@ function user_roles($membersonly = FALSE, $permission = NULL) {
 }
 
 /**
- * Fetch a user role from database.
+ * Fetches a user role by role ID.
+ *
+ * @param $rid
+ *   An integer representing the role ID.
  *
- * @param $role
- *   A string with the role name, or an integer with the role ID.
  * @return
- *   A fully-loaded role object if a role with the given name or ID
+ *   A fully-loaded role object if a role with the given ID exists, or FALSE
- *   exists, FALSE otherwise.
+ *   otherwise.
+ *
+ * @see user_role_load_by_name()
  */
-function user_role_load($role) {
+function user_role_load($rid) {
-  $field = is_int($role) ? 'rid' : 'name';
   return db_select('role', 'r')
     ->fields('r')
-    ->condition($field, $role)
+    ->condition('rid', $rid)
     ->execute()
     ->fetchObject();
 }
+
+/**
+ * Fetches a user role by role name.
+ *
+ * @param $role_name
+ *   A string representing the role name.
+ *
+ * @return
+ *   A fully-loaded role object if a role with the given name exists, or FALSE
+ *   otherwise.
+ *
+ * @see user_role_load()
+ */
+function user_role_load_by_name($role_name) {
+  return db_select('role', 'r')
+    ->fields('r')
+    ->condition('name', $role_name)
+    ->execute()
+    ->fetchObject();
+}
+
 /**
  * Save a user role to the database.
  *
@@ -2588,7 +2612,12 @@ function user_role_save($role) {
  *   A string with the role name, or an integer with the role ID.
  */
 function user_role_delete($role) {
-  $role = user_role_load($role);
+  if (is_int($role)) {
+    $role = user_role_load($role);
+  }
+  else {
+    $role = user_role_load_by_name($role);
+  }
 
   db_delete('role')
     ->condition('rid', $role->rid)
@@ -2608,6 +2637,18 @@ function user_role_delete($role) {
   drupal_static_reset('user_role_permissions');
 }
 
+/**
+ * Menu access callback for user role editing.
+ */
+function user_role_edit_access($role) {
+  // Prevent the system-defined roles from being altered or removed.
+  if ($role->rid == DRUPAL_ANONYMOUS_RID || $role->rid == DRUPAL_AUTHENTICATED_RID) {
+    return FALSE;
+  }
+
+  return user_access('administer permissions');
+}
+
 /**
  * Determine the modules that permissions belong to.
  *

modules/user/user.test

@@ -1457,3 +1457,66 @@ class UserEditedOwnAccountTestCase extends DrupalWebTestCase {
     $this->drupalLogin($account);
   }
 }
+
+/**
+ * Test case to test adding, editing and deleting roles.
+ */
+class UserRoleAdminTestCase extends DrupalWebTestCase {
+
+  public static function getInfo() {
+    return array(
+      'name' => 'User role administration',
+      'description' => 'Test adding, editing and deleting user roles.',
+      'group' => 'User',
+    );
+  }
+
+  function setUp() {
+    parent::setUp();
+    $this->admin_user = $this->drupalCreateUser(array('administer permissions', 'administer users'));
+  }
+
+  /**
+   * Test adding, renaming and deleting roles.
+   */
+  function testRoleAdministration() {
+    $this->drupalLogin($this->admin_user);
+
+    // Test adding a role. (In doing so, we use a role name that happens to
+    // 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);
+    $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);
+    $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.'));
+
+    // Test renaming a role.
+    $old_name = $role_name;
+    $role_name = '456';
+    $edit = array('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.'));
+
+    // Test deleting a role.
+    $this->drupalPost("admin/people/permissions/roles/edit/{$role->rid}", NULL, t('Delete role'));
+    $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.'));
+
+    // Make sure that the system-defined roles cannot 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->drupalGet('admin/people/permissions/roles/edit/' . DRUPAL_AUTHENTICATED_RID);
+    $this->assertResponse(403, t('Access denied when trying to edit the built-in authenticated role.'));
+  }
+}
+