Problem/Motivation

If you have a site where the comments are translateable and your user cancel method is 'user_cancel_reassign', then in case if a user is being cancelled and it has comments which are also translated, those comments will be deleted.

Steps to reproduce

Set user cancel settings to user_cancel_reassign.
Have a node where the comments can be translated.
Create a user.
Comment with that user.
Translate with that user (or probably any other user).
Then cancel this user account with user_cancel_reassign.

Proposed resolution

Reassign the translations too to Anonymous user, not just the original comment (this is what the node module does too).

The problem lays in the comment_user_predelete() function, where the

  $entity_query = \Drupal::entityQuery('comment');
  $entity_query->condition('uid', $account->id());
  $cids = $entity_query->execute();

will find the translated comments too, therefore when user is being deleted, the comment will die with it.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

golddragon007 created an issue. See original summary.

golddragon007’s picture

golddragon007’s picture

Status: Active » Needs review
larowlan’s picture

mohit_aghera’s picture

@larowlan
I've uploaded the initial patch to test the comment translation delete.
I checked the node module and didn't found any test cases to validate node translation updates.
Do you think it would be helpful to add those in this issue?

henry.odiete’s picture

Reviewed the test and can confirm the patch provided works for comments. I don't see why not for the node module. though it's not part of the issue

henry.odiete’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs tests

The fix itself looks fine.

Some minor comments on the test coverage, and it would also be good to have a test-only patch here to show the test failure.

  1. +++ b/core/modules/user/tests/src/Functional/UserCancelTest.php
    @@ -608,4 +609,84 @@ public function testUserDeleteWithContentAndNodeAccess() {
    +
    +    // Create a user.
    +    $account = $this->drupalCreateUser(['cancel account']);
    +    $this->drupalLogin($account);
    +    // Load a real user object.
    +    $user_storage->resetCache([$account->id()]);
    +    $account = $user_storage->load($account->id());
    +
    +    // Create a simple node.
    +    $node = $this->drupalCreateNode(['uid' => $account->id()]);
    +
    

    All of this code is self-explanatory and would be fine without a comment.

  2. +++ b/core/modules/user/tests/src/Functional/UserCancelTest.php
    @@ -608,4 +609,84 @@ public function testUserDeleteWithContentAndNodeAccess() {
    +    $test_comment = $storage->load($comment->id());
    +    $this->assertTrue(($test_comment->getOwnerId() == 0 && $test_comment->isPublished()), 'Comment of the user has been attributed to anonymous user.');
    +    $this->assertEqual($test_comment->getAuthorName(), $anonymous_user->getDisplayName(), 'Comment of the user has been attributed to anonymous user name.');
    +    $comment_translation = $test_comment->getTranslation('ur');
    

    The first assertion could be split into two separate assertions - i.e. first test the owner ID is 0, then in a separate assertion, test that the comment it published.

    The assertEqual() call doesn't need a custom message.

ravi.shankar’s picture

Status: Needs work » Needs review
FileSize
5.68 KB
2.33 KB
4.52 KB

Tried to address comment #8, please reivew.

Status: Needs review » Needs work

The last submitted patch, 9: test_only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Matroskeen’s picture

Status: Needs work » Needs review
FileSize
5.68 KB

Just re-uploading a patch from #9, so it stops failing and moving back to NW :)
[please don't credit me for that]

catch’s picture

Status: Needs review » Needs work
+++ b/core/modules/user/tests/src/Functional/UserCancelTest.php
@@ -678,11 +675,12 @@ public function testUserAnonymizeTranslations() {
-    $this->assertEqual($test_comment->getAuthorName(), $anonymous_user->getDisplayName(), 'Comment of the user has been attributed to anonymous user name.');
+    $this->assertTrue($test_comment->getOwnerId() == 0);

This can be $this->assertEqual($test_comment->getOwnerId(), 0); now that it's been split.

Matroskeen’s picture

Version: 8.9.x-dev » 9.2.x-dev
Assigned: Unassigned » Matroskeen
Matroskeen’s picture

Assigned: Matroskeen » Unassigned
Status: Needs work » Needs review
FileSize
5.57 KB
4.18 KB

I noticed some additional issues like:
1) Unused variable;
2) Deprecated asserts;
3) Unnecessary t() calls;

Here is the patch with a clean-up. Let's see.

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

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.

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

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.

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.

ranjith_kumar_k_u’s picture

Re-reolled #14 for 9.5

Status: Needs review » Needs work

The last submitted patch, 18: 3166561-18.patch, failed testing. View results

andregp’s picture

Status: Needs work » Needs review
FileSize
5.48 KB
1.72 KB

Fixed failing test from #18

victorml’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed and tested. Works perfectly fine.

  • catch committed 1b0253b on 10.1.x
    Issue #3166561 by Matroskeen, ravi.shankar, andregp, mohit_aghera,...

  • catch committed 70f4990 on 10.0.x
    Issue #3166561 by Matroskeen, ravi.shankar, andregp, mohit_aghera,...
  • catch committed 16fc07e on 9.5.x
    Issue #3166561 by Matroskeen, ravi.shankar, andregp, mohit_aghera,...
catch’s picture

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

Committed/pushed to 10.1.x, cherry-picked to 10.0.x and 9.5.x.

Leaving RTBC against 9.4.x since we just did the patch release, will cherry-pick there once it's unfrozen.

  • xjm committed 7c56156 on 9.4.x authored by catch
    Issue #3166561 by Matroskeen, ravi.shankar, andregp, mohit_aghera,...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

The aforementioned cherry-pick never happened, so cherry-picked and pushed now after a different commit freeze on 9.4.x. :)

Status: Fixed » Closed (fixed)

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