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

CommentFileSizeAuthor
#101 drupal8.user-module.1946466-101.patch32.07 KBjibran
#101 interdiff.txt778 bytesjibran
#100 drupal8.user-module.1946466-100.patch32.07 KBjibran
#100 interdiff.txt614 bytesjibran
#96 drupal8.user-module.1946466-96.patch32.06 KBjibran
#96 interdiff.txt5.63 KBjibran
#92 drupal8.user-module.1946466-92.patch32.16 KBdisasm
#92 interdiff.txt3.95 KBdisasm
#89 drupal8.user-module.1946466-89.patch32.26 KBjibran
#89 interdiff.txt2.22 KBjibran
#87 drupal8.user-module.1946466-87.patch32.24 KBjibran
#87 diff.txt1.36 KBjibran
#86 1946466-86.patch32.25 KBjibran
#83 1946466-83.patch32.24 KBjibran
#83 interdiff.txt2.93 KBjibran
#82 1946466-82.patch32.26 KBjibran
#82 interdiff.txt2.9 KBjibran
#80 1946466-80.patch32.44 KBjibran
#80 interdiff.txt14.33 KBjibran
#75 1946466-75.patch32.41 KBjibran
#67 user-1946466-67.patch32.46 KBtim.plunkett
#67 interdiff.txt1014 bytestim.plunkett
#65 user-1946466-65.patch32.43 KBtim.plunkett
#65 interdiff.txt6.34 KBtim.plunkett
#63 1946466-confirm_form_user.patch32.15 KBCrell
#63 interdiff.txt2 KBCrell
#60 user-1946466-60.patch32.15 KBtim.plunkett
#60 interdiff.txt939 bytestim.plunkett
#58 user-1946466-58.patch32.11 KBtim.plunkett
#56 user-1946466-56.patch32.07 KBtim.plunkett
#56 interdiff.txt11.7 KBtim.plunkett
#52 confirm-form.patch26.53 KBUnitoch
#45 user-1946466-45.patch27.31 KBtim.plunkett
#42 interdiff.txt783 bytestim.plunkett
#42 user-1946466-42.patch27.29 KBtim.plunkett
#40 user-1946466-40.patch26.52 KBtim.plunkett
#40 interdiff.txt6.64 KBtim.plunkett
#35 user-1946466-35.patch24.54 KBtim.plunkett
#35 interdiff.txt896 bytestim.plunkett
#33 user-1946466-33.patch24.54 KBtim.plunkett
#33 interdiff.txt2.16 KBtim.plunkett
#31 interdiff.txt4.8 KBtim.plunkett
#31 user-1946466-31.patch23.68 KBtim.plunkett
#27 user-1946466-27.patch23.14 KBtim.plunkett
#27 interdiff.txt1.27 KBtim.plunkett
#25 user-1946466-25.patch23.12 KBtim.plunkett
#21 interdiff.txt1.71 KBandypost
#21 1946466-confirm-user-21.patch21.66 KBandypost
#20 interdiff.txt1.15 KBandypost
#20 1946466-confirm-user-20.patch21.48 KBandypost
#18 1946466-confirm-user-18.patch22.22 KBandypost
#14 1946466-confirm-user-14.patch27.85 KBandypost
#9 interdiff.txt14.46 KBandypost
#9 1946466-confirm-9.patch30.44 KBandypost
#8 1946466-confirm-8.patch9.03 KBandypost
#6 interdiff.txt921 bytesandypost
#6 1946466-confirm-6.patch9.3 KBandypost
#4 interdiff.txt1.89 KBandypost
#4 1946466-confirm-4.patch9.29 KBandypost
#2 1946466-confirm-1.patch8.65 KBandypost
#1 1946466-confirm-1.patch8.66 KBandypost
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

Status: Active » Needs review
FileSize
8.66 KB

initial patch

andypost’s picture

FileSize
8.65 KB

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
FileSize
9.29 KB
1.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
FileSize
9.3 KB
921 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
FileSize
9.03 KB

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

FileSize
30.44 KB
14.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
FileSize
27.85 KB

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
FileSize
22.22 KB

Status: Needs review » Needs work

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

andypost’s picture

Status: Needs work » Needs review
FileSize
21.48 KB
1.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

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
FileSize
23.12 KB

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
FileSize
1.27 KB
23.14 KB

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
FileSize
23.68 KB
4.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
FileSize
2.16 KB
24.54 KB

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
FileSize
896 bytes
24.54 KB

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
FileSize
6.64 KB
26.52 KB

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
FileSize
27.29 KB
783 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
FileSize
27.31 KB

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
FileSize
26.53 KB

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
FileSize
11.7 KB
32.07 KB

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
FileSize
32.11 KB
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
FileSize
939 bytes
32.15 KB

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: Update the markup in UserMultipleCancelConfirm::buildForm() 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
FileSize
2 KB
32.15 KB

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
FileSize
6.34 KB
32.43 KB

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
FileSize
1014 bytes
32.46 KB

_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
FileSize
32.41 KB

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
FileSize
14.33 KB
32.44 KB

Fixed #79.

Status: Needs review » Needs work

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

jibran’s picture

Status: Needs work » Needs review
FileSize
2.9 KB
32.26 KB

Fixed Tests.

jibran’s picture

FileSize
2.93 KB
32.24 KB

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
FileSize
32.25 KB

3-way merge :D

jibran’s picture

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
FileSize
2.22 KB
32.26 KB

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
FileSize
3.95 KB
32.16 KB

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

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

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

jibran’s picture

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.