Deleting user role throws PHP notices and prevents delete operation

This patch works great for me.

In function user_role_delete changing param $role to $rid will make better sense.

Patch works. Seems the most straight-forward way of solving this.

We definitely need tests for this. This is critical functionality that should not ever be broken.

@webchick I added a patch for the missing tests here: http://drupal.org/node/624854.

It currently fails automated testing as it needs this patch to go in first, though.

Tested patch. Patch makes bad thing go away, is good :)

@gcopenhaver I agree with you param $role makes sense. I didn't read the comment line properly then.

Changing title and re-rolling patch with test from #624854: Tests for adding, editing and deleting roles

A better test.

+++ modules/user/user.admin.inc	27 Nov 2009 19:36:37 -0000
@@ -826,15 +826,15 @@ function user_admin_role_submit($form, &
   if ($form_state['values']['op'] == t('Save role')) {
     user_role_save($role);
-    drupal_set_message(t('The role has been renamed.'));
+    drupal_set_message(t('The role %name has been renamed.', array('%name' => $role->name)));

1) $role is altered by reference, so this message will display the renamed role name for %name.

2) Not sure why we presume that a role has been renamed without determining it first, but that's entirely wrong. (It's true that Drupal core does not allow to do something else on the form, but hey, forms can be altered! :)

