Part of #1945406: [meta] Convert all of confirm_form() to ConfirmFormBase

There's 3 confirm_form() usages

user_admin_role_delete_confirn() - needs access controller for Role entity

user_cancel_confirm_form() & user_multiple_cancel_confirm()

Related issues
#1872870: Implement a RoleListController and RoleFormController
#1938884: Replace the fallback user listing with a list controller
#1851086: Replace admin/people with a View

Files: 
CommentFileSizeAuthor
#101 drupal8.user-module.1946466-101.patch32.07 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 59,241 pass(es).
[ View ]
#101 interdiff.txt778 bytesjibran
#100 drupal8.user-module.1946466-100.patch32.07 KBjibran
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#100 interdiff.txt614 bytesjibran
#96 drupal8.user-module.1946466-96.patch32.06 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 59,172 pass(es).
[ View ]
#96 interdiff.txt5.63 KBjibran
#92 drupal8.user-module.1946466-92.patch32.16 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,585 pass(es), 48 fail(s), and 8 exception(s).
[ View ]
#92 interdiff.txt3.95 KBdisasm
#89 drupal8.user-module.1946466-89.patch32.26 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,920 pass(es).
[ View ]
#89 interdiff.txt2.22 KBjibran
#87 drupal8.user-module.1946466-87.patch32.24 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,803 pass(es).
[ View ]
#87 diff.txt1.36 KBjibran
#86 1946466-86.patch32.25 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,126 pass(es).
[ View ]
#83 1946466-83.patch32.24 KBjibran
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1946466-83.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#83 interdiff.txt2.93 KBjibran
#82 1946466-82.patch32.26 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,432 pass(es).
[ View ]
#82 interdiff.txt2.9 KBjibran
#80 1946466-80.patch32.44 KBjibran
FAILED: [[SimpleTest]]: [MySQL] 58,286 pass(es), 48 fail(s), and 26 exception(s).
[ View ]
#80 interdiff.txt14.33 KBjibran
#75 1946466-75.patch32.41 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 57,933 pass(es).
[ View ]
#67 user-1946466-67.patch32.46 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user-1946466-67.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#67 interdiff.txt1014 bytestim.plunkett
#65 user-1946466-65.patch32.43 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,599 pass(es), 9 fail(s), and 0 exception(s).
[ View ]
#65 interdiff.txt6.34 KBtim.plunkett
#63 1946466-confirm_form_user.patch32.15 KBCrell
PASSED: [[SimpleTest]]: [MySQL] 57,630 pass(es).
[ View ]
#63 interdiff.txt2 KBCrell
#60 user-1946466-60.patch32.15 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,471 pass(es).
[ View ]
#60 interdiff.txt939 bytestim.plunkett
#58 user-1946466-58.patch32.11 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,507 pass(es).
[ View ]
#56 user-1946466-56.patch32.07 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user-1946466-56.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#56 interdiff.txt11.7 KBtim.plunkett
#52 confirm-form.patch26.53 KBUnitoch
PASSED: [[SimpleTest]]: [MySQL] 57,312 pass(es).
[ View ]
#45 user-1946466-45.patch27.31 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,104 pass(es).
[ View ]
#42 interdiff.txt783 bytestim.plunkett
#42 user-1946466-42.patch27.29 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user-1946466-42.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#40 user-1946466-40.patch26.52 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,817 pass(es), 5 fail(s), and 3 exception(s).
[ View ]
#40 interdiff.txt6.64 KBtim.plunkett
#35 user-1946466-35.patch24.54 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 55,949 pass(es), 33 fail(s), and 15 exception(s).
[ View ]
#35 interdiff.txt896 bytestim.plunkett
#33 user-1946466-33.patch24.54 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,761 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#33 interdiff.txt2.16 KBtim.plunkett
#31 interdiff.txt4.8 KBtim.plunkett
#31 user-1946466-31.patch23.68 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,416 pass(es), 33 fail(s), and 6 exception(s).
[ View ]
#27 user-1946466-27.patch23.14 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,680 pass(es).
[ View ]
#27 interdiff.txt1.27 KBtim.plunkett
#25 user-1946466-25.patch23.12 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,298 pass(es), 8 fail(s), and 136 exception(s).
[ View ]
#21 interdiff.txt1.71 KBandypost
#21 1946466-confirm-user-21.patch21.66 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 56,073 pass(es).
[ View ]
#20 interdiff.txt1.15 KBandypost
#20 1946466-confirm-user-20.patch21.48 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 55,812 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#18 1946466-confirm-user-18.patch22.22 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 55,722 pass(es), 66 fail(s), and 33 exception(s).
[ View ]
#14 1946466-confirm-user-14.patch27.85 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1946466-confirm-user-14.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#9 interdiff.txt14.46 KBandypost
#9 1946466-confirm-9.patch30.44 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 54,232 pass(es), 51 fail(s), and 27 exception(s).
[ View ]
#8 1946466-confirm-8.patch9.03 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 54,454 pass(es).
[ View ]
#6 interdiff.txt921 bytesandypost
#6 1946466-confirm-6.patch9.3 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 53,306 pass(es).
[ View ]
#4 interdiff.txt1.89 KBandypost
#4 1946466-confirm-4.patch9.29 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 53,344 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#2 1946466-confirm-1.patch8.65 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 53,258 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#1 1946466-confirm-1.patch8.66 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1946466-confirm-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new8.66 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1946466-confirm-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

