Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
User entitiess do not have a delete form, they have a cancel form. This implies the action one takes with a User entity is 'cancel' rather than 'delete'.
The access handler currently responds to 'delete' actions, not 'cancel' actions.
Proposed resolution
Deprecate the 'delete' access operation in favour of 'cancel'.
Remaining tasks
Decide next steps per #39
User interface changes
None
API changes
Change to User access operations.
Comment | File | Size | Author |
---|---|---|---|
#33 | 2250377-33.patch | 2.79 KB | darvanen |
Comments
Comment #1
BerdirComment #2
dawehnerI am not sure whether it is semantically a good idea to replace the canceling with deleting, given that the cancel operation does more than a simple delete.
Comment #3
ParisLiakos CreditAttribution: ParisLiakos commentedi tend to agree with dawehner. Maybe we should keep the overriden actions() method and just switch it to a link instead of submit, pointing to the cancel template route
Comment #4
BerdirYeah, the alternative fix would then be to copy the actions() delete implementation and define the link based on the cancel form template.
Comment #5
sunThe button on the edit form should definitely use #type 'link' and not #type 'submit'.
However, since these link templates are "global" and also exposed via REST, this requires some more considerations. Technically,
The original plan of #8: Let users cancel their accounts was to remove the "delete" operation entirely for users and only have a "cancel" operation for them.
However, that didn't work so well for API-level operations that really just want to delete an account (e.g. Drush), so #705306: user_cancel_delete method calls into a "standard" user_delete_multiple API re-introduced a parallel "delete" operation, but without a corresponding UI.
I don't know the answer to this problem space, but I hope that this background information is useful.
Comment #6
ParisLiakos CreditAttribution: ParisLiakos commentedthis is probably enough?
Comment #7
BerdirThis will fail as there are like a dozen button submits in UserCancelTest, DblogTest and possibly others, but yes that's the way to go then :)
Comment #19
darvanenFlagging this as a follow-up for #3258321: Cancel account button on user form triggers server-side validation.
Comment #20
AaronMcHalePostponing this on #3258321: Cancel account button on user form triggers server-side validation.. Once that is committed, this issue will need updated/rescoped.
Comment #21
AaronMcHale#3258321: Cancel account button on user form triggers server-side validation. has been committed, unpostponing, this issue will also need an update.
Comment #23
darvanenWell, here's a patch that changes the user cancel form to a user delete form but having made it I now understand #5 more fully. Semantically it is not a delete form, it's a cancel form.
Personally I think it's better off left alone and maybe apply some credit to #3258321: Cancel account button on user form triggers server-side validation. for work that was done here as patch #6 is basically what happened there?
Comment #25
darvanenI was hoping that failure was some weirdness on my local machine. I can't for the life of me figure out why it's failing.
Comment #26
darvanenAsked in the #bugsmash slack channel and @acbramley agrees that:
Perhaps if the point is to make things consistent we should make the references to user cancellation more consistent like in this patch?
If we did change the permission it will require an update hook implementation and change record, deprecation etc. I'm just putting this to Needs Review for feedback on whether this is a desirable thing to do, or if maybe we should just close this.
Comment #28
darvanenComment #29
darvanenComment #30
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
See [CR TBC]'
Does this need a change record?
Comment #31
darvanenWhy yes, it does. Here's a draft for review:
https://www.drupal.org/node/3346438
I was waiting to see if there was any feedback on the approach but I guess it's not going to get eyes if it's incomplete.
Comment #32
smustgrave CreditAttribution: smustgrave at Mobomo commentedChange looks good. Even though it's a task wonder if we could add a simple test that if we use "delete" we get the trigger_error correctly.
Comment #33
darvanenYep, good point, this also meant I discovered I hadn't set the error type, so that's done now. Test behaves properly on local so here goes nothing.
Comment #34
darvanenOops, crafted the test patch badly, here's what it should have been.
Comment #37
Ajeet Tiwari CreditAttribution: Ajeet Tiwari at OpenSense Labs for DrupalFit commenteduploaded patch for 10.1.x.
Comment #39
darvanenSo, @sun was right in #5 (of course they were). API resources make this a bit of a challenge.
For example in \Drupal\jsonapi\Routing\Routes::getIndividualRoutesForResourceType line 308 we have
$individual_remove_route->setRequirement('_entity_access', "entity.delete");
which sets the route permission for *any* delete operation on an entity.
So we're faced with a few options:
Honestly I'm inclined towards the third option.
Hiding patch from #37 as it regressed functionality, did not apply, has no interdiff, and did not come with any explanation of what it was trying to achieve.
Comment #40
Ajeet Tiwari CreditAttribution: Ajeet Tiwari at OpenSense Labs for DrupalFit commentedUploading patch again for 10.1.x after resolvng the errors of custom commands.
Comment #41
smustgrave CreditAttribution: smustgrave at Mobomo commentedPer #39
Comment #42
smustgrave CreditAttribution: smustgrave at Mobomo commentedThank you for the interest @Ajeet Tiwari but it is expected to test a patch before uploading so have to remove credit for that.
Just FYI to help get the message out there.
Starting March 2023, simple rerolls, rebases, or merges will no longer receive issue credit. Only rerolls that address a merge conflict will be credited, and the merge conflict that was resolved must be documented in the text of an issue comment.
To receive credit for contributing to this issue, assist with other outstanding tasks or unaddressed feedback.
See the issue credit guidelines for more information.
Comment #43
Ajeet Tiwari CreditAttribution: Ajeet Tiwari at OpenSense Labs for DrupalFit commentedtried to fix the CCF error and to fix the error in #34.
Comment #44
_utsavsharma CreditAttribution: _utsavsharma at OpenSense Labs for DrupalFit commentedFixed CCF for #43.
Comment #45
smustgrave CreditAttribution: smustgrave at Mobomo commentedHiding #43 as there is no interdiff being provided. Also removing credit as it's expected patches to be tested before uploading. #43 appears to be adding additional changes also.
#44 carried forward those additional changes.
Please read #39 before continuing.
@darvanen would agree on option 3 also FWIW