User associated contents are deleted

Is this still an issue in 8.x-3.x? If yes, we should switch over to that latest version.

This seems to be still an issue we recently patch this for one of our website and it seems to be working pretty well. I thought I'd share it here. Let me know your thoughts. The logic is quite straight forward and is running to updated queries to reassign the user ID before the user gets deleted.

@samuelpodina my concerne here would be, that we'd have to care for all kind of entities, not only nodes? Or are only nodes auto-deleted? What about other (core & conbtrib) entity types?

I guess this needs a core solution and we should just trigger the interface function of that...

I couldn't really recreate this issue, using the current 8.x-3.x dev version. Maybe I did something wrong as the selected option is in french.

Steps I did to reproduce this issue:

• Create a new account, "Test User"
• Login as "Test User" and create an article
• Go to the Database and set the "login" timestamp of the "Test User" to "1651743908" (Thu May 05 2022 09:45:08 GMT+0000)
• Login as Admin, go to the purge-rule page and set the following configurations:
• Press "Purge users now"
• The "Test User" was purged, but its content is still available and published by "Anonymous" as expected.

@Anybody and I talked about this internally, and we originally thought the "When cancelling a user account" setting under "/admin/config/people/accounts" might be overriding this module's behavior, but I haven't touched this setting, and it is still set to "Disable the account and keep its content.".

Any ideas on how to properly reproduce this issue, as the issue summary doesn't really give any proper reproduction guidelines?

@matoeil: Could you please explain the steps to reproduce this in a clean environment or provide tests to reproduce it?
Once we're able to reproduce this, we can find a fix. The patches above are not appropriate sadly.

@Anybody and I talked about this internally, and we originally thought the "When cancelling a user account" setting under "/admin/config/people/accounts" might be overriding this module's behavior, but I haven't touched this setting, and it is still set to "Disable the account and keep its content.".

@matoeil what's your "When cancelling a user account" setting at "/admin/config/people/accounts"?

Thank you very much for your investigations @Grevil!

@matoeil
@adstokoe

to get this fixed ASAP: Did you also run into this issue? If yes, could you please tell the steps to reproduce this from scratch? Thanks!

After more investigations I think content gets delete when a users own more then 10 content items. For example node entity switches to the Batch api if there more then 10 nodes checkout /core/modules/node/node.module line 651 is calling the node_mass_update method. I believe the batch api need browser access to work that's why when you delete a user with less then 10 items it works. I think we need to come up with a different approach to reassign content, maybe a hook_user_predelete.

@samuelpodina thanks! Would you agree it might be caused by that system setting?

@Grevil could you please retry the steps from #20 with 11 existing nodes? Then we could eventually even have a relatively simple test for this?

@anybody I don't think the system setting has any impact on this since this modules invokes the user_cancel with the correct method selected in the purge_users module config.

@samuelpodina thanks! So any ideas what might then cause this in the batch as root-cause?
#24 would explain why contents are not updated (not processed), but why are they deleted?

@anybody because all of this happens before the predelete hook is called which means all nodes that haven't been processed/reassigned go through node_user_predelete which runs the following code:

/**
 * Implements hook_ENTITY_TYPE_predelete() for user entities.
 */
function node_user_predelete($account) {
  // Delete nodes (current revisions).
  // @todo Introduce node_mass_delete() or make node_mass_update() more flexible.
  $nids = \Drupal::entityQuery('node')
    ->condition('uid', $account->id())
    ->accessCheck(FALSE)
    ->execute();
  // Delete old revisions.
  $storage_controller = \Drupal::entityTypeManager()->getStorage('node');
  $nodes = $storage_controller->loadMultiple($nids);
  $storage_controller->delete($nodes);
  $revisions = $storage_controller->userRevisionIds($account);
  foreach ($revisions as $revision) {
    node_revision_delete($revision);
  }
}

I think I found a better way to cover all types of entities not just node. I created an new patch let me know what you think.

Switched to dependency injection usage for entity type manager and db connection.

@samuelpodina thank you very much, nice additions and findings!

Could you please add this as MR to make review easier?

Furthermore this line should please be removed for code style reasons:
Kudos to Clive https://drupal.stackexchange.com/a/310613

Would comments make sense at some lines which are not clear without?

Applied the patch by @samuelpodina. I'll test the behaviour before and after the patch and review the code, so we can fix this ASAP!

FYI, 1 failing test should be unrelated, as it currently also fails in dev.

Interesting, I couldn't even delete the content, when the user has more than 10 contents created.

An AJAX HTTP error occurred.
HTTP Result Code: 500
Debugging information follows.
Path: /batch?id=2&op=do_nojs&op=do
StatusText: error
ResponseText: The website encountered an unexpected error. Please try again later.TypeError: _node_mass_update_helper(): Argument #1 ($node) must be of type Drupal\node\NodeInterface, null given, called in /var/www/html/web/core/modules/node/node.admin.inc on line 133 in _node_mass_update_helper() (line 82 of core/modules/node/node.admin.inc). _node_mass_update_batch_process(Array, Array, NULL, 1, 1, Array) (Line: 295)
_batch_process() (Line: 137)
_batch_do() (Line: 93)
_batch_page(Object) (Line: 55)
Drupal\system\Controller\BatchController->batchPage(Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 580)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 163)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 74)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 42)
Drupal\tracer\StackMiddleware\TracesMiddleware->handle(Object, 1, 1) (Line: 34)
Drupal\webprofiler\StackMiddleware\WebprofilerMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 686)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

@grevil this happened when you chose the option to delete the user and it's content?

@samuelpodina without the patch. I will investigate further.

Ok, so basically WITHOUT the patch applied it will throw the error mentioned in #36 and delete all the user's content + the user itself.

Patch looks and works great! I just removed some duplicate code and, added proper method docs and fixed some phpcs issues.

At first, I thought "getTablesWithUidColumn()" might be a bit error-prone, as we are getting ALL tables containing a "uid" on the given entity type storage object. But since we are making sure, that the entity type is an instance of "ContentEntityTypeInterface", I think this should be just fine! :)

Let's have @Anybody the last word here and I will create tests in the meantime.

But from my side, LGTM!

FYI, here is a diff between the original patch and my refactoring: https://git.drupalcode.org/project/purge_users/-/merge_requests/17/diffs...

@grevil thank you for taking the time to review, test and refactor

Thank you very much for working in this @samuelpodina and @Grevil very much appreciated!

To be honest I don't really like the custom SQL solution due to its risks and untypical solution in the context of Drupal. But to be honest, I have no better idea!

So I added the following to the module page now:

Important notes:

This module has to use custom logic especially when reassigning content to guest / anonymous.
So please

• test and use the module carefully
• ensure to have regular and working backups
• exclude roles which should never be touched (especially roles with lots of content created)
• report and fix bugs and issues with us

The background is, that there are no good core concepts for some of the cases, which would allow to solve these better.

Any additions?

Furthermore I changed the switch statement a bit to

• better copy code, but have clear break;'s
• move the default: case to the end

As this was clearly broken before and now tested to be working, I think we should merge it to reduce the risk here. Otherwise I would have waited for some weeks to retrieve further feedback, but that doesn't make much sense here.

@Grevil please decide, if we should first fix other issues and make a release before merging this, so one could get back to before this larger change or to merge it directly, before creating a new release.

Thanks!

What the hell is going on with the Jenkins test script... it's updating to composer 3 internally to run the tests?

OK, now tests run properly again, final review please with the added tests and the switch case fix!

Thanks all!

And especially @samuelpodina, clean patch. 👍

Automatically closed - issue fixed for 2 weeks with no activity.