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.We should avoid deleting data on user cancel.
To quote from the Drupal API https://api.drupal.org/api/drupal/modules%21user%21user.api.php/function...
"the module should either do nothing, unpublish content, or anonymize content" ... it should not be deleting user data.
Right now we have this code removing all the flag when user cancel their account.
/**
* Implements hook_user_cancel().
*/
function flag_user_cancel($edit, $account, $method) {
flag_user_account_removal($account);
}
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | 2489992-4.patch | 454 bytes | casey |
| |||
| #2 | 2489992-2.patch | 374 bytes | michaellenahan |
| |||
Comments
Comment #1
michaellenahan CreditAttribution: michaellenahan at erdfisch commentedI agree ... cancelling the user account is not the same as deleting it.
Comment #2
michaellenahan CreditAttribution: michaellenahan at erdfisch commentedHere's an initial, very rudimentary patch.
A more complete patch should take into account the $method argument.
For the various possibilities of the $method argument, and their meanings, see: https://api.drupal.org/api/drupal/modules%21user%21user.pages.inc/functi...
See https://api.drupal.org/api/drupal/modules%21comment%21comment.module/fun... for an example that uses a switch statement to handle the various methods differently.
Comment #3
joachim CreditAttribution: joachim as a volunteer commented> "the module should either do nothing, unpublish content, or anonymize content" ... it should not be deleting user data.
Taking these options one by one:
- do nothing
That leaves flag counts that look incorrect.
- unpublish content
Can't do that, there is no concept of a flagging being unpublished
- anonymize content
We don't have a session ID we can set on the flaggings, and anon flagging might not be enabled.
Comment #4
casey CreditAttribution: casey at SWIS commentedComment #5
eelkeblokI am seeing this functionality actually causing issues on cancelling a user that has a lot of flaggings. I think blanket deleting all flaggings whenever hook_user_cancel is called is not the right approach;
That may also end up being less work, because by then a lot of content created by the user may have been deleted as well, and thereby also any associated flaggings, through the implementation of hook_entity_predelete() (which is likely to have happened in a batch, for properly implemented cancellation methods).This tells me there is actually no need for a hook_user_cancel() implementation in Flag, and it is in fact only causing problems.
Of course, please tell me if I'm missing something. Setting back to "Needs review" to raise the awareness again.
Edit: actually, the idea that a lot of flaggings would have been deleted by deleting the user's content is probably invalid, as flaggings mostly would apply to content created by others.
Comment #6
eelkeblokBTW, if it is determined deleting flaggings is the right approach in some cases (maybe when the user is deleted but the content is re-assigned, so there will not actually be many content-related unflaggings) , it would be a good idea to do so by adding to the cancellation batch (it is not a very well documented feature that when hook_user_cancel() is called, there is actually a batch standing by which can be added to by calling batch_set() with an array that contains some extra operations). We may still get in trouble when trying to delete the flaggings at the very end when the user is deleted and there is still a large number left, at that point.