Add a confirm form for role deletion

Attached patch will add confirm form.

Probably you need to change some tests

The tests need to be updated accordingly.

+++ modules/user/user.admin.inc	26 Mar 2010 10:29:37 -0000
@@ -961,15 +961,18 @@ function user_admin_role_validate($form,
+ * Form submit handler for the user_admin_role() form.
+ */ ¶

Trailing white-space here (and below).

+++ modules/user/user.admin.inc	26 Mar 2010 10:29:37 -0000
@@ -961,15 +961,18 @@ function user_admin_role_validate($form,
+  elseif ($form_state['values']['op'] == t('Delete role')) {    ¶
+    $form_state['redirect'] = 'admin/people/permissions/roles/delete/' . $form_state['values']['rid'];
+    return;
   }

This should really be a separate form button submit handler.

+++ modules/user/user.admin.inc	26 Mar 2010 10:29:37 -0000
@@ -980,6 +983,30 @@ function user_admin_role_submit($form, &
+  user_role_delete((int) $form_state['values']['rid']);

The cast to integer shouldn't be required.

142 critical left. Go review some!

1. Updated test.

// Test deleting a role.
-    $this->drupalPost("admin/people/permissions/roles/edit/{$role->rid}", NULL, t('Delete role'));
+    $this->drupalPost("admin/people/permissions/roles/delete/{$role->rid}", NULL, t('Delete'));

2. Removed type casting and $form['name'] from user_admin_role_delete_confirm().

+++ modules/user/user.module	26 Mar 2010 13:54:51 -0000
@@ -1533,7 +1533,13 @@ function user_menu() {
+  $items['admin/people/permissions/roles/delete/%user_role'] = array(
+    'title' => 'Delete role',
+    'page arguments' => array('user_admin_role_delete_confirm', 5),

We are missing a 'page callback' here.

+++ modules/user/user.admin.inc	26 Mar 2010 13:54:48 -0000
@@ -968,8 +971,8 @@ function user_admin_role_submit($form, &
   elseif ($form_state['values']['op'] == t('Delete role')) {
-    user_role_delete((int) $form_state['values']['rid']);
-    drupal_set_message(t('The role has been deleted.'));
+    $form_state['redirect'] = 'admin/people/permissions/roles/delete/' . $form_state['values']['rid'];
+    return;
   }

We still want to add a separate form button #submit handler user_admin_role_delete_submit() instead of this mixing here.

+++ modules/user/user.test	26 Mar 2010 13:54:53 -0000
@@ -1506,7 +1506,7 @@ class UserRoleAdminTestCase extends Drup
     // Test deleting a role.
-    $this->drupalPost("admin/people/permissions/roles/edit/{$role->rid}", NULL, t('Delete role'));
+    $this->drupalPost("admin/people/permissions/roles/delete/{$role->rid}", NULL, t('Delete'));

We should keep the original line here and just add a

$this->drupalPost(NULL, NULL, t('Delete'));

afterwards, so the proper redirection is tested as well.

142 critical left. Go review some!

@Sun Thanks for your comments. Type casting is required otherwise user_role_delete() does not works as expected.

We are missing a 'page callback' here.

Added page callback.

FYI: Role edit menu item is also not using 'page callback' see http://drupal.org/node/619584#comment-2771532

We still want to add a separate form button #submit handler user_admin_role_delete_submit() instead of this mixing here.

I am not sure how to add #submit there.

We should keep the original line here and just add a $this->drupalPost(NULL, NULL, t('Delete'));

Done.

@sivaji Take an example from http://api.drupal.org/api/function/node_form/7 for handler node_form_delete_submit()

I guess point 2 in #9 mean this

@@ -934,6 +934,7 @@ function user_admin_role($form, $form_st
   $form['actions']['delete'] = array(
     '#type' => 'submit',
     '#value' => t('Delete role'),
+    '#submit' => array('user_admin_role_delete_submit'),
   );

 function user_admin_role_submit($form, &$form_state) {
   $role = (object)$form_state['values'];
   if ($form_state['values']['op'] == t('Save role')) {
     user_role_save($role);
     drupal_set_message(t('The role has been renamed.'));
   }
-  elseif ($form_state['values']['op'] == t('Delete role')) {
-    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')) {
     user_role_save($role);
     drupal_set_message(t('The role has been added.'));

 /**
+ * Form submit handler for the user_admin_role() form.
+ */
+function user_admin_role_delete_submit($form, &$form_state) {
+  $form_state['redirect'] = 'admin/people/permissions/roles/delete/' . $form_state['values']['rid'];
+}
+

This looked good to me. Committed to CVS HEAD. Thanks!