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

andypost’s picture

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

andypost’s picture

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.

andypost’s picture

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.

andypost’s picture

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

andypost’s picture

Issue summary:View changes

Updated issue summary.

andypost’s picture

Status:Needs review» Postponed
andypost’s picture

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

andypost’s picture

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.

Anonymous’s picture

Issue summary:View changes

Updated issue summary.

tim.plunkett’s picture

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

andypost’s picture

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

andypost’s picture

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

andypost’s picture

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.

jibran’s picture

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.

andypost’s picture

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.

andypost’s picture

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

andypost’s picture

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

tim.plunkett’s picture

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

andypost’s picture

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

tim.plunkett’s picture

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

Working on this.

tim.plunkett’s picture

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.

tim.plunkett’s picture

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

BC NG OMG

tim.plunkett’s picture

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

andypost’s picture

Status:Needs review» Reviewed & tested by the community

Should be ok

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
tim.plunkett’s picture

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.

tim.plunkett’s picture

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.

tim.plunkett’s picture

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.

tim.plunkett’s picture

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

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

tim.plunkett’s picture

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.

tim.plunkett’s picture

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.

tim.plunkett’s picture

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
jibran’s picture

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

tim.plunkett’s picture

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

uid / id() changes

jibran’s picture

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.

tim.plunkett’s picture

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.

jibran’s picture

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

jibran’s picture

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. :(

alexpott’s picture

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
Unitoch’s picture

Assigned:tim.plunkett» Unitoch

I'm going to try rerolling this patch.

Unitoch’s picture

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.

jibran’s picture

Status:Needs review» Reviewed & tested by the community

Back to RTBC

catch’s picture

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?

tim.plunkett’s picture

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?

tim.plunkett’s picture

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.

tim.plunkett’s picture

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

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.

tim.plunkett’s picture

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.

jibran’s picture

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.

alexpott’s picture

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

Crell’s picture

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.

alexpott’s picture

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.

tim.plunkett’s picture

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.

tim.plunkett’s picture

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.

becw’s picture

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.

tim.plunkett’s picture

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.

becw’s picture

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.

becw’s picture

Assigned:becw» Unassigned
tim.plunkett’s picture

drupal_set_message has no equivalent yet.

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

jibran’s picture

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

jibran’s picture

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

Straight reroll.

thedavidmeister’s picture

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.

thedavidmeister’s picture

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

jibran’s picture

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.

alexpott’s picture

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.

jibran’s picture

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.

jibran’s picture

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

Fixed Tests.

jibran’s picture

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.

tim.plunkett’s picture

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

jibran’s picture

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

3-way merge :D

jibran’s picture

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

Reroll.

dawehner’s picture

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

jibran’s picture

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

Status:Needs review» Reviewed & tested by the community

Great!

alexpott’s picture

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

disasm’s picture

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.

jibran’s picture

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.

jibran’s picture

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.

jibran’s picture

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

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

dawehner’s picture

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

jibran’s picture

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.

jibran’s picture

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.

dawehner’s picture

Status:Needs work» Needs review
Issue tags:+FormInterface, +WSCCI-conversion, +FormBase
dawehner’s picture

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

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

jibran’s picture

Yay!!! thank you everybody.

tstoeckler’s picture

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

jibran’s picture

tstoeckler’s picture

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

Anonymous’s picture

Issue summary:View changes

Updated issue summary.