Provide a Entity Handler for user cancelation

Having a generic solution sounds like a good plan, don't think the implementation needs to differ a lot between the entity types. As long as it's possible for entity types to override the default implementation, having a default would save developers a lot of work.

I can imagine you might also want different actions per entity type. For example, unpublish comments, reassign content and media. That might be more challenging.

Ah worked out how the deletes occur - we have node_user_predelete() and comment_user_predelete() - we have to do something about that too.

Yes this sounds like a very good idea, it also aids in GDPR compliance.

Regarding the actual implementation, from the IS:

However this might cause more problems as something like a commerce order might implement EntityOwnerInterface and I'm not sure that deleting them is correct.

I agree with this, I see two possible ways forward:

1. Opt-in per Entity Type: this could mean adding a new key to the Entity Type Annotation which when set to true would enable this, alternatively adding a new trait which must be added to each Entity class that wants to implement this. In the case of using a trait, the trait would contain the necessary code and functions which would be called on user cancel, this would require instanceof checks to be done before calling these methods on each Entity.
2. Opt-out per Entity Type: this could simply involve a new key for the Entity Type Annotation (same as above) which is TRUE by default and for an Entity Type to opt-out they must add the key to their Entity Type Annotation and explicitly set it to FALSE.

One possible issue with the opt-out approach is that it could break backwards compatibility, so ultimately we might have to go with opt-in, update all Core Entity Types and communicate the change to Contrib through a change record and updated documentation.

Guess it could be event listener on entity, so every entity type of authored interface got reaction from opt-in/out

I think this issue is starting to become critical no?

Since EntityOwnerTrait can be used as a default implementation of EntityOwnerInterface → the owner can't be NULL anymore for media entities.

Therefore if you cancel a user you can't edit a media entity that was own by this user. The following error is thrown:

Drupal\Core\Entity\EntityStorageException: SQLSTATE[23502]: Not null violation: 7 ERROR: null value in column "uid" violates not-null constraint DETAIL: Failing row contains (188, 241, image, nl, 1, building-buildings-busy-297743_0.jpg, 442, car, null, 1000, 667, null, 1558535344, 1563353147, 1, 1).: INSERT INTO drupal_webshop_nl.media_field_data (mid, vid, bundle, langcode, status, name, thumbnail__target_id, thumbnail__alt, thumbnail__title, thumbnail__width, thumbnail__height, uid, created, changed, default_langcode, revision_translation_affected) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8, :db_insert_placeholder_9, :db_insert_placeholder_10, :db_insert_placeholder_11, :db_insert_placeholder_12, :db_insert_placeholder_13, :db_insert_placeholder_14, :db_insert_placeholder_15); Array ( ) in Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (line 847 of /var/www/html/src/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php).

(Captain Obvious Note: When updating the media entity if you add an owner no errors are thrown.)

I opened the issue Quick win: Media entities support user cancel → for people who need a temporary solution.

Adding two related Media issues, which are probably smaller-scope duplicates of this one.

Right, changing "Priority to Major" he wrote, whilst not changing Priority to Major in the previous edit :/

I've changed priority to Major because I think Cause a significant admin- or developer-facing bug with no workaround. applies here. For the same urgency reason changing this to Version 8.7.x-dev.

As shown in #3057004: Not having an author when editing/creating a Media Entity results in an EntityStorageException → deleting a Media entities owning user and then trying to save one of these media entities leads to a Fatal Error and the Media entity can't be saved. Since Files and Taxonomies basically have the same issue, I expect the same thing to happen when deleting a user owning one of those entities.

As shown in #3069291: Quick win: Media entities support user cancel → copying the "Node behaviour" for deleting a user fixes this issue for cases after the patch has been applied. For cases before the patch was applied a hook_update_N should be applied.

Now I'm all for solving this in a nice, generic way. But we also need something for now.

I therefore ask the Big Brains that wander this issue to advise on if we should be spending our time on fixing it in a (semi-) non-generic way, by applying the "Node behaviour" on Files, Media and Taxonomies in the way of #3069291 first to put out the immediate fire, and after that return to make it all pretty and nice, as it should be?
Or if we should direct our effort into getting this solved in the generic way straight away?

Here's a test that shows that deleting a user which owns a Media entity in anyway known to me, so deleting programmatically through user_delete and through the GUI using both user_cancel_delete and user_cancel_reassign and then saving that Media entity through the GUI will end up in 500-errors.

