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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Issue summary: View changes
dawehner’s picture

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

ParisLiakos’s picture

i 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

Berdir’s picture

Yeah, the alternative fix would then be to copy the actions() delete implementation and define the link based on the cancel form template.

sun’s picture

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

  1. The delete operation still exists, but only at the API level. It essentially short-circuits the cancel process to choose the "delete account and all content" method.
  2. There is only a form for the cancel operation.
  3. There is no form for the delete operation.
  4. There is a permission for (A) canceling your own account and (B) canceling other accounts (= "administer users").
  5. There is no permission for deleting a user account (unless you count "administer users").

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.

ParisLiakos’s picture

Status: Active » Needs review
FileSize
2.6 KB

this is probably enough?

Berdir’s picture

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

Status: Needs review » Needs work

The last submitted patch, 6: drupal-2250377-6.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev
darvanen’s picture

AaronMcHale’s picture

Title: ProfileForm::actions() defines new delete element instead of altering it as it expects » [PP-1] ProfileForm::actions() defines new delete element instead of altering it as it expects
Status: Needs work » Postponed
Issue tags: +Needs issue summary update

Postponing this on #3258321: Cancel account button on user form triggers server-side validation.. Once that is committed, this issue will need updated/rescoped.

AaronMcHale’s picture

Title: [PP-1] ProfileForm::actions() defines new delete element instead of altering it as it expects » ProfileForm::actions() defines new delete element instead of altering it as it expects
Version: 9.3.x-dev » 9.4.x-dev
Status: Postponed » Needs work

#3258321: Cancel account button on user form triggers server-side validation. has been committed, unpostponing, this issue will also need an update.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

darvanen’s picture

Status: Needs work » Needs review
Issue tags: +Bug Smash Initiative
FileSize
7.8 KB

Well, 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?

Status: Needs review » Needs work

The last submitted patch, 23: 2250377-23.patch, failed testing. View results

darvanen’s picture

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

darvanen’s picture

Status: Needs work » Needs review
FileSize
1.64 KB

Asked in the #bugsmash slack channel and @acbramley agrees that:

Yeah cancel !== delete for users, in fact deleting is only 1 of the options when cancelling a user

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.

Status: Needs review » Needs work

The last submitted patch, 26: 2250377-26.patch, failed testing. View results

darvanen’s picture

Version: 9.5.x-dev » 10.1.x-dev
Category: Bug report » Task
Status: Needs work » Needs review
FileSize
1.82 KB
darvanen’s picture

Issue summary: View changes
smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs change record

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

darvanen’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update, -Needs change record
FileSize
1.01 KB
1.85 KB

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

smustgrave’s picture

Status: Needs review » Needs work

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

darvanen’s picture

Status: Needs work » Needs review
FileSize
1.98 KB
946 bytes
2.79 KB

Yep, 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.

darvanen’s picture

Oops, crafted the test patch badly, here's what it should have been.

The last submitted patch, 33: 2250377-33.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 34: 2250377-34-failing-test-only.patch, failed testing. View results

Ajeet Tiwari’s picture

Status: Needs work » Needs review
FileSize
2.8 KB

uploaded patch for 10.1.x.

The last submitted patch, 33: 2250377-33.patch, failed testing. View results

darvanen’s picture

Issue summary: View changes
Status: Needs review » Needs work

So, @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:

  1. Put special cases into the API systems to cater for the "cancel" entity access value, or
  2. Instead of deprecating the delete key for users, differentiate between them for API vs Forms, possibly with an additional permission, or
  3. Leave well enough alone.

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.

Ajeet Tiwari’s picture

Status: Needs work » Needs review
FileSize
2.8 KB

Uploading patch again for 10.1.x after resolvng the errors of custom commands.

smustgrave’s picture

Status: Needs review » Needs work

Per #39

smustgrave’s picture

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

Ajeet Tiwari’s picture

Status: Needs work » Needs review
FileSize
3.51 KB

tried to fix the CCF error and to fix the error in #34.

_utsavsharma’s picture

Fixed CCF for #43.

smustgrave’s picture

Status: Needs review » Needs work

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

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.