I tried to delete a role that I had created on the admin/config/people/roles page and got these "notices".

* Notice: Trying to get property of non-object in user_role_delete() (line 2417 of /home/greg/workspace/drupal-head/modules/user/user.module).
* Notice: Trying to get property of non-object in user_role_delete() (line 2420 of /home/greg/workspace/drupal-head/modules/user/user.module).
* Notice: Trying to get property of non-object in user_role_delete() (line 2424 of /home/greg/workspace/drupal-head/modules/user/user.module).
* Notice: Trying to get property of non-object in block_user_role_delete() (line 913 of /home/greg/workspace/drupal-head/modules/block/block.module).

It also said:

The role has been deleted.

But the role was not deleted.

I did a typecast in user_admin_role_submit() on line 823 of user.admin.inc. Now the roles are deleted as they should be.

I think this is a better option than changing the is_int() to is_numeric() in user_role_load(), as that would break things if someone attempted to use a number as the name of the role.

I've provided a patch.

Files: 
CommentFileSizeAuthor
#40 user-roles-619584-40.patch9.47 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user-roles-619584-40.patch. View
#35 2010-03-19_editpage.png25.98 KBGeorg
#34 user_role.patch8.27 KBmr.baileys
PASSED: [[SimpleTest]]: [MySQL] 18,779 pass(es). View
#33 user_role.patch8.27 KBmr.baileys
PASSED: [[SimpleTest]]: [MySQL] 18,767 pass(es). View
#28 user-roles-delete.patch5.77 KBmr.baileys
PASSED: [[SimpleTest]]: [MySQL] 17,648 pass(es). View
#23 619584_deleting_role_throws_notice_6.patch7.22 KBSivaji
PASSED: [[SimpleTest]]: [MySQL] 17,424 pass(es). View
#21 619584_deleting_role_throws_notice.5.patch6.75 KBSivaji
Passed on all environments. View
#18 619584_deleting_role_throws_notice.4.patch4.04 KBSivaji
Passed on all environments. View
#16 619584_deleting_role_throws_notice.3.patch4.22 KBSivaji
Failed on MySQL 5.0 ISAM, with: 15,430 pass(es), 3 fail(s), and 0 exception(es). View
#14 619584_deleting_role_throws_notice.3.patch4.22 KBSivaji
Invalid PHP syntax in user.test. View
#10 619584_deleting_role_throws_notice.patch3.88 KBSivaji
Failed on MySQL 5.0 ISAM, with: 15,343 pass(es), 3 fail(s), and 0 exception(es). View
#7 619584_deleting_role_throws_notice.patch3.65 KBSivaji
Failed on MySQL 5.0 ISAM, with: 15,345 pass(es), 3 fail(s), and 0 exception(es). View
user.admin_.inc_.patch718 bytespreventingchaos
Passed: 14662 passes, 0 fails, 0 exceptions View

Comments

Sivaji’s picture

This patch works great for me.

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

preventingchaos’s picture

I think using $role in user_role_delete() makes sense, since it is able to accept the role name or the role rid.

The problem was that user_role_delete() relies on user_role_load() which determines whether it got the role rid or role name based on the variable type. If the type of $role is an integer, it's expecting the role rid. If the type of $role is not an integer, it's expecting the role name. The role rid that user_admin_role_submit() gets from the form is a string, despite being a numeric value, and is calling user_role_delete() with this string, causing an incorrect database query to be run. Drupal 6 didn't have this problem because user_admin_role_submit() had ran it's own database queries to delete the role by the role rid, rather than calling a special function for doing it like it does in Drupal 7. For Drupal 7, I think the only thing that needs to be changed is to typecast the numeric string to an integer, and everything should as expected.

Example without typecasting ($form_state['values']['rid'] == '1'):
user_admin_role_submit() calls user_role_delete($role = '1') which calls user_role_load($role = '1')
causes user_role_load() to set $field to 'name', searching the database for a role named '1'.

With typecasting as an integer ((int)$form_state['values']['rid'] == 1):
user_admin_role_submit() calls user_role_delete($role = 1) which calls user_role_load($role = 1)
causes user_role_load() to set $field to 'rid', searching the database for a role with the rid of 1.

mcjim’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs Documentation

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

mcjim’s picture

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

joachim’s picture

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

Sivaji’s picture

Title: Cannot delete roles » Deleting user role throws PHP notices and prevents delete operation
FileSize
3.65 KB
Failed on MySQL 5.0 ISAM, with: 15,345 pass(es), 3 fail(s), and 0 exception(es). View

@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

Sivaji’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

Sivaji’s picture

FileSize
3.88 KB
Failed on MySQL 5.0 ISAM, with: 15,343 pass(es), 3 fail(s), and 0 exception(es). View

A better test.

Sivaji’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

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

Sivaji’s picture

Status: Needs work » Needs review
FileSize
4.22 KB
Invalid PHP syntax in user.test. View

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

Status: Needs review » Needs work

The last submitted patch failed testing.

Sivaji’s picture

Status: Needs work » Needs review
FileSize
4.22 KB
Failed on MySQL 5.0 ISAM, with: 15,430 pass(es), 3 fail(s), and 0 exception(es). View

Attached patch corrects the PHP syntax error.

Status: Needs review » Needs work

The last submitted patch failed testing.

Sivaji’s picture

Status: Needs work » Needs review
FileSize
4.04 KB
Passed on all environments. View

Removed assertText() as suggested in #13.

sun’s picture

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

Sivaji’s picture

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.

Sivaji’s picture

FileSize
6.75 KB
Passed on all environments. View

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.

sun’s picture

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

Sivaji’s picture

FileSize
7.22 KB
PASSED: [[SimpleTest]]: [MySQL] 17,424 pass(es). View

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.

mr.baileys’s picture

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

mr.baileys’s picture

Status: Needs review » Needs work

Forgot to switch status...

sun’s picture

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.

mr.baileys’s picture

@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);
}
mr.baileys’s picture

FileSize
5.77 KB
PASSED: [[SimpleTest]]: [MySQL] 17,648 pass(es). View

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

Status: Needs work » Needs review
marcvangend’s picture

David_Rothstein’s picture

Priority: Normal » Critical
Status: Needs review » Needs work

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

David_Rothstein’s picture

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

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
8.27 KB
PASSED: [[SimpleTest]]: [MySQL] 18,767 pass(es). View

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

FileSize
8.27 KB
PASSED: [[SimpleTest]]: [MySQL] 18,779 pass(es). View

Fixed typo.

Georg’s picture

FileSize
25.98 KB

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).
mr.baileys’s picture

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?

catch’s picture

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?

Georg’s picture

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

I tested again against the most current HEAD.

andypost’s picture

Status: Needs review » Needs work
+  if (isset($role) && ($role->rid == 1 || $role->rid == 2)) {

1 and 2 should be a RUPAL_ANONYMOUS_RID and DRUPAL_AUTHENTICATED_RID

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
9.47 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user-roles-619584-40.patch. View

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.

quicksketch’s picture

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

catch’s picture

Status: Needs review » Reviewed & tested by the community

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

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD.

Sivaji’s picture

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.

Sivaji’s picture

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 ?

andypost’s picture

Status: Fixed » Closed (fixed)
Issue tags: -Needs Documentation

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