Problem/Motivation
At the moment every content entity type must implement hook_user_cancel()
to do updates when a user is cancelled. We have comment_user_cancel()
and node_user_cancel()
but we are missing implementation for other Core Entities, including: taxonomy_user_cancel()
, file_user_cancel()
and media_user_cancel()
. We need to handle both user_cancel_block_unpublish
and user_cancel_reassign
.
Also we have node_user_predelete()
and comment_user_predelete()
that will delete all the nodes and comments if the user is deleted. We might need something generic for that too.
In addition, many custom/contrib Content Entity Types likely have missing or incomplete implementations of the relevant hooks to properly react when a user is cancelled.
Proposed resolution
It has been decided to provide a generic solution using a Entity Handler dedicated to reacting on the cancelation a user.
The proposed handler will support each option that the administrator has for cancelling a user (mainly deleting all associated content or making that content anonymous). Additionally a implementation will also be provided which will support batch processing.
Each Content Entity Type will have to add this new handler to their Entity Type Annotation, developers can take advantage of the handlers provided by core, or can extend those handlers to define their own custom functionality. The latter is not required for custom Entity Types and the handlers provided will work with custom Entity Types.
Remaining tasks
- Continue to refine the proposed resolution and review associated patches.
- Framework manager decision is required on which namespace these new handler classes should use, see #39.
User interface changes
@todo - perhaps the user cancel form should give more indication of how many and what entities are going to be affected.
API changes
- Modifications to User module to provide new handlers.
- Modifications to some core modules to replace the usage of
hook_user_cancel()
with these new handlers.
Data model changes
No data model changes.
Release notes snippet
TODO.
Comment | File | Size | Author |
---|---|---|---|
#97 | 3043725-97.patch | 42.16 KB | Jelle_S |
#96 | 3043725-96.patch | 42.13 KB | Jelle_S |
Issue fork drupal-3043725
Show commands
Start 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:
Comments
Comment #2
alexpottComment #3
seanBHaving 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.
Comment #4
alexpottAh worked out how the deletes occur - we have node_user_predelete() and comment_user_predelete() - we have to do something about that too.
Comment #5
AaronMcHaleYes this sounds like a very good idea, it also aids in GDPR compliance.
Regarding the actual implementation, from the IS:
I agree with this, I see two possible ways forward:
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.
Comment #6
andypostGuess it could be event listener on entity, so every entity type of authored interface got reaction from opt-in/out
Comment #7
beram CreditAttribution: beram as a volunteer and at Ekino commentedI 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:
(Captain Obvious Note: When updating the media entity if you add an owner no errors are thrown.)
Comment #8
beram CreditAttribution: beram as a volunteer and at Ekino commentedI opened the issue Quick win: Media entities support user cancel for people who need a temporary solution.
Comment #9
phenaproximaAdding two related Media issues, which are probably smaller-scope duplicates of this one.
Comment #10
SpokjeI'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?
Comment #11
SpokjeRight, changing "Priority to Major" he wrote, whilst not changing Priority to Major in the previous edit :/
Comment #12
SpokjeHere's a test that shows that deleting a user which owns a Media entity in anyway known to
mankindme, so deleting programmatically throughuser_delete
and through the GUI using bothuser_cancel_delete
anduser_cancel_reassign
and then saving that Media entity through the GUI will end up intears500-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.
Comment #13
Spokje*grmbl*
git diff and new files
*grmbl*
Comment #16
dww+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 becomemedia_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
Comment #17
phenaproximaIt 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:
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.
Comment #18
phenaproximaAnother 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.
Comment #19
larowlanI 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?
Comment #20
alexpottI don't think we can do
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:
Taxonomy terms are not owned. And neither are Shortcuts - though we need an issue to clean up the shortcut_set_users on user delete.
Looking at some contrib examples:
Comment #21
alexpottI 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.
Comment #22
larowlanAnyone interested in creating a proof of concept of #19?
Should we postpone the related issues on that?
Comment #23
AaronMcHaleLiterally laughed out loud when I read that, it's so true.
Yes and no, I think it depends on the implementation, couple of scenarios:
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.
I'd love to, but I'm completely time pushed at the moment.
Comment #24
phenaproximaI'll try a proof of concept by moving the node update stuff into a handler.
Comment #25
phenaproximaSo 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.
Comment #27
phenaproximaOkay, this should fix the one broken test. Not bad for a proof of concept.
Comment #28
phenaproximaAdopted 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.
Comment #30
andypostComment #31
phenaproximaMore 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:
Comment #32
phenaproximaI've done enough proving of the concept here, I think. De-assigning so others can tear into it!
Comment #33
phenaproximaIdea 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.
Comment #34
phenaproximaGo 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.
Comment #35
phenaproximaArgh! This should be a 9.1.x issue anyway.
Comment #36
phenaproximaThat'll teach me to be "clever" with core's dependency injection patterns.
Comment #38
AaronMcHaleI 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:
Handler
,EntityHandler
or evenEntity\Handler
? I'm just really mindful that many Core modules already have a lot of classes sitting at the top level undersrc
. More from an organisational perspective, if we keep these in a sub-folder, it wouldn't contribute to the clutter and keeps things cleaner.Would it be better for
Drupal\comment\CancellationHandler
to extendBatchCancellationHandler
instead ofDefaultCancellationHandler
, I imagine some sites will have users with thousands of comments, to quote @alexpott above:Is using
hook_user_cancel()
necessary? I mean, strictly speaking couldn't the code in here just be added to the existinguser_cancel()
function? To me it would seem simpler to do that.Thanks,
-Aaron
Comment #39
phenaproximaThis should fix the test failures in #36.
Responding to Aaron's feedback:
Comment #40
phenaproximaWhoops.
Comment #41
AaronMcHaleRe #39
Comment #42
AaronMcHaleI 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
Comment #43
AaronMcHaleTitle and summary updates.
Comment #44
AaronMcHaleAs 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.
Comment #45
andypostIt looks like duplicate of #3079085: Move user cancellation functions to a service
Comment #46
andypostComment #50
phenaproximaTransferring credit from #3079085: Move user cancellation functions to a service 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.
Comment #51
AaronMcHaleRe #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 inuser.module
, what I wonder is, do we want to do anything about the actual delete method on the Entity Storage. Neither theUser
Entity Class nor theUserStorage
storage class overrideEntityBase::delete
norSqlContentEntityStorage::delete
. The reason I bring that up is because, in theory, a developer could simply call$user->delete()
(which is basically whatuser_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 triggerhook_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 inHEAD
, 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
inUserStorage
and just make it throw the exception I described, advising against using it, then inUserStorage
we create a new method, called something likecallParentDelete
which just callsparent::delete
, this method would be marked as@internal
and not present inUserStorageInterface
. In our service we then callUserStorage::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 callingUserStorage::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
andUserStorage::delete
, in D10 we would be able to change this to thrown an exception.Comment #52
andypostOther 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
Comment #54
SpokjePatch #39 needs a reroll for
9.2.x
.Patch #50 also needs a reroll for
9.2.x
.Besides that, it needs to be merged with #39:
Thus spoketh phenaproxima in #39
Comment #55
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedRe-rolled #39 against latest 9.2.x.
Comment #56
mohit_aghera CreditAttribution: mohit_aghera at QED42 commentedAttempt to fix the test cases. I tried to debug the root cause and found following things:
If we see in old
node_user_cancel()
method, we are addingaccessCheck(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.
Comment #58
AaronMcHaleRe #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.
Comment #59
AaronMcHaleJust 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:
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?Comment #60
phenaproximaI 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?
Comment #61
AaronMcHaleActually, 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 satisfyEntityInterface::delete
andEntityStorageInterface::delete
?Comment #62
phenaproximaIMHO, 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).
Comment #64
AaronMcHaleYeah 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
orMETHOD_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 triggeringMETHOD_REASSIGN
.In terms of implementing it,
User::delete
(as far as I can tell), isn't explicitly overridden, it just inherits the behaviour ofEntityBase::delete
, which ultimately just ends up callingSqlContentEntityStorage::delete
, which in turn callsEntityStorageBase::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 inUserStorage
and an implementation inUserStorageInterface
for documentation and since we're introducing a new optional boolean parameter.Comment #65
jonathanshaw+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:
invoked from user_cancel() like this:
Perhaps we should make this the annotation:
and invoke it from EntityStorageBase like this:
For minimal extra complexity we gain a significantly more flexible feature.
Comment #66
AaronMcHaleHiding patch files now that we have a MR.
Comment #67
AaronMcHaleRe @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.
Comment #68
phenaproximaAfter 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.
Comment #69
phenaproximaThe current merge request cleanly applies to 9.3.x, so removing the "Needs reroll" tag.
Comment #70
jonathanshawFair 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.
Comment #71
phenaproximaTo be honest, I don't really see how that is beneficial compared to hook_user_delete(), which AFAICT provides the same functionality...
Comment #72
jonathanshawI 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.
Comment #73
phenaproximaOpened #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.
Comment #74
phenaproximaAdded #3212624: [PP-1] Comments should be batch processed upon user cancellation to convert comments to use the batch cancellation handler.
Comment #75
AaronMcHaleRemoving 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 :)
Comment #76
quietone CreditAttribution: quietone as a volunteer commentedjibran 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.
Comment #77
phenaproximaCrediting @quietone for review.
Comment #78
quietone CreditAttribution: quietone as a volunteer commentedHaha, and the joke is on me. jibran asked me to review a different issue!
Comment #80
Jelle_SThis needs a reroll, but I don't have push access, so here's a patch for those who need it.
Comment #82
Jelle_SAnother reroll
Comment #83
rpayanmComment #85
Shubham Chandra CreditAttribution: Shubham Chandra as a volunteer and at Dotsquares Ltd. commentedRerolled patch #80 with Drupal 9.5.x
Comment #86
Shubham Chandra CreditAttribution: Shubham Chandra as a volunteer and at Dotsquares Ltd. commentedupdated version of the patch of the #85 with Drupal 9.5.x
Comment #87
andypostI think this handler needs better documentation,especially about when and how to add it
Comment #88
ameymudras CreditAttribution: ameymudras at Salsa Digital commentedFixed the coding standard issues related to the above patch #86. It still needs documentation changes so keeping it in needs work state
Comment #89
ameymudras CreditAttribution: ameymudras at Salsa Digital commentedFixed the coding standard issue with the above patch
Comment #90
joachim CreditAttribution: joachim as a volunteer commentedThese 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?
Same - wording.
Why would they implement entity hooks -- isn't this what the handler is for?
This is a new hook being invented. It needs to be documented.
What happens if a user entity is deleted via the API?
Comment #92
larowlanHeads 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.
Comment #93
claudiu.cristeaJust 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:
hook_user_cancel()
. Suddenly, we have two ways, two APIs, to react on user cancellation. #3135592: Cannot implement a custom user cancellation method is deprecatinghook_user_cancel()
(to be removed in D11) and puts in place a unique way to react on user cancellation, based on event dispatcher.Should we reconsider all these architectural aspects here?
Comment #95
andypostClosed as duplicate for the comment module #355926: Add comment_mass_update()
Comment #96
Jelle_SReroll of #89
Comment #97
Jelle_SReroll of #96
Comment #98
claudiu.cristeaThis needs first a decision/discussion on #93