As reported at #3257592: permission/option that an administrator can create/update/delete users although domain is blocked, server-side validation of invalid user accounts is preventing editors from reaching the cancel account form via the 'Cancel account' button at the bottom of the user profile form.
There is no reason to run validation on the form when using an action which is effectively a link to elsewhere, with no form processing occuring.
Expected behaviour: Browser loads the cancel account form
Current behaviour: User profile form reloads with validation error on email field
There are several ways we could tackle this:
$element['delete']['#limit_validation_errors'] = []; to the cancel button in \Drupal\user\ProfileForm.None
None
None
N/A
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | 3258321-2.patch | 606 bytes | darvanen |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
darvanenFor anyone who wants a quick fix for this right now, here's a patch using solution 1 from the IS.
Comment #3
mstrelan commentedAn easy way to reproduce the issue in core:
Comment #4
darvanenOh duh, of course, updated IS.
Comment #5
dww@darvanen Asked for opinions on this via Slack. Most other entities in core (citation needed 🤓😉) have a Delete link, not a button, for exactly this reason. I think that'd be the cleanest, best, most consistent solution here. So +1 to proposed solution 3, even though it's a (slightly?) bigger change.
Comment #6
aaronmchaleWe looked at this issue during #3257468: Drupal Usability Meeting 2022-01-14.
There was no objections to changing the button to a link, and making this consistent with other core entity types. It was also highlighted that this is potentially an accessibility issue, so tagging for accessiblity review.
During the meeting we also checked Node, and the "Delete" action is indeed a normal link.
Comment #7
mgiffordCan someone provide a snapshot here as to the change? Is there before/after for HTML? Demo environment?
Buttons do things, Links go somewhere. Is it a button or a link?
When you delete a node, you generally leave the page (because it has been deleted).
Anyways, need more information.
Comment #8
darvanen@mgifford the change hasn't been written yet but I'm sure that can be provided when it is.
As it stands, the "cancel account" *button* has a single purpose: take the user to the cancel account form. I believe it's for that reason it should be a link instead, as you said, links go somewhere.
Self-assigning to have a crack at making the change.
Comment #10
darvanenAlright, the MR has a link replacement which worked locally but there are a few things to discuss:
First, this is the access check that was already on the button:
'#access' => $this->entity->id() > 1 && $this->entity->access('delete'),why is that check for user 1 there? It's an unrelated change, kind of, but if we're touching this code I'd prefer to remove it.
Secondly, and this may end up making the first point moot: if we change the link template name from 'cancel-form' to 'delete-form', then the creation of the delete button will be entirely handled by \Drupal\Core\Entity\EntityForm::actions further up the chain.
Thoughts?
Comment #11
darvanenIt should also be noted that without addressing #2243575: Add #button_type support for #type link this change will result in visual changes to the button/link in some (most?) themes:
Comment #12
mstrelan commentedCan't delete the user 1 account so don't show the button. This should be abstracted in something like
$entity->canDelete()or something more meaningful.Comment #13
darvanenAh right, I'd prefer to see that on \Drupal\user\UserAccessControlHandler::checkAccess but that's now a completely unrelated change worth a separate issue.
I don't see how to expand it to make any more sense without it being rather verbose nested if statements (yuck). Would a comment suffice?
Comment #14
aaronmchaleLet's not touch the access check, keep the scope of the issue well defined. This will likely be removed anyway as part of #540008: Add a container parameter that can remove the special behavior of UID#1. I have added a comment to that effect #540008-312: Add a container parameter that can remove the special behavior of UID#1.
Comment #15
darvanenAny thoughts about this alternative approach?
Comment #16
aaronmchaleI would agree that is a good idea, although I think we should try to keep the scope of this issue well defined. For instance, if in this issue we change it from a button to a link, that aligns the UI/UX with
\Drupal\Core\Entity\EntityForm::actions, then making it easier to make that change in a follow-up issue. As both tasks have very different considerations and scopes.Comment #17
darvanenComment #18
aaronmchaleTested the MR and everything works as expected, changes in MR look good, nice touch on the destination parameter.
I was going to RTBC this, but setting it back to NW as we need to find or create a follow-up issue for exploring whether
\Drupal\Core\Entity\EntityForm::actionscan be used.Thanks,
-Aaron
Comment #19
darvanenThanks but that's pulled directly from
\Drupal\Core\Entity\EntityForm::actionsso I can't take credit ;)The only changes I made were to use access check with
uid > 1and to change the link template name fromdelete-formtocancel-form.I just found #2250377: ProfileForm::actions() defines new delete element instead of altering it as it expects that covers the follow-up.
Comment #20
aaronmchaleGreat, yeah we can reuse #2250377: ProfileForm::actions() defines new delete element instead of altering it as it expects for the next step, it'll just need to be updated/rescoped to clearly show what the next steps are, but the main next step will be changing cancel-form to delete-form is basically what needs done next. Once that's done we'll be in a good place to strip down
ProfileForm::actions().Moving this to RTBC.
Note regarding the "needs accessibility review" tag, which is still present: this was added as a suggestion by @xjm during the UX meeting because it was felt that if this is an improvement from an accessibility perspective then it's easier to get it committed, even if the change is slightly disruptive.
Comment #21
lauriiiCan we have a change record for this change since this could impact themes and modules.
The change in Claro described in #11 is correct - the styles there are Claro styles for links with button class and this change will make this consistent with other entity forms.
Comment #22
darvanenTechnically this change will make it *more* consistent with other entity forms that already use the link template, but yes, a CR is a fair request. I'll do it tomorrow (late here)
Comment #23
darvanenTook a stab at a draft CR: #3266437: User form cancel button element changed to link element.
Comment #24
dwwReviewed the CR. Looks great, thanks! Very clear about what the impact of this change might be and how to address it. Removing that tag.
Comment #25
darvanenOops, guess that kind of notation doesn't work with non-issue nodes. Here's a link to the CR, IS updated too.
User form cancel button element changed to link element.
Comment #26
dwwWhich subsystem maintainer needs to review this? Was that intentional? Also, is "user system" or "user.module" a more appropriate component than "entity system"?
Comment #27
darvanenAh yeah good question, I was tossing up whether to add that in then forgot about the tag while I was altering the links.
Short answer: I don't know. Removing it until someone decides it's needed.
Comment #34
catchComment #37
catchI think @mgifford's comment in #7 is actually sufficient here for accessibility review - the button was redirecting to go to a page instead of doing anything, which means it always should have been a link, and we were working around it not being one. Additionally we're using the delete link pattern on other entity types already and user was the odd one out. So making it consistent here matches the observation in #7.
Adding credit for usability meeting attendees too.
Committed/pushed to 10.0.x and cherry-picked to 9.4.x, thanks!
Comment #38
aaronmchaleThanks @catch, this now unblocks #2250377: ProfileForm::actions() defines new delete element instead of altering it as it expects if anyone has the time to work on it.
Comment #39
darvanenThanks @catch!