This makes it impossible to save any Media entity, even unchanged when the owning user was correctly deleted.
I hope that proves my point for bumping this issue up to Major.

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

Drupal 8.7.9 → was released on November 6 and is the final full bugfix release for the Drupal 8.7.x series. Drupal 8.7.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.8.0 on December 4, 2019. ( Drupal 8.8.0-beta1 → is available for testing.)

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

*grmbl*
git diff and new files
*grmbl*

+100 to #9. Those two linked issues seem duplicates, but I'm not enough of a Media Maven to really
"decide" on the direction / scope here. This is the earliest issue, and the most general scope, and seems like the best place to focus efforts. But this is a major bug leading to real data loss in the wild, and the general solutions discussed here don't have patches yet. Therefore, it seems like leaving those other issues open is better for folks trying to work around this now. But, that risks fracturing the discussion, effort, etc. So perhaps linking those patches and adding the actual error messages to this summary (to make this issue more discoverable via search) would be best?

Meanwhile, #3057004-16: Not having an author when editing/creating a Media Entity results in an EntityStorageException → and #3069291-2: Quick win: Media entities support user cancel → are extremely similar patches, since both are basically just a copy/paste rename of node_mass_update() and friends to become media_mass_update(). That's already raising red flags about DRY ("Don't Repeat Yourself"). :(

Would anyone with more authority in such matters be willing to weigh in on how to best proceed? ;)

Thanks!
-Derek

It seems like what would be really useful here is the ability to easily mass-update any content entities, not just nodes. There is already a pattern for this in core: the ConfigEntityUpdater class. What if we created something similar for content entities and then wrote thin wrappers for particular content entities? Something like:

function media_user_cancel($account) {
  Drupal::classResolver(ContentEntityUpdater::class)->update('media', function (MediaInterface $media) use ($account) {
    if ($media->getOwnerId() === $account->id()) {
      $media->setOwnerId(0);
      return TRUE;
    }
  });
}

Obviously we'd have to do some weird dancing around and through the Batch API, but it's worth it to prevent this widespread and incredibly unfortunate potential for data loss.

Another option is to attack this from the other direction: we could make all core entities that implement EntityOwnerInterface insensitive to a missing user, rather than letting them get into fatal code paths. For example, at some point in its life cycle, the entity could verify that its owner exists, and take some sort of action on itself (like anonymizing and/or unpublishing) if not.

This solution feels more "magic" to me, though, so it wouldn't be my first choice. Just a thought.

I feel like what we need here is something similar to what we do for moderation, have an entity 'handler' for user deleting.

User module would ship with a base class, and each entity-type could opt-in to this behaviour by adding a 'user_delete' handler.

Then user could implement a default hook_user_cancel and loop over entity types that have such a handler and do the necessary work.

Then node_user_cancel, comment_user_cancel etc could go away.

What are your thoughts on that sort of API @alexpott?

I don't think we can do

we could make all core entities that implement EntityOwnerInterface insensitive to a missing user,

part of this is about privacy and choices about what data a site keeps on you when you remove an account.

Here's a list of the entity types in core that implement entity owner interface:

• ContentModerationState - these will be deleted when the corresponding node is deleted - but the more interesting thing is what happens when the node owner is re-assigned - I'm not even sure why this has a UID. It seems wrong.
• Comment - handled already - but it is amusing that the updates are not batched (better not delete myself from drupal.org)
• Node - handled already
• Workspace - needs something done
• File - needs something done
• Media - needs something done

Taxonomy terms are not owned. And neither are Shortcuts - though we need an issue to clean up the shortcut_set_users on user delete.

• I think we should file an issue to discuss removing the owner stuff from ContentModerationState.
• Files and Media are problematic - the problem is that these are designed to be shared - you are uploading to a media library. Therefore mass deleting all a user's media item feels odd. On the other hand - isn't that what a user would expect - the ability to remove everything they've ever uploaded.

Looking at some contrib examples:

• Flag - deletes regardless of user cancel method - makes sense
• Group - changes the cancel method descriptions a bit (concatenates t()! - can result in double escaping) - and reassigns to user 1 and not user 0

I like the idea in #19 - making it a handler is a good idea. We could also trigger a deprecation if the entity type uses the entity owner trait and doesn't have the handler and in D10 throw an exception.

Anyone interested in creating a proof of concept of #19?