initial patch

StatusFileSize
new8.65 KB
FAILED: [[SimpleTest]]: [MySQL] 53,258 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Proper patch

Status:Needs review» Needs work

The last submitted patch, 1946466-confirm-1.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new9.29 KB
FAILED: [[SimpleTest]]: [MySQL] 53,344 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new1.89 KB

Fix test for new string

Status:Needs review» Needs work

The last submitted patch, 1946466-confirm-4.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new9.3 KB
PASSED: [[SimpleTest]]: [MySQL] 53,306 pass(es).
[ View ]
new921 bytes

Should be green now.
Patch will require re-roll after path changes #1872870: Implement a RoleListController and RoleFormController

Issue summary:View changes

Updated issue summary.

Status:Needs review» Postponed

Status:Postponed» Needs review
StatusFileSize
new9.03 KB
PASSED: [[SimpleTest]]: [MySQL] 54,454 pass(es).
[ View ]

Patch could be done without waiting unified access check and could be re-rolled after #1947892: Improve DX with EntityAccessControllerInterface
Re-roll for #1939660: Use YAML as the primary means for service registration
Next step here is convert user_cancel_confirm_form()

StatusFileSize
new30.44 KB
FAILED: [[SimpleTest]]: [MySQL] 54,232 pass(es), 51 fail(s), and 27 exception(s).
[ View ]
new14.46 KB

Initial patch form user cancellation forms.
Seems needs common base class/interface to implement common functionality.

Also found user_admin() still uses $_POST and that's data passed to user_multiple_cancel_confirm()

So needs follow-up to refactor this ugliness

Status:Needs review» Needs work

The last submitted patch, 1946466-confirm-9.patch, failed testing.

Issue summary:View changes

Updated issue summary.

A lot of that is cleaned up in #1851086: Replace admin/people with a View. Perhaps this should be postponed.

I think we should convert cancel and delete confirm forms but cancel_multiple could be done with
#1839516: Introduce entity operation providers

Filed #1992428: Convert Roles management to a Controller to finish roles conversion after #1872870: Implement a RoleListController and RoleFormController
So probably better move role-delete form conversion in that issue and deal here with user-cancel forms

Status:Needs work» Needs review
StatusFileSize
new27.85 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1946466-confirm-user-14.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-roll for current HEAD

Status:Needs review» Needs work
Issue tags:-FormInterface, -WSCCI-conversion

The last submitted patch, 1946466-confirm-user-14.patch, failed testing.

Status:Needs work» Needs review

#14: 1946466-confirm-user-14.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+FormInterface, +WSCCI-conversion

The last submitted patch, 1946466-confirm-user-14.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new22.22 KB
FAILED: [[SimpleTest]]: [MySQL] 55,722 pass(es), 66 fail(s), and 33 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 1946466-confirm-user-18.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new21.48 KB
FAILED: [[SimpleTest]]: [MySQL] 55,812 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new1.15 KB

fix route

EDIT still get in UserCancelTest
Fatal error: Call to a member function id() on a non-object in /home/andypost/www/core8/core/modules/user/lib/Drupal/user/Form/UserCancelConfirm.php on line 139

StatusFileSize
new21.66 KB
PASSED: [[SimpleTest]]: [MySQL] 56,073 pass(es).
[ View ]
new1.71 KB