+++ modules/user/user.admin.inc	27 Nov 2009 19:36:37 -0000
@@ -826,15 +826,15 @@ function user_admin_role_submit($form, &
   elseif ($form_state['values']['op'] == t('Delete role')) {
-    user_role_delete($form_state['values']['rid']);
...
+    user_role_delete((int)$form_state['values']['rid']);

Whatever this cast to integer tries to fix, it's wrong, because if 'rid' doesn't contain a numeric role id, then something entirely different entirely elsewhere went entirely wrong.

+++ modules/user/user.test	27 Nov 2009 19:36:39 -0000
@@ -1315,3 +1315,65 @@ class UserEditTestCase extends DrupalWeb
+  protected $admin_user;
+  protected $role;
+  protected $rid;

Not strictly necessary.

+++ modules/user/user.test	27 Nov 2009 19:36:39 -0000
@@ -1315,3 +1315,65 @@ class UserEditTestCase extends DrupalWeb
+    $all_rids = array_keys($this->admin_user->roles);
+    sort($all_rids);
+    $this->rid = array_pop($all_rids);

You want to use max(array_keys($this->admin_user->roles)) instead.

+++ modules/user/user.test	27 Nov 2009 19:36:39 -0000
@@ -1315,3 +1315,65 @@ class UserEditTestCase extends DrupalWeb
+    $this->drupalLogin($this->admin_user);
...
+    $this->drupalLogin($this->admin_user);
...
+    $this->drupalLogin($this->admin_user);

Seems like this can be moved into setUp().

+++ modules/user/user.test	27 Nov 2009 19:36:39 -0000
@@ -1315,3 +1315,65 @@ class UserEditTestCase extends DrupalWeb
+    $this->assertText(t('The role %name has been added.', array('%name' => $edit['name'])), t('Successful save message displayed.'));
...
+    $this->assertText(t('The role %name has been renamed.', array('%name' => $edit['name'])), t('Successful editing message displayed.'));
...
+    $this->assertText(t('The role %name has been deleted.', array('%name' => $edit['name'])), t('Successful delete message displayed.'));

We don't need the additional assertion message ("Successfully...") here.

+++ modules/user/user.test	27 Nov 2009 19:36:39 -0000
@@ -1315,3 +1315,65 @@ class UserEditTestCase extends DrupalWeb
+   /**
+   * Test deleting a role.
+   */

Wrong indentation.

+++ modules/user/user.test	27 Nov 2009 19:36:39 -0000
@@ -1315,3 +1315,65 @@ class UserEditTestCase extends DrupalWeb
+  function testDeleteRole() {
...
+    $edit = array('name' => $this->role);
+    $this->drupalPost("admin/config/people/roles/edit/{$this->rid}", $edit, t('Delete role'));

We want to pass an empty array() instead of $edit here.

+++ modules/user/user.test	27 Nov 2009 19:36:39 -0000
@@ -1315,3 +1315,65 @@ class UserEditTestCase extends DrupalWeb
+  }
+
+}

No blank line here, please.

This review is powered by Dreditor.

@sun Thank you for reviewing and correcting the mistakes in my patch. Attached is the new patch which addresses all the above mentioned corrections.

Attached patch corrects the PHP syntax error.

Removed assertText() as suggested in #13.

+++ modules/user/user.admin.inc	30 Nov 2009 09:31:18 -0000
@@ -760,11 +760,14 @@ function theme_user_admin_permissions($v
 function user_admin_role() {
   $rid = arg(5);
   if ($rid) {
-    if ($rid == DRUPAL_ANONYMOUS_RID || $rid == DRUPAL_AUTHENTICATED_RID) {
+    if ($rid == DRUPAL_ANONYMOUS_RID || $rid == DRUPAL_AUTHENTICATED_RID || !is_numeric($rid)) {
       drupal_goto('admin/config/people/roles');
     }
     // Display the edit role form.
     $role = db_query('SELECT * FROM {role} WHERE rid = :rid', array(':rid' => $rid))->fetchObject();
+    if (empty($role)) {
+      drupal_goto('admin/config/people/roles');
+    }

wuhuhuh? We still have arg() code in HEAD?! OMG.

Do you see a way to remove the usage of arg() for this function?

Or, to shortcut this: We need a menu argument loader function here and the function needs to take $role as argument.

The first condition that checks for anonymous/authenticated/named must be removed and a page request that was redirected here should _never_ reach this function in the first place.

+++ modules/user/user.admin.inc	30 Nov 2009 09:31:18 -0000
@@ -826,15 +829,15 @@ function user_admin_role_submit($form, &
-    drupal_set_message(t('The role has been renamed.'));
+    drupal_set_message(t('Renamed role to %name.', array('%name' => $role->name)));
...
-    user_role_delete($form_state['values']['rid']);
-    drupal_set_message(t('The role has been deleted.'));
+    user_role_delete((int)$form_state['values']['rid']);
+    drupal_set_message(t('Deleted role %name.', array('%name' => $role->name)));
...
-    drupal_set_message(t('The role has been added.'));
+    drupal_set_message(t('Added new role %name.', array('%name' => $role->name)));

These changes can be reverted, because they have nothing to do with this bug.

This review is powered by Dreditor.

Or, to shortcut this: We need a menu argument loader function here and the function needs to take $role as argument.

@sun This sounds good for me. But i am struck with menu argument loader, i changed user module's hook_menu item like

$items['admin/config/people/roles/edit/%user_role'] = array(
    'title' => 'Edit role',
    'page callback' => 'user_admin_role',
    'page arguments' => array(5),
    'access arguments' => array('administer permissions'),
    'type' => MENU_CALLBACK,
    'file' => 'user.admin.inc',
  );

This invokes user_role_load already define in user.module file but page callback user_admin_role never invoked instead i get drupal's "page not found" error. If i replace "%user_role" with "%" it invokes user_admin_role. I even replaced "%user_role" with "%user", '%user_uid_optional' etc. but no hope. In all the case the respective *_load function is called and drupal's "page not found" error is returned. I cleared cache, ran cron still it doesn't works.

Let me know how to proceed with this.

In IRC mbutcher and sdboyer helped me to figure out the problem explained in #20. Attached patch makes the necessary changes mentioned in #19 except status message, because i prefer to have short and more descriptive messages.

+++ modules/user/user.admin.inc	4 Dec 2009 09:05:14 -0000
@@ -761,19 +761,16 @@ function theme_user_admin_permissions($v
+ *   Object containing role name and id.

A user role object as returned from user_admin_role_load().

+++ modules/user/user.admin.inc	4 Dec 2009 09:05:14 -0000
@@ -761,19 +761,16 @@ function theme_user_admin_permissions($v
+  if ($role) {

!empty()

+++ modules/user/user.admin.inc	4 Dec 2009 09:05:14 -0000
@@ -812,6 +809,9 @@ function user_admin_role() {
+ * Validation function for the user role add and edit form.

Form validation handler for user_admin_role() form.

+++ modules/user/user.admin.inc	4 Dec 2009 09:05:14 -0000
@@ -831,19 +831,22 @@ function user_admin_role_validate($form,
+ * Submit function for user role add and edit form.

Form submit handler for user_admin_role() form.

+++ modules/user/user.admin.inc	4 Dec 2009 09:05:14 -0000
@@ -831,19 +831,22 @@ function user_admin_role_validate($form,
+    user_role_delete((int)$form_state['values']['rid']);

Do we still have to convert to an integer here? If so, there should be an inline comment above this line that quickly explains why.

+++ modules/user/user.module	4 Dec 2009 09:05:16 -0000
@@ -1625,6 +1625,22 @@ function user_category_load($uid, &$map,
+ *   The ID of a user role to alter.

s/to alter/to load/

+++ modules/user/user.module	4 Dec 2009 09:05:16 -0000
@@ -1625,6 +1625,22 @@ function user_category_load($uid, &$map,
+ * @see user_admin_role()
+ */
+function user_admin_role_load($rid) {

We also want to add

@see user_role_load()

+++ modules/user/user.module	4 Dec 2009 09:05:16 -0000
@@ -1625,6 +1625,22 @@ function user_category_load($uid, &$map,
+    // This will set page not found

Can be removed.

+++ modules/user/user.module	4 Dec 2009 09:05:16 -0000
@@ -2376,13 +2392,15 @@ function user_roles($membersonly = FALSE
+  // is_int() never sets $field to 'rid'

This inline comment can be removed. (and would have to be above the commented-on line anyway)

+++ modules/user/user.module	4 Dec 2009 09:05:16 -0000
@@ -2424,7 +2442,6 @@ function user_role_save($role) {
-

Please revert.

+++ modules/user/user.test	4 Dec 2009 09:05:19 -0000
@@ -1319,3 +1319,59 @@ class UserEditTestCase extends DrupalWeb
+class UserRoleEditTestCase extends DrupalWebTestCase {

UserRoleAdminTestCase

+++ modules/user/user.test	4 Dec 2009 09:05:19 -0000
@@ -1319,3 +1319,59 @@ class UserEditTestCase extends DrupalWeb
+  protected $admin_user;
+  protected $role;
+  protected $rid;

These are technically not required and can be removed.

+++ modules/user/user.test	4 Dec 2009 09:05:19 -0000
@@ -1319,3 +1319,59 @@ class UserEditTestCase extends DrupalWeb
+      'name' => 'Role edit',

User role administration

+++ modules/user/user.test	4 Dec 2009 09:05:19 -0000
@@ -1319,3 +1319,59 @@ class UserEditTestCase extends DrupalWeb
+      'description' => 'Test role edit page.',

Test adding, editing, and deleting user roles.

+++ modules/user/user.test	4 Dec 2009 09:05:19 -0000
@@ -1319,3 +1319,59 @@ class UserEditTestCase extends DrupalWeb
+    $this->assertNoRaw("admin/config/people/roles/edit/{$this->rid}", t('Role edit link removed.'));

Should rather use assertNoLinkByHref()

Powered by Dreditor.

Do we still have to convert to an integer here? If so, there should be an inline comment above this line that quickly explains why.

No. It works fine irrespective of type casting.

Thanks for reviewing this. It looks much better now. All the changes has been done in the attached patch.

+++ modules/user/user.module	25 Jan 2010 09:26:39 -0000
@@ -1681,6 +1681,21 @@ function user_category_load($uid, &$map,
+ * Returns a role object.
+ * @param $rid
+ *   The ID of user role to load.
+ * @see user_admin_role()
+ * @see user_role_load()

There should be a blank line after the first summary line, and there should be a @return directive (See user_load for an example).

+++ modules/user/user.module	25 Jan 2010 09:26:39 -0000
@@ -1681,6 +1681,21 @@ function user_category_load($uid, &$map,
+function user_admin_role_load($rid) {
+  if ($rid == DRUPAL_ANONYMOUS_RID || $rid == DRUPAL_AUTHENTICATED_RID) {
+    return FALSE;
+  }
+  return user_role_load($rid);
+}

Uh oh. This changed the behavior in a way that we can now visit admin/config/people/roles/edit/<role-name> too. Not necessarily bad, but I'm not sure we should allow this. I managed to delete the authenticated user role by visiting "admin/config/people/roles/edit/authenticated user" and pressing delete.

Powered by Dreditor.

Forgot to switch status...

This changed the behavior in a way that we can now visit admin/config/people/roles/edit/ too. Not necessarily bad, but I'm not sure we should allow this. I managed to delete the authenticated user role by visiting "admin/config/people/roles/edit/authenticated user" and pressing delete.

Nothing unusual here. You can also directly type node/2/edit or even node/2/delete in your browser's address bar.

@sun: that's not exactly the same though... In this case the menu loader checks the $rid against the authenticated and anonymous roles and returns a 404 for them, allowing you to only change custom roles. However, if you use "authenticated user" instead of "2" in the URL, you bypass that check. It's inconsistent.

I'd prefer an additional is_numeric check:

function user_admin_role_load($rid) {
  if (!is_numeric($rid) || $rid == DRUPAL_ANONYMOUS_RID || $rid == DRUPAL_AUTHENTICATED_RID) {
    return FALSE;
  }
  return user_role_load($rid);
}

Re-rolled...

• ... because of #699842: Move permissions to a tab at admin/people/permissions.
• Removed the changed messages. We should focus on fixing the bug, the messages can be changed elsewhere...
• Added the is_numeric check in the wildcard loader function.
• Grouped the add/edit/delete tests in one test method to improve performance.

Marked #722636: Don't delete user roles (custom user roles) and #687912: Can't delete custom user roles as duplicates.

This is a critical bug, right?

+ * @param $role
+ *   A user role object as returned from user_admin_role_load().
  * @ingroup forms

Minor, but I think there should be a blank line before the @ingroup line.

+  /**
+   * Test adding, renaming and deleting roles.
+   */
+  function testRoleAdministration() {
+    $this->drupalLogin($this->admin_user);
+    // Test adding a role.
+    $this->role = $this->randomName();
+    $edit = array('name' => $this->role);
+    $this->drupalPost('admin/people/permissions/roles', $edit, t('Add role'));
+    $this->assertText(t('The role has been added.'), t('The role has been added.'));

These tests seem a bit fragile... Why aren't we testing that the role is actually found in the database? (Similar comment applies for other tests.)

+function user_admin_role_load($rid) {
+  if (!is_numeric($rid) || $rid == DRUPAL_ANONYMOUS_RID || $rid == DRUPAL_AUTHENTICATED_RID) {
+    return FALSE;
+  }

I think at a minimum this needs a code comment explaining why in the world we want to avoid loading the anonymous and authenticated user roles here...

 function user_role_load($role) {
-  $field = is_int($role) ? 'rid' : 'name';
+  $field = is_numeric($role) ? 'rid' : 'name';

As stated in the original post, this is going to cause havoc if anyone ever uses a number as the name of their role. Granted, that's a bit of an edge case, but we still don't want it to break. (For some fun, trying applying this patch and then trying to create two roles on your site, both named "22" ... explosions ensue.)

I think the right way to fix this bug is to have two different functions: user_role_load() should only accept a role ID, and user_role_load_by_name() should only accept a role name. This would also be consistent with the way the existing user_load() and user_load_by_name() functions work.

That's an API change, of course. I don't make the decisions, but if it were up to me, I'd definitely think it's worth a minor change to the API in order to fix this bug the right way :)

+function user_admin_role_load($rid) {
+  if (!is_numeric($rid) || $rid == DRUPAL_ANONYMOUS_RID || $rid == DRUPAL_AUTHENTICATED_RID) {
+    return FALSE;
+  }

I think at a minimum this needs a code comment explaining why in the world we want to avoid loading the anonymous and authenticated user roles here...

Actually, more to the point: This type of code belongs in an access callback, not in the loader function. In other words, ideally the menu item should just use %user_role here so that the normal user_role_load() is called, and denying access for anonymous and authenticated should happen in the access callback.

This might only be possible if we make the API change I suggested above - but in that case, it's just another reason why we should do that.

Thanks for the feedback David!

1. I think your suggestion of splitting up user_role_load into user_role_load($rid) and user_role_load_by_name($name) is sensible. I've included this change in the patch, lets see if this can still get into D7.
2. user_role_delete was slightly altered to call either user_role_load or user_role_load_by_name depending on the type of input. I wonder if we should a) either limit user_role_delete to accept only ints, or b) split it up into a user_role_delete and a user_role_delete_by_name (the latter probably being overkill). This obiously depends on whether the change in 1. has any chance of making it into D7.
3. Because user_role_delete can still take either a string or an int, we still need to cast to (int) as was done in the OP (addressing 2. would allow us to skip the cast)
4. Dropped user_admin_role_load (which was introduced by earlier patches in this issue) again. User_role_load is now used as menu loader.
5. Moved the authenticated/anonymous exclusion to a newly created access callback (user_role_edit_access). My only concern here is that I'm wondering whether "not found" would not be better than "access denied".
6. Updated the tests

Fixed typo.

After applying the last patch, the edit page is broken.

When going to "edit role" it displays following errors:

    *  Notice: Trying to get property of non-object in user_admin_role()  (line 784 of F:\xampp\htdocs\drupal\modules\user\user.admin.inc).
    * Notice: Trying to get property of non-object in user_admin_role() (line 792 of F:\xampp\htdocs\drupal\modules\user\user.admin.inc).

I'm guessing you need to rebuild the menu since the menu loader was changed. Can you clear your cache (Administer > Configuration > Development > Performance) and report back if this does not make the notices go away?

I'd go for user_role_delete() only taking an int. I can't imagine vast numbers of D7 modules are using it with a string, and it didnt exist in D6, so in the scale of API changes it's pretty small. Also why do we load the role if an int is passed, that'd be the ID anyway no?

answering #36: yes, after clearing my cach, everything works correctly!

I tested again against the most current HEAD.

+  if (isset($role) && ($role->rid == 1 || $role->rid == 2)) {

1 and 2 should be a RUPAL_ANONYMOUS_RID and DRUPAL_AUTHENTICATED_RID

Here's a reroll that addresses #39, and also makes some small fixes/improvements to the documentation and tests.

I didn't change user_role_delete() to only take an integer just yet, since it doesn't seem 100% necessary to fix this bug - the is_int() check should work OK. However, I definitely think it would be a good idea :)

We also should do a followup issue somewhere about why there is no confirmation form on "delete role"... that's a little dangerous. However, it's a preexisting problem.

Following, as David pointed out to me, this issue would be a tremendous help to #228061: Usability UMN: Allow roles to be weighted.

Works for me. Opened #750290: Add a confirm form for role deletion so it doesn't get lost. RTBC.

Committed to CVS HEAD.

For #40

We also should do a followup issue somewhere about why there is no confirmation form on "delete role"... that's a little dangerous. However, it's a preexisting problem.

I have submitted a patch for this #750290: Add a confirm form for role deletion, need reviews.

I find some bug in the above committed patch. When fixing #750290: Add a confirm form for role deletion Sun pointed out that i am missing page callback. In the above patch user_menu(), $items['admin/people/permissions/roles/edit/%user_role'] does not uses 'page callback' doing so will make the menu system to use parent items callback (@see Return value section http://api.drupal.org/api/function/hook_menu/7). Unfortunately the parent item $items['admin/people/permissions/roles'] uses the required page callback => drupal_get_form(). Any changes we make in parent menu item will break the delete operation isn't it ?

it is not, let's continue at #750290: Add a confirm form for role deletion