Should we postpone the related issues on that?

Comment - handled already - but it is amusing that the updates are not batched (better not delete myself from drupal.org)

Literally laughed out loud when I read that, it's so true.

Files and Media are problematic - the problem is that these are designed to be shared - you are uploading to a media library. Therefore mass deleting all a user's media item feels odd. On the other hand - isn't that what a user would expect - the ability to remove everything they've ever uploaded.

Yes and no, I think it depends on the implementation, couple of scenarios:

1. Content editors on a University website create nodes and media, if the user leaves the organisation their user is eventually deleted but those shared nodes and media items remain.
2. Users of a community website create nodes, comments and media, they each have a "personal media library" (fancy way of saying modified Media Library View that only show content they've created), when a user is deleted you expect all of their content to be deleted, including what's in their personal media library.

I think scenario 2 is a bit of a stretch though, I imagine in reality everything would probably just become owned by the anonymous user to perverse the content. I'm sure there's a stringer example out there, but this was the best I could come up with in 5 minutes.

I also really like the idea proposed in #19, additionally I think it forces developers of custom entities (like me!) to think about what'll happen in this scenario, far too often I completely overlook this and it's almost a after thought, this solution puts it a bit more front end centre.

Anyone interested in creating a proof of concept of #19?

Should we postpone the related issues on that?

I'd love to, but I'm completely time pushed at the moment.

I'll try a proof of concept by moving the node update stuff into a handler.

So here's a pretty crude proof of concept -- I took the stuff in the Node module and adapted it into an entity handler that should, in theory, work for any content entity type.

I don't know how much test coverage this has, but let's see what, if anything, it breaks. If it doesn't break anything, and people think this architecture looks good, I think the next step is probably to clean it up, refactor it into smaller, nicer methods, and add test coverage for all core entity types using this handler.

The last submitted patch, 25: 3043725-25.patch, failed testing. View results →

Okay, this should fix the one broken test. Not bad for a proof of concept.

Adopted BatchBuilder and simplified a couple of things. More refactoring is yet to come; the code we're adapting here hasn't been touched since 2014, and clearly it's from another era. I suspect we can make it a lot cleaner.

The last submitted patch, 27: 3043725-27.patch, failed testing. View results →

More refactoring. Since it's a big set of changes, no interdiff this time. Let's see if any tests break.

The most notable things here:

• I may have missed something, but I don't think that the $revisions flag was needed. Given the way the entity query system purports to work, I see no reason we can't just always query for revisions. My guess, in this case, is that the code I adapted from node.admin.inc is utterly fossilized and not up-to-date with how the entity query system currently works.
• Speaking of node.admin.inc, I removed it. It serves no further purpose, and node_mass_update(), despite not being internal, is not even executable without its helper methods -- all of which are prefixed with _ and therefore, as @larowlan tells it to me, not subject to our API contract.
• Added an interface for the cancellation handler, with constants defining the built-in cancellation methods.
• Split the implementation into two -- the base class, DefaultCancellationHandler, doesn't bother with the batch system. It is subclassed by BatchCancellationHandler, which does the Batch API stuff. I think this is quite a bit cleaner.

I've done enough proving of the concept here, I think. De-assigning so others can tear into it!

Speaking of node.admin.inc, I removed it. It serves no further purpose, and node_mass_update(), despite not being internal, is not even executable without its helper methods -- all of which are prefixed with _ and therefore, as @larowlan tells it to me, not subject to our API contract.

Idea for how to handle this from a BC perspective: we should probably just leave it exactly as it is, and deprecate the entire file for removal in Drupal 10. Although ultimately superseded by the cancellation handler, it does work rather differently -- it can mass update any field values, not just the ones related to ownership and publishing status. Which leads me to believe that there is very likely code out there which is using it for convenience. So deprecation seems like the least disruptive thing to do.

Go big or go home.

Just to see how much stuff this breaks, I implemented user_user_cancel() to invoke all cancellation handlers, removing node_user_cancel() and comment_user_cancel() in the process. This also restores node.admin.inc based on my reasoning in #33. Per @alexpott's comment in #20, I also added the batch cancellation handler to media, workspaces, and files.

Argh! This should be a 9.1.x issue anyway.

That'll teach me to be "clever" with core's dependency injection patterns.

The last submitted patch, 36: 3043725-36.patch, failed testing. View results →

I really like what's been done here so far, nice work @phenaproxima :)

Just a few minor things I picked up while reading through the patch:

1. Could we have all of these classes sit in a sub-folder of each respective module, maybe: Handler, EntityHandler or even Entity\Handler? I'm just really mindful that many Core modules already have a lot of classes sitting at the top level under src. More from an organisational perspective, if we keep these in a sub-folder, it wouldn't contribute to the clutter and keeps things cleaner.

2. +++ b/core/modules/comment/src/CancellationHandler.php
   @@ -0,0 +1,57 @@
   +<?php
   +
   +namespace Drupal\comment;
   +
   [...]
   +
   +class CancellationHandler extends DefaultCancellationHandler {
   

   Would it be better for Drupal\comment\CancellationHandler to extend BatchCancellationHandler instead of DefaultCancellationHandler, I imagine some sites will have users with thousands of comments, to quote @alexpott above:

   Comment - handled already - but it is amusing that the updates are not batched (better not delete myself from drupal.org)

3. +/**
   + * Implements hook_user_cancel().
   + */
   +function user_user_cancel($edit, UserInterface $account, $method) {
   

   Is using hook_user_cancel() necessary? I mean, strictly speaking couldn't the code in here just be added to the existing user_cancel() function? To me it would seem simpler to do that.

Thanks,
-Aaron

Whoops.

This should fix the test failures in #36.

Responding to Aaron's feedback:

1. I agree they should be in a sub-folder, but not sure what it should be named. I'll leave that to the framework managers to make a recommendation, or point me to existing precedent in core. (For what it's worth, Content Moderation puts its handler in the Entity\Handler sub-namespace.)
2. Ideally yes, but I think that might be best done in a follow-up issue, since it would be a behavior change.
3. Right you are. Fixed.

Re #39

1. Well in that case since Entity\Handler is already an existing pattern in a Core module, I'd vote to just continue that pattern, makes sense if you think about it, these are handler classes which relate directly to Entities. There is already a president in a number of areas for using these nested namespaces/folders, Plugin is the obvious one that comes to mind.
2. Yep makes sense.

I had a further idea on this.

It would be nice if we could give administrators some level of visibility into which and how many Entities will be impacted by their chosen operation before they proceed.

Consider the scenario, a user who hadn't created any Nodes, but had authored many Media and Taxonomy entities, along with some other entities of custom types. This user is to be deleted, so I check the list of content for that user and nothing shows up, foolishly assuming that they only created some Nodes, then seeing they don't own any Nodes, I then assume its safe to delete that user, but only after realise how many Media, Taxonomy and other custom Entities have just been deleted. I now need to attempt to restore from some earlier backup of the site (this is assuming I have a backup).

I believe we could solve this by providing some visibility into which Entity Types and how many of those Entity Types will be impacted. We already have this kind of UI pattern in place for when you delete a Field, so I think using that similar pattern would be sensible.

In terms of the actual implementation this could and probably should be done in a follow-up issue, but I wanted to raise it here so that we could at least have visibility of the ground work that would be required. I envision we would probably want an additional method on the handler interface and base class. This method could be provided in the base class because all it would need to do is check that the entities of this type can be owned, and query for the number of entities with that owner, similar to the DefaultCancellationHandler::getQuery method in the latest path.

Happy to create the follow-up issue if we think it's a good idea to consider further.

Thanks,
-Aaron

Title and summary updates.

As per #42 and subsequent agreement in Slack, created #3163993: [PP-1] User Cancellation: give administrators visibility into which entities will be impacted by the cancellation of the particular user → and postponed on this issue, set parent/child relationship.

It looks like duplicate of 📌 Move user cancellation functions to a service Closed: duplicate

Transferring credit from 📌 Move user cancellation functions to a service Closed: duplicate and re-posting the latest patch from there, since it has some good work (not to mention tests) that we might want to merge into the current patch.

phenaproxima → credited kim.pepper → .

phenaproxima → credited jibran → .

phenaproxima → credited dpi → .

Re #50:

Yeah I like what's been accomplished there, merging that in seems sensible since both patches are very close to overlapping.

I like the idea of a service to essentially replace the user_cancel and related functions in user.module, what I wonder is, do we want to do anything about the actual delete method on the Entity Storage. Neither the User Entity Class nor the UserStorage storage class override EntityBase::delete nor SqlContentEntityStorage::delete. The reason I bring that up is because, in theory, a developer could simply call $user->delete() (which is basically what user_cancel is ultimately doing); but in doing so they completely bypass the user cancelation process, meaning Entities never get a chance to react to the deletion, and the result is a whole lot of broken Entities.

Now, let me clarify, this isn't a new problem, from what I can tell even in HEAD calling $user->delete() wouldn't even trigger hook_user_cancel() so this may be out of scope here and so could be dealt with in a follow-up, but figured it was worth mentioning.

Ultimately though that problem might end up being non-issue, because at the end of the day we are relying on calling $user->delete() in both our approach and in HEAD, and so we can't simply just override the delete methods and throw exceptions telling developers what the proper way to do it is, because logically that would also break our shiny new service.

Or would it... Well in theory we could override SqlContentEntityStorage::delete in UserStorage and just make it throw the exception I described, advising against using it, then in UserStorage we create a new method, called something like callParentDelete which just calls parent::delete, this method would be marked as @internal and not present in UserStorageInterface. In our service we then call UserStorage::callParentDelete, and in doing so we are still able to safely delete the User Entity, but avoid triggering the exception which would be triggered by calling UserStorage::delete.

Again though, happy to discuss more in a follow-up issue, and to avoid BC-break we could only warn against the use of User::delete and UserStorage::delete, in D10 we would be able to change this to thrown an exception.

Drupal 9.1.0-alpha1 → will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule → and the Allowed changes during the Drupal 9 release cycle → .

Other option could be queue some jobs to process each kind of entity to run some default cancellation method.
So when user deleted from UI this jobs could run batch via each entity type having this handler

Patch #39 ✨ Provide a Entity Handler for user cancelation Needs work needs a reroll for 9.2.x.

Patch #50 ✨ Provide a Entity Handler for user cancelation Needs work also needs a reroll for 9.2.x.
Besides that, it needs to be merged with #39:

since it has some good work (not to mention tests) that we might want to merge into the current patch.

Thus spoketh phenaproxima in #39 ✨ Provide a Entity Handler for user cancelation Needs work

Re-rolled #39 against latest 9.2.x.

Drupal 9.2.0-alpha1 → will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Attempt to fix the test cases. I tried to debug the root cause and found following things:

+++ b/core/modules/node/node.module
@@ -607,33 +606,6 @@ function node_ranking() {
-function node_user_cancel($edit, UserInterface $account, $method) {
-  switch ($method) {
-    case 'user_cancel_block_unpublish':
-      // Unpublish nodes (current revisions).
-      $nids = \Drupal::entityQuery('node')
-        ->accessCheck(FALSE)
-        ->condition('uid', $account->id())
-        ->execute();
-      module_load_include('inc', 'node', 'node.admin');
-      node_mass_update($nids, ['status' => 0], NULL, TRUE);
-      break;
-
-    case 'user_cancel_reassign':

If we see in old node_user_cancel() method, we are adding accessCheck(FALSe) to prevent additional acceess checks.
I think if we do the same thing, that might prevent the test case failures.

For now, I've added a patch with that fix. Let's see if it doesn't affect other test cases.

Re #56 by @mohit_aghera: yeah I think that makes sense, if we were already doing that in the node module then makes sense to bring it over. It does seems rather unnecessary to trigger a bunch of access checking in this case.

Just as a general reminder of outstanding follow-ups and discussion thus far:

We still have an outstanding discussion starting at #38 (#38.1) going through to #41 regarding what namespace this should live under. I'm in favour of Entity\Handler.

Similarly, in #38 (#38.2) through #41, we decided we might need a follow-up for the question:

Would it be better for Drupal\comment\CancellationHandler to extend BatchCancellationHandler instead of DefaultCancellationHandler

Personally, I don't mind creating this unless anyone else wants to beat me to it :)

In #51 I posed an idea about how we might address the implications of a developer simply calling $user->delete() without any prior checking, which could have pretty bad consequences in some situations. That change in itself could be considered a API breaking change, so I think that probably needs to be a follow-up so it can be properly discussed. Any opinions from anyone on whether it should be done here or in a follow-up?

about how we might address the implications of a developer simply calling $user->delete() without any prior checking

I wonder if it might not make sense for the user storage handler to call the cancellation handler during delete(), unless there's some flag explicitly passed to bypass that? So basically, calling $user->delete() would invoke the cancellation stuff. If a developer wanted to delete a user specifically without cancellation, they'd have to do something like \Drupal::entityTypeManager()->getStorage('user')->delete($account, FALSE).

Thoughts?

I wonder if it might not make sense for the user storage handler to call the cancellation handler during delete(), unless there's some flag explicitly passed to bypass that? So basically, calling $user->delete() would invoke the cancellation stuff. If a developer wanted to delete a user specifically without cancellation, they'd have to do something like \Drupal::entityTypeManager()->getStorage('user')->delete($account, FALSE).

Actually, yeah that might be a good and rather elegant approach here.

Okay so, I think what we're basically saying is that, calling $user->delete() is the nice and safe way to do it, but as a developer if have a use-case where you really just want just get rid of that user from the database and either don't need to care or are (in other ways) dealing with the consequences, then go ahead and call ->getStorage('user')->delete($account, FALSE).

I like the logic with that, so I think that makes sense, and we probably should include code docs explaining that.

Do we want to somehow provide the option to pass the method that should be used e.g. CancellationHandlerInterface::METHOD_REASSIGN, and if so should there be a default option if none is passed (to satisfy EntityInterface::delete and EntityStorageInterface::delete?

Do we want to somehow provide the option to pass the method that should be used e.g. CancellationHandlerInterface::METHOD_REASSIGN, and if so should there be a default option if none is passed (to satisfy EntityInterface::delete and EntityStorageInterface::delete?

IMHO, that is not something that should be exposed by User::delete() -- it should just use a hard-coded default cancellation method (probably whatever the default is now when a node is deleted). I'd think that, if you as a developer want to choose a specific cancellation method, you should invoke the cancellation handler directly (especially since, in such an advanced use case, you might want to use different cancellation methods for different cancellation-aware entity types).

IMHO, that is not something that should be exposed by User::delete() -- it should just use a hard-coded default cancellation method (probably whatever the default is now when a node is deleted). I'd think that, if you as a developer want to choose a specific cancellation method, you should invoke the cancellation handler directly (especially since, in such an advanced use case, you might want to use different cancellation methods for different cancellation-aware entity types).

Yeah that makes sense. I guess that then comes back round to my original thought, what should the default behaviour be?

Out of the four possible options, if you're calling ::delete, then you're not intending to just cancel/block the user (we have other methods if that is the intention e.g. UserInterface::block); So we're left with two choices: METHOD_REASSIGN or METHOD_DELETE. Given those two options, I say we should default to the lesser of two evils, METHOD_REASSIGN, as calling ::delete in my opinion shouldn't result in the indirect permanent deletion of potentially hundreds or thousands of other Entities, here we should do the minimal amount to ensure that deleting a user doesn't put a site in a broken state, that being triggering METHOD_REASSIGN.

In terms of implementing it, User::delete (as far as I can tell), isn't explicitly overridden, it just inherits the behaviour of EntityBase::delete, which ultimately just ends up calling SqlContentEntityStorage::delete, which in turn calls EntityStorageBase::delete. I imagine this is probably what we're going to end up overriding in any case, but there is a further layer of abstraction going on hidden behind abstract functions and hooks, so I didn't bother diving any deeper. We probably just need an override function in UserStorage and an implementation in UserStorageInterface for documentation and since we're introducing a new optional boolean parameter.

I wonder if it might not make sense for the user storage handler to call the cancellation handler during delete(), unless there's some flag explicitly passed to bypass that?

+1

============

If handlers are good for repsonding to user deletion, maybe they're good for respomdig to other deletions too. Would it not make sense to make the annotation more flexible to allow for this possibility?

The current patch proposes this annotation:

handlers = {
  ...
  "user_cancel" = "Drupal\user\BatchCancellationHandler",
}

invoked from user_cancel() like this:

  $entity_type_manager = \Drupal::entityTypeManager();
  foreach ($entity_type_manager->getDefinitions() as $entity_type) {
    if ($entity_type->hasHandlerClass('user_cancel')) {
      $entity_type_manager->getHandler($entity_type->id(), 'user_cancel')
        ->cancelAccount($account, $method);
    }
  }

Perhaps we should make this the annotation:

handlers = {
  ...
  "deletion" = {
    "user" = "Drupal\user\BatchCancellationHandler",
  }
}

and invoke it from EntityStorageBase like this:

public function delete(array $entities) {
  ...

  // Perform the delete and reset the static cache for the deleted entities.
  $this
    ->doDelete($keyed_entities);
  $this
    ->resetCache(array_keys($keyed_entities));

  foreach ($this->entityTypeManager->getDefinitions() as $entity_type) {
    $deletionHandlers = $entity_type->getHandlerClasses()['form']
    if (isset($deletionHandlers[$this->entityTypeId()]) {
      $handler = $this->entityTypeManager->createHandlerInstance($deletionHandlers[$this->entityTypeId()], $entity_type);
      $handler->delete($keyedEntities);
    }
  }

}

For minimal extra complexity we gain a significantly more flexible feature.

Hiding patch files now that we have a MR.

Re @jonathanshaw in #65: I like your thinking there, in theory it might work, thing is though we need to be able to pass the cancellation method to the handler, and your code snippet for EntityStorageBase doesn't seem to accommodate that. I'm not sure how we could accommodate that in a generic approach when the cancellation method is very specific to User Entities.

More broadly I think if another Entity type wanted to implement a deletion handler of sorts, they might also have some custom parameters that they want to have passed to the handler. Off the top of my head, I'm struggling to think of an example of another Entity Type that would want to provide a deletion/cancellation handler in as broad a sense as this.

As I said, I like that kind of thinking, I'm all for expanding the potential of the Entity API itself, but we would need to overcome those hurdles first I think.

After poking at it a bit, I think we should defer the User::delete() stuff to a follow-up. Doing it here would bloat and confuse the scope of this issue. Adding entity type-specific handlers here is a clear feature request that doesn't change the existing behavior of User::delete(). Modifying the delete logic might mean backwards compatibility issues and (light) API changes, so it makes sense to handle those in another issue.

The current merge request cleanly applies to 9.3.x, so removing the "Needs reroll" tag.

I think we should defer the User::delete() stuff to a follow-up. Doing it here would bloat and confuse the scope of this issue.

Fair enough.

I do still wonder whether it's be best to use a more generic annotation syntax that could be extended in future to entity types other than users. e.g.

handlers = {
  ...
  "deletion" = {
    "user" = "Drupal\user\BatchCancellationHandler",
  }
}

I do still wonder whether it's be best to use a more generic annotation syntax that could be extended in future to entity types other than users. e.g.

To be honest, I don't really see how that is beneficial compared to hook_user_delete(), which AFAICT provides the same functionality...

I don't really see how that is beneficial compared to hook_user_delete(), which AFAICT provides the same functionality...

I don't really either, but the IS doesn't give me a clear idea of why a cancellation handler is beneficial compared with hook_user_handler. so I trusted there must be a reason and wondered if it applied more generically ... I don't the question to obstruct the work here though.

Opened #3212623: [PP-1] Determine the relationship between deleting users and cancelling them → to discuss how User::delete() and the cancellation handlers should relate and interact.

Added #3212624: [PP-1] Comments should be batch processed upon user cancellation → to convert comments to use the batch cancellation handler.

Removing the Needs Followup tag that I added in #59 as all have now been created, thanks @phenaproxima for creating those, and yeah I'm happy to move the User::delete discussion to that followup :)

jibran asked me to do a review.

I read the IS and am pleased to say it was concise, informative and quite up to date. Thank you alexpott and AaronMcHale. There were quite a few follows up to make and they have all been done. I found only 1 item that has not yet been action on. It is changing the namespace is " Use Entity\Handler namespace' This was brought up in #38, #39. In #41 it was pointed out that there was precedence so I think it should be implemented.

I then started to review the MR and managed to only do the files in modules/user before running out of time.

Crediting @quietone for review.

Drupal 9.3.0-rc1 → was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Haha, and the joke is on me. jibran asked me to review a different issue!

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

This needs a reroll, but I don't have push access, so here's a patch for those who need it.

Another reroll

The last submitted patch, 82: 3043725-82.patch, failed testing. View results →

Rerolled patch #80 with Drupal 9.5.x

updated version of the patch of the #85 with Drupal 9.5.x

+++ b/core/modules/node/tests/src/Functional/UserCancellationTest.php
@@ -0,0 +1,88 @@
+ * Tests how nodes react to user cancellation.

+++ b/core/modules/user/src/CancellationHandlerInterface.php
@@ -0,0 +1,64 @@
+ * Defines an interface for entity types to react to user account cancellation.
+ */
+interface CancellationHandlerInterface {

I think this handler needs better documentation,especially about when and how to add it

Fixed the coding standard issues related to the above patch #86. It still needs documentation changes so keeping it in needs work state

Fixed the coding standard issue with the above patch

Drupal 9.5.0-beta2 → and Drupal 10.0.0-beta2 → were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

1. +++ b/core/modules/user/src/CancellationHandlerInterface.php
   @@ -0,0 +1,64 @@
   +   * With this cancellation method, the user account is blocked and the
   +   * current revision of any entity associated with the account is unpublished.
   

   These are saying what *should* be done by the handlers, so the wording 'is unpublished' doesn't seem right to me.

   Also, what should a handler do if its entity type doesn't support the published status?

2. +++ b/core/modules/user/src/CancellationHandlerInterface.php
   @@ -0,0 +1,64 @@
   +   * revisions of any entity associated with it are reassigned to the anonymous
   

   Same - wording.

3. +++ b/core/modules/user/src/CancellationHandlerInterface.php
   @@ -0,0 +1,64 @@
   +   * With this cancellation method, the user account is deleted. Modules may
   +   * implement entity hooks to react to the deletion as they see fit; it is
   +   * assumed that all entities associated with the account will be deleted.
   

   Why would they implement entity hooks -- isn't this what the handler is for?

4. +++ b/core/modules/user/src/CancellationHandlerInterface.php
   @@ -0,0 +1,64 @@
   +   *   hook_user_cancel_methods().
   

   This is a new hook being invented. It needs to be documented.

5. +++ b/core/modules/user/src/Entity/Handler/BatchCancellationHandler.php
   @@ -0,0 +1,216 @@
   +      batch_set($batch->toArray());
   

   What happens if a user entity is deleted via the API?

Heads up there's another issue with a different approach over here #3135592: Cannot implement a custom user cancellation method → - it would be a good idea to compare notes and try and agree on one approach that solves all issues.

Just came across this, thanks to @larowlan. The main goal of #3135592: Cannot implement a custom user cancellation method → is to solve an API bug, that prevents 3rd-party to implement a custom user cancellation method, even we pretend that it's possible. It's not. The modernization of the whole user cancellation process was only a necessity. The approach was suggested by @alexpott, in #3135592-14: Cannot implement a custom user cancellation method → , who is also the author of this ticket.

I like the entity handler approach. However, I see some aspects:

• The handler is dedicated to entity types, meaning that other 3rd party will still use hook_user_cancel(). Suddenly, we have two ways, two APIs, to react on user cancellation. #3135592: Cannot implement a custom user cancellation method → is deprecating hook_user_cancel() (to be removed in D11) and puts in place a unique way to react on user cancellation, based on event dispatcher.
• Dispatching an event might be a better API because you ca determine the order of 3rd-party reactions by using the subscribers priority. E.g. you want comment to react before node. Disclaimer: I didn't check yet if this solution allows that.
• Didn't have time to read the code here but I wonder if this solves also #3135592: Cannot implement a custom user cancellation method →

Should we reconsider all these architectural aspects here?

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

Closed as duplicate for the comment module 📌 Add comment_mass_update() Closed: duplicate

Reroll of #89

Reroll of #96

This needs first a decision/discussion on #93

Rerolled patch for Drupal 10.3.x

+++ b/core/modules/user/src/Entity/Handler/BatchCancellationHandler.php
@@ -0,0 +1,216 @@
+      $this->updateEntity($entity, $method);

As found in 🐛 _node_mass_update_batch_process fails during user cancel when revision is deleted Needs review the entity could technically be removed between creating the batch and the execution of this method. We should check if $entity is properly loaded.

+++ b/core/modules/user/src/Entity/Handler/DefaultCancellationHandler.php
@@ -0,0 +1,185 @@
+    $this->storage->save($entity);

As found in 🐛 Revision user incorrectly appears as anonymous user when node author is cancelled Needs work a new revision is created when content moderation is used. We should probably try to prevent this by checking for SynchronizableInterface and doing something like this:

  $original_syncing = $entity->isSyncing();
  $entity->setSyncing(TRUE);
  $this->storage->save($entity);
  $entity->setSyncing($original_syncing);

Can we get an answer on #93 from the subsystem maintainer before doing any development here?

Since I needed something that works in a current project, fixed the issues found in my last review. I agree we should get answers for the questions in #93.