It needs to call a constructor to init protected $this->account variable

+++ b/core/modules/user/lib/Drupal/user/Form/UserMultipleCancelConfirm.phpundefined
@@ -0,0 +1,150 @@
+          $admin_form = new UserCancelConfirm();
+          $admin_form_mock = array();
+          // Calling this directly required to init form object with $account.
+          $admin_form->buildForm($admin_form_mock, $admin_form_state, $account);
+          $admin_form->submitForm($admin_form_mock, $admin_form_state);

This looks absolutely insane. How is this okay?

Another approach could be implementation of userCancel() method for user entity
Also we could implement this on top of #1846172: Replace the actions API
Also related #1851086: Replace admin/people with a View

Assigned:Unassigned» tim.plunkett
Status:Needs review» Needs work

Working on this.

Status:Needs work» Needs review
StatusFileSize
new23.12 KB
FAILED: [[SimpleTest]]: [MySQL] 56,298 pass(es), 8 fail(s), and 136 exception(s).
[ View ]

Not 100% happy with this yet, UserCancelConfirm is weird (in HEAD too, not the fault of the previous patch author).

Status:Needs review» Needs work

The last submitted patch, user-1946466-25.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.27 KB
new23.14 KB
PASSED: [[SimpleTest]]: [MySQL] 57,680 pass(es).
[ View ]

BC NG OMG

+++ b/core/modules/user/lib/Drupal/user/Form/UserMultipleCancelConfirm.phpundefined
@@ -0,0 +1,187 @@
+          $admin_form = new UserCancelConfirm();
+          // Calling this directly required to init form object with $account.
+          $admin_form->buildForm($admin_form_mock, $admin_form_state, $account);
+          $admin_form->submitForm($admin_form_mock, $admin_form_state);

This is unfortunate, but necessary.

Status:Needs review» Reviewed & tested by the community

Should be ok

Status:Reviewed & tested by the community» Needs work

Status:Needs work» Needs review
StatusFileSize
new23.68 KB
FAILED: [[SimpleTest]]: [MySQL] 58,416 pass(es), 33 fail(s), and 6 exception(s).
[ View ]
new4.8 KB

This might need more work, but here's an initial reroll.

Status:Needs review» Needs work

The last submitted patch, user-1946466-31.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.16 KB
new24.54 KB
FAILED: [[SimpleTest]]: [MySQL] 57,761 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Some updates.

Status:Needs review» Needs work

The last submitted patch, user-1946466-33.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new896 bytes
new24.54 KB
FAILED: [[SimpleTest]]: [MySQL] 55,949 pass(es), 33 fail(s), and 15 exception(s).
[ View ]

Ughh.

Status:Needs review» Needs work
Issue tags:-FormInterface, -WSCCI-conversion

The last submitted patch, user-1946466-35.patch, failed testing.

Status:Needs work» Needs review

#35: user-1946466-35.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+FormInterface, +WSCCI-conversion

The last submitted patch, user-1946466-35.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new6.64 KB
new26.52 KB
FAILED: [[SimpleTest]]: [MySQL] 56,817 pass(es), 5 fail(s), and 3 exception(s).
[ View ]

Reroll, injected more.

Status:Needs review» Needs work

The last submitted patch, user-1946466-40.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new27.29 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user-1946466-42.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new783 bytes

#42: user-1946466-42.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+FormInterface, +WSCCI-conversion

The last submitted patch, user-1946466-42.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new27.31 KB
PASSED: [[SimpleTest]]: [MySQL] 57,104 pass(es).
[ View ]

uid / id() changes

Status:Needs review» Needs work

I tested it on simpletest.me. When I tried to delete a user from user edit page it has some issue sometimes it takes me back to admin/people without out doing anything and sometimes it takes me to confirm from.

Status:Needs work» Needs review

What do you mean "without doing anything"?
simplytest.me doesn't have outgoing emails enabled, which is the focus of a majority of this functionality. It works find during real testing.

"without doing anything" mean it doesn't block or delete the user it just redirects me back to admin/people.

Status:Needs review» Reviewed & tested by the community

#46 is due to some existing bug in HEAD. Patch is working fine both for single and multiple users.
Thanks @tim.plunkett for helping me figuring this out and for being patient with my review.

