Remove access check from submit() in UserCancelForm

First solve and discuss in this issue http://drupal.org/node/687586 and then come back here to apply the same solution.

subscribing

The only place I can find that calls user_cancel_confirm_form_submit() in 8.x is user_multiple_cancel_confirm_submit(). I can't find anything that calls user_multiple_cancel_confirm_submit(). Is this some feature of drupal/php that I'm unaware of where some behind the behind the scenes mojo in user_cancel_confirm_form() calls user_cancel_confirm_form_submit() (or the equivalent for the multiple cancel)? Or are these both just functions that have been deprecated but are still used somewhere I can't find?

@emclaughlin module_formID_submit() is called dynamically when the form generated by module_formID() is submitted successfully.

That's the default submit callback (others can be defined explicitly in $form['#submit']).

I have reviewed the approach in 687586 and applied the same approach here. Let me know if anything additional is required as I see that despite a successful patch the 687586 issue was re-opened.

Revised patch attached - now checking for null response from user_access call

A little confused by why this is failing - I'm not getting the same notice when I test locally and stepping through adding debug output at each step has been successful.

I've added an extra isset check in this patch - hopefully that is ok but let me know if I'm missing something!

Looks good, doesn't add new features and don't break anything, so...

Wait!

1) Why use numbers here:

+     '#value' => ($admin_access ? 1 : 0),

And then we use a boolean for the same:

+  $admin_access = FALSE;

Should be more consistent use TRUE/FALSE in both places.

2) Instead of use this:

+  $admin_access = FALSE;
+  
+  if (isset($form_state['values']['administer_users'])) {
+    $admin_access = $form_state['values']['administer_users'];
+  }

I think it's better the compact form:

$admin_access = isset($form_state['values']['administer_users']) ? $form_state['values']['administer_users'] : FALSE;

Thanks for the feedback and suggestions. Revised patch attached.

Good point - revised patch attached.

Gah, sorry - typo on the form state values. Fixed working patch attached.

Gah, sorry - typo on the form state values. Fixed working patch attached.

A while since this has moved so re-rolled against latest D8 build, apply cleanly so seeing what testbot makes of it now.

Sorry, not sure how I thought that last patch would apply... re-rolled now and re-queueing for testing.

#25: user-cancel_submit_remove-687588-25.patch queued for re-testing.

Seeing whether testbot still likes this one

Ok, did a little investigating with @esod. This needs to be rerolled for D8. Looks like this form + submit handler in D8 is now located in /core/modules/user/src/Form/UserCancelForm.php

line 125 shows that the administer user check is still inside of the submit handler:

if ($this->currentUser()->hasPermission('administer users') && empty($form_state['values']['user_cancel_confirm']) && $this->entity->id() != $this->currentUser()->id()) {
      user_cancel($form_state['values'], $this->entity->id(), $form_state['values']['user_cancel_method']);

Here's a patch @esod and I worked on for D8 to move the user access check out of submit() to $form.

Oh whoops. Forgot #value in all of the excitement. Here's a new patch.

Ok, I think I figured out by looking at UserCancelTest and running that test locally.

As i think, in buildForm() method, we have

$user = $this->currentUser();

so why not just use
$user->hasPermission('administer users');

instead of

$this->currentUser()->hasPermission('administer users');

So resubmitted patch.

Trying to reroll

1. +++ b/core/modules/user/src/Form/UserCancelForm.php
   @@ -110,6 +110,12 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    // Store the user permissions so that it can be altered in hook_form_alter() if desired.
   

   This comment goes over the 80-character line-wrap.

2. +++ b/core/modules/user/src/Form/UserCancelForm.php
   @@ -110,6 +110,12 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    ¶
   

   Trailing whitespace here.

Thanks.

This looks good to me. I've updated the issue summary with a beta phase evaluation.

Back to RTBC.

Actually this could use a small test.

We are working on the test during sprint weekend in London Canada!

Here is a test, adding a proposed commit message to the issue summary because I had a lot of help from other sprinters.

The patches should fail and pass, respectively.

Also this could potentially be backported I'd think, not sure why it was removed around #31.

+1 for the new test! I think this is back to RTBC now.

Thanks @jhedstrom!

Looks like there was just a small context change. Rerolled.

Back to RTBC assuming bot goes green.

And again...

Committed 20a76f0 and pushed to 8.0.x. Thanks!

Thank you for adding the beta evaluation to the summary.