PS: I blame the timing of my review 4:27am. :(

Status:Reviewed & tested by the community» Needs work

Needs a reroll...

git ac https://drupal.org/files/user-1946466-45.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 27967  100 27967    0     0  17647      0  0:00:01  0:00:01 --:--:-- 19862
error: patch failed: core/lib/Drupal/Core/Database/Driver/mysql/Connection.php:114
error: core/lib/Drupal/Core/Database/Driver/mysql/Connection.php: patch does not apply
error: patch failed: core/modules/user/user.module:1770
error: core/modules/user/user.module: patch does not apply

Assigned:tim.plunkett» Unitoch

I'm going to try rerolling this patch.

Assigned:Unitoch» Unassigned
Status:Needs work» Needs review
StatusFileSize
new26.53 KB
PASSED: [[SimpleTest]]: [MySQL] 57,312 pass(es).
[ View ]

Here's a rerolled version of #45 above.

Status:Needs review» Reviewed & tested by the community

Back to RTBC

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/user/lib/Drupal/user/Form/UserCancelForm.phpundefined
@@ -0,0 +1,174 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function getQuestion() {
+    global $user;
+    if ($this->entity->id() == $user->id()) {
+      return t('Are you sure you want to cancel your account?');
+    }
+    return t('Are you sure you want to cancel the account %name?', array('%name' => $this->entity->label()));

Why isn't the translation service injected? Declaring global $user in several methods isn't great either - could that be a class property at least - the only thing needed is the uid.

+++ b/core/modules/user/lib/Drupal/user/Form/UserCancelForm.phpundefined
@@ -0,0 +1,174 @@
+    // @todo move to proper place
+    form_load_include($form_state, 'inc', 'user', 'user.pages');
+    $this->cancelMethods = user_cancel_methods();

Could we not resolve that here?

+++ b/core/modules/user/lib/Drupal/user/Form/UserCancelForm.phpundefined
@@ -0,0 +1,174 @@
+    // Cancel account immediately, if the current user has administrative
+    // privileges, no confirmation mail shall be sent, and the user does not
+    // attempt to cancel the own account.
+    if (user_access('administer users') && empty($form_state['values']['user_cancel_confirm']) && $this->entity->id() != $user->id()) {
+      user_cancel($form_state['values'], $this->entity->id(), $form_state['values']['user_cancel_method']);

the own account, should be 'their own account?

Issue tags:+Stalking Crell

With this patch, user_cancel_methods() is called from three places:

  • \Drupal\user\Form\UserMultipleCancelConfirm
  • \Drupal\user\AccountSettingsForm
  • \Drupal\user\Form\UserCancelForm

Those three do not (and cannot) share a common parent, so where should user_cancel_methods() live?

Status:Needs work» Needs review
StatusFileSize
new11.7 KB
new32.07 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user-1946466-56.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

So I've continued to use t().

Fixed the rest. I'll open a follow-up for dealing with user_cancel_methods() directly.

EDIT: #2049579: Replace user_cancel_methods() with something OO

Status:Needs review» Needs work

The last submitted patch, user-1946466-56.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new32.11 KB
PASSED: [[SimpleTest]]: [MySQL] 57,507 pass(es).
[ View ]

Status:Needs review» Needs work

+++ b/core/modules/user/lib/Drupal/user/Form/UserMultipleCancelConfirm.phpundefined
@@ -0,0 +1,220 @@
+    $accounts = $this->tempStoreFactory->get('user_user_operations_cancel')->get($GLOBALS['user']->id());

We can get $account from $this->request

+++ b/core/modules/user/lib/Drupal/user/Form/UserMultipleCancelConfirm.phpundefined
@@ -0,0 +1,220 @@
+    $form['accounts'] = array('#prefix' => '<ul>', '#suffix' => '</ul>', '#tree' => TRUE);
+    foreach ($accounts as $uid => $account) {
+      // Prevent user 1 from being canceled.
+      if ($uid <= 1) {
+        continue;
+      }
+      $form['accounts'][$uid] = array(
+        '#type' => 'hidden',
+        '#value' => $uid,
+        '#prefix' => '<li>',
+        '#suffix' => String::checkPlain($account->label()) . "</li>\n",
+      );

I wish @sun can review this. This is ugly this should use theme_item_list.

Status:Needs work» Needs review
StatusFileSize
new939 bytes
new32.15 KB
PASSED: [[SimpleTest]]: [MySQL] 57,471 pass(es).
[ View ]

Fixed the first part.

The second one is more complex than theme_item_list, the '#type' => 'hidden', does a bit more.
That's very much out of scope.

Status:Needs review» Reviewed & tested by the community

#54 and #59 is addressed. Created #2049921: UserMultipleCancelConfirm::buildForm() using '#prefix' and '#suffix' with '#type' => 'hidden' for second point in #59. Back to RTBC.

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/user/lib/Drupal/user/Form/UserCancelForm.phpundefined
@@ -0,0 +1,176 @@
+    $this->userId = $request->attributes->get('account')->id();
+++ b/core/modules/user/lib/Drupal/user/Form/UserMultipleCancelConfirm.phpundefined
@@ -0,0 +1,222 @@
+    $accounts = $this->tempStoreFactory
+      ->get('user_user_operations_cancel')
+      ->get($request->attributes->get('account')->id());
...
+    $current_user_id = $this->request->attributes->get('account')->id();

'account' has changed to '_account' see #2043781: Drupal::request()->attributes->get('account') may conflict with an account object loaded from the path

Status:Needs work» Reviewed & tested by the community
Issue tags:-Stalking Crell
StatusFileSize
new2 KB
new32.15 KB
PASSED: [[SimpleTest]]: [MySQL] 57,630 pass(es).
[ View ]

Just a reroll for #62. No other changes.

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/user/lib/Drupal/user/Form/UserCancelForm.phpundefined
@@ -0,0 +1,176 @@
+    $admin_access = user_access('administer users');
...
+      '#access' => $admin_access || user_access('select account cancellation method'),
...
+    if (user_access('administer users') && empty($form_state['values']['user_cancel_confirm']) && $this->entity->id() != $this->userId) {

Shouldn't be using user_access here - it's been deprecated.

Status:Needs work» Needs review
StatusFileSize
new6.34 KB
new32.43 KB
FAILED: [[SimpleTest]]: [MySQL] 57,599 pass(es), 9 fail(s), and 0 exception(s).
[ View ]

Killed deprecated things. This didn't pass 100% for me locally, but lets see what the bot says.

Status:Needs review» Needs work

The last submitted patch, user-1946466-65.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1014 bytes
new32.46 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user-1946466-67.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

_user_mail_notify was converted to use NG users now.

Assigned:Unassigned» becw
Status:Needs review» Needs work
Issue tags:+Needs tests

+++ b/core/modules/user/user.module
@@ -882,14 +882,6 @@ function user_menu() {
-  $items['admin/people/cancel'] = array(
-    'title' => 'Cancel user',
-    'page callback' => 'drupal_get_form',
-    'page arguments' => array('user_multiple_cancel_confirm'),
-    'access arguments' => array('administer users'),
-    'file' => 'user.admin.inc',
-    'type' => MENU_CALLBACK,
-  );

Should this hook_menu entry be deleted entirely?

Also, when I went to test the multiple-user-cancel confirm form manually, I got a PHP notice and no form:

Notice: Undefined index: user_multiple_cancel_confirm in drupal_retrieve_form() (line 825 of core/includes/form.inc).

I suspect that we should have a test for admin/people/cancel. I'm going to try to fix this up this afternoon.

All of the type MENU_CALLBACK were only in hook_menu for routing and not menu links, so yes, they can be deleted outright.

If I just apply the patch and try to test, I also get that error. But clearing the cache, I don't anymore.

Status:Needs work» Needs review
Issue tags:-Needs tests

Ah, you're right, my bad.

+++ b/core/modules/user/lib/Drupal/user/Form/UserCancelForm.php
@@ -0,0 +1,183 @@
+      drupal_set_message(t('A confirmation request to cancel your account has been sent to your e-mail address.'));
+++ b/core/modules/user/lib/Drupal/user/Form/UserMultipleCancelConfirm.php
@@ -0,0 +1,222 @@
+      drupal_set_message($message, $redirect ? 'error' : 'warning');

drupal_set_message() probably shouldn't be used inside this class, if possible. Is there a service for this yet?

I'm guessing/hoping that updating user_cancel_methods() is out-of-scope for this issue.

Assigned:becw» Unassigned

drupal_set_message has no equivalent yet.

user_cancel_methods is #2049579: Replace user_cancel_methods() with something OO

#67: user-1946466-67.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+FormInterface, +WSCCI-conversion

The last submitted patch, user-1946466-67.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new32.41 KB
PASSED: [[SimpleTest]]: [MySQL] 57,933 pass(es).
[ View ]

Straight reroll.

I get Notice: Array to string conversion in form_process_checkbox() (line 3257 of core/includes/form.inc). on the create user page.

I don't think it's related to this patch though, I reverted and cleared the cache and the notice was still there #2069929: Notice: Array to string conversion in form_process_checkbox() (line 3257 of core/includes/form.inc). on create user page.

I was looking over this tonight, ran out of time to properly RTBC or not, the forms do seem to be working though, which is good :)

Status:Needs review» Reviewed & tested by the community

Thanks @thedavidmeister for the review. I am going to RTBC it once more after #49, #53 and #61. I think I can RTBC it because I only rerolled it.

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/user/lib/Drupal/user/Form/UserCancelForm.phpundefined
@@ -0,0 +1,183 @@
+      return t('Are you sure you want to cancel your account?');
...
+    return t('Are you sure you want to cancel the account %name?', array('%name' => $this->entity->label()));
...
+      $description = t('Select the method to cancel the account above.');
...
+    return t('Cancel account');
...
+      '#title' => t('Require e-mail confirmation to cancel account.'),
...
+      '#description' => t('When enabled, the user must confirm the account cancellation via e-mail.'),
...
+      '#title' => t('Notify user when account is canceled.'),
...
+      '#description' => t('When enabled, the user will receive an e-mail notification after the account has been canceled.'),
+++ b/core/modules/user/lib/Drupal/user/Form/UserMultipleCancelConfirm.phpundefined
@@ -0,0 +1,222 @@
+    return t('Are you sure you want to cancel these user accounts?');
...
+    return t('Cancel accounts');
...
+      $message = t('The user account %name cannot be canceled.', array('%name' => $accounts[1]->label()));
...
+      '#title' => t('When cancelling these accounts'),
...
+      '#title' => t('Require e-mail confirmation to cancel account.'),
+      '#default_value' => FALSE,
+      '#description' => t('When enabled, the user must confirm the account cancellation via e-mail.'),
...
+      '#title' => t('Notify user when account is canceled.'),
...
+      '#description' => t('When enabled, the user will receive an e-mail notification after the account has been canceled.'),

use $this->t() from FormBase

+++ b/core/modules/user/lib/Drupal/user/Form/UserCancelForm.phpundefined
@@ -0,0 +1,183 @@
+  public function buildForm(array $form, array &$form_state, Request $request = NULL) {

No need to pass the request in anymore

+++ b/core/modules/user/lib/Drupal/user/Form/UserCancelForm.phpundefined
@@ -0,0 +1,183 @@
+    $this->user = $request->attributes->get('_account');

Let's use the FormBase::getCurrentUser() method

+++ b/core/modules/user/lib/Drupal/user/Form/UserMultipleCancelConfirm.phpundefined
@@ -0,0 +1,222 @@
+  public function buildForm(array $form, array &$form_state, Request $request = NULL) {
+    $this->request = $request;

We should be using FormBase's getRequest() method.

+++ b/core/modules/user/lib/Drupal/user/Form/UserMultipleCancelConfirm.phpundefined
@@ -0,0 +1,222 @@
+      ->get($request->attributes->get('_account')->id());

Let's use FormBase::getCurrentUser()

+++ b/core/modules/user/lib/Drupal/user/Form/UserMultipleCancelConfirm.phpundefined
@@ -0,0 +1,222 @@
+      return new RedirectResponse(url('admin/people', array('absolute' => TRUE)));
...
+        return new RedirectResponse(url('admin/people', array('absolute' => TRUE)));

Should be inject the url generator service

+++ b/core/modules/user/user.moduleundefined
@@ -928,11 +920,7 @@ function user_menu() {
   $items['user/%user/cancel'] = array(
     'title' => 'Cancel account',
-    'page callback' => 'drupal_get_form',
-    'page arguments' => array('user_cancel_confirm_form', 1),
-    'access callback' => 'entity_page_access',
-    'access arguments' => array(1, 'delete'),
-    'file' => 'user.pages.inc',
+    'route_name' => 'user_cancel_confirm',

We can use the _title property on the route definition and remove this completely.

Status:Needs work» Needs review
Issue tags:+FormBase
StatusFileSize
new14.33 KB
new32.44 KB
FAILED: [[SimpleTest]]: [MySQL] 58,286 pass(es), 48 fail(s), and 26 exception(s).
[ View ]

Fixed #79.

Status:Needs review» Needs work

The last submitted patch, 1946466-80.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.9 KB
new32.26 KB
PASSED: [[SimpleTest]]: [MySQL] 58,432 pass(es).
[ View ]

Fixed Tests.

StatusFileSize
new2.93 KB
new32.24 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1946466-83.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Also updated UserMultipleCancelConfirm.

#83: 1946466-83.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+FormInterface, +WSCCI-conversion, +FormBase

The last submitted patch, 1946466-83.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new32.25 KB
PASSED: [[SimpleTest]]: [MySQL] 58,126 pass(es).
[ View ]

3-way merge :D

StatusFileSize
new1.36 KB
new32.24 KB
PASSED: [[SimpleTest]]: [MySQL] 58,803 pass(es).
[ View ]

Reroll.

Status:Needs review» Needs work
  1. +++ b/core/modules/user/lib/Drupal/user/Form/UserCancelForm.php
    @@ -0,0 +1,175 @@
    +  /**
    +   * The current user.
    +   *
    +   * @var \Drupal\Core\Session\AccountInterface
    +   */
    +  protected $user;

    Let's also maybe wait until FormBase gets a currentUser method.

  2. +++ b/core/modules/user/lib/Drupal/user/Form/UserCancelForm.php
    @@ -0,0 +1,175 @@
    +  public function getCancelPath() {
    +    return 'user/' . $this->entity->id();
    +  }

    What about using $entity->uri()?

  3. +++ b/core/modules/user/lib/Drupal/user/Form/UserCancelForm.php
    @@ -0,0 +1,175 @@
    +      '#default_value' => ($override_access ? FALSE : TRUE),

    You could just use !$override_access

  4. +++ b/core/modules/user/lib/Drupal/user/Form/UserCancelForm.php
    @@ -0,0 +1,175 @@
    +    return parent::buildForm($form, $form_state);

    Is there a specific reason to not call parent::buildForm at the top of the parent function?

  5. +++ b/core/modules/user/lib/Drupal/user/Form/UserCancelForm.php
    @@ -0,0 +1,175 @@
    +      $this->entity->user_cancel_method = $form_state['values']['user_cancel_method'];
    +      $this->entity->user_cancel_notify = $form_state['values']['user_cancel_notify'];

    On the longrun we should store that information somewhere else, maybe in a tempstore?

  6. +++ b/core/modules/user/lib/Drupal/user/Form/UserMultipleCancelConfirm.php
    @@ -0,0 +1,224 @@
    +      $container->get('plugin.manager.entity')->getStorageController('user'),
    +      $container->get('plugin.manager.entity'),

    Let's use entity.manager now.

  7. +++ b/core/modules/user/lib/Drupal/user/Form/UserMultipleCancelConfirm.php
    @@ -0,0 +1,224 @@
    +      return new RedirectResponse($this->urlGenerator->generateFromPath('admin/people', array('absolute' => TRUE)));

    There will be a patch relative soon which provides a $this- >redirect() method, so we maybe we see that one before this one is ready.

  8. +++ b/core/modules/user/user.module
    @@ -874,14 +874,6 @@ function user_menu() {
    -  $items['admin/people/cancel'] = array(
    -    'title' => 'Cancel user',
    +++ b/core/modules/user/user.routing.yml
    @@ -61,6 +61,13 @@ user_admin_permission:
    +user_multiple_cancel_confirm:
    +  pattern: '/admin/people/cancel'
    +  defaults:
    +    _form: '\Drupal\user\Form\UserMultipleCancelConfirm'
    +  requirements:
    +    _permission: 'administer users'
    +
    user_role_list:

    Missing _title key

Status:Needs work» Needs review
StatusFileSize
new2.22 KB
new32.26 KB
PASSED: [[SimpleTest]]: [MySQL] 58,920 pass(es).
[ View ]

Thanks @dawehner for the review.

  1. I'll reroll this when that will be in.
  2. Fixed.
  3. Fixed.
  4. parent::buildForm($form, $form_state); is adding form action element I think it is ok to call it at the end.
  5. Don't know what to do.
  6. Fixed
  7. I'll reroll this when that will be in.
  8. Fixed.

Status:Needs review» Reviewed & tested by the community

Great!

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/user/lib/Drupal/user/Form/UserCancelForm.php
@@ -0,0 +1,176 @@
+  /**
+   * The current user.
+   *
+   * @var \Drupal\Core\Session\AccountInterface
+   */
+  protected $user;
...
+    if ($this->entity->id() == $this->user->id()) {
...
+    if ($this->user->hasPermission('administer users') || $this->user->hasPermission('select account cancellation method')) {
...
+    $this->user = $this->getCurrentUser();
...
+    $admin_access = $this->user->hasPermission('administer users');
...
+      '#title' => ($this->entity->id() == $this->user->id() ? $this->t('When cancelling your account') : $this->t('When cancelling the account')),
...
+    $override_access = $admin_access && ($this->entity->id() != $this->user->id());
...
+    if ($this->user->hasPermission('administer users') && empty($form_state['values']['user_cancel_confirm']) && $this->entity->id() != $this->user->id()) {

Lets use $this->getCurrentUser() and not store $user on the object. We're creating an unnecessary dependency on buildForm() in all the other functions that use $this->user

Status:Needs work» Needs review
StatusFileSize
new3.95 KB
new32.16 KB
FAILED: [[SimpleTest]]: [MySQL] 58,585 pass(es), 48 fail(s), and 8 exception(s).
[ View ]

Addressing #91.

Status:Needs review» Needs work
Issue tags:-FormInterface, -WSCCI-conversion, -FormBase

The last submitted patch, drupal8.user-module.1946466-92.patch, failed testing.

Status:Needs work» Needs review

#92: drupal8.user-module.1946466-92.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+FormInterface, +WSCCI-conversion, +FormBase

The last submitted patch, drupal8.user-module.1946466-92.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new5.63 KB
new32.06 KB
PASSED: [[SimpleTest]]: [MySQL] 59,172 pass(es).
[ View ]

Status:Needs review» Needs work
Issue tags:-FormInterface, -WSCCI-conversion, -FormBase

The last submitted patch, drupal8.user-module.1946466-96.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+FormInterface, +WSCCI-conversion, +FormBase

#96: drupal8.user-module.1946466-96.patch queued for re-testing.

+++ b/core/modules/user/lib/Drupal/user/Form/UserCancelForm.php
@@ -0,0 +1,173 @@
+ * Provides a confirmation form for cancelling user account.
+++ b/core/modules/user/lib/Drupal/user/Form/UserMultipleCancelConfirm.php
@@ -0,0 +1,216 @@
+ * Provides a confirmation form for cancelling user account.

The documentation for two different classes should be a little bit different.

StatusFileSize
new614 bytes
new32.07 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

How about "Provides a confirmation form for cancelling multiple users account." for UserMultipleCancelConfirm.

StatusFileSize
new778 bytes
new32.07 KB
PASSED: [[SimpleTest]]: [MySQL] 59,241 pass(es).
[ View ]

Fixed comment to Provides a confirmation form for cancelling multiple user accounts.

Status:Needs review» Needs work
Issue tags:-FormInterface, -WSCCI-conversion, -FormBase

The last submitted patch, drupal8.user-module.1946466-101.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+FormInterface, +WSCCI-conversion, +FormBase

Status:Needs review» Reviewed & tested by the community

The form was tested manually and the code looks fine for me, tomorrow as well as tonight

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

Yay!!! thank you everybody.

+++ b/core/modules/user/lib/Drupal/user/Form/UserMultipleCancelConfirm.php
@@ -0,0 +1,216 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function getCancelRoute() {
+  }

This part looks weird to me. ConfirmFormInterface::getCancelRoute() does not document returning NULL. What is the wanted effect here? Redirecting to the front page? In that case we should return the '' route. I'm not really sure, but I think either this should be updated or the documentation should be amended in case returning NULL is a valid thing.

+++ b/core/modules/user/lib/Drupal/user/Form/UserCancelForm.php
@@ -0,0 +1,173 @@
+    // @todo Convert to getCancelRoute() after https://drupal.org/node/1987896.
+    $uri = $this->entity->uri();
+    $form['actions']['cancel']['#href'] = $uri['path'];

Hmm... Thanks @jibran, but I'm afraid I don't really understand what that comment is supposed to tell me. Anyway, I found this @todo a little below the code, which probably explains this. A @todo on getCancelRoute() would have been nice, but meh...

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

Issue summary:View changes

Updated issue summary.