Revision user incorrectly appears as anonymous user when node author is cancelled

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

Retested and can confirm the issue.

At this time we will need a D10 version of the patch.

Elevated to major as this leads to data lose.

Patch for 10.1.x.

public function userRevisionAuthorRevisionIds(AccountInterface $account)
1. Should be typehinted as a new function

2. Both spots

Did not test yet.

Just trying to help you out here @smustgrave, the following patch address #44. I've also took a look at the testing part and it seems good to me.

Forgot to move to NR.

Thanks @murilohp just retested this and it's working as expected.

And change requested in #44 was addressed.

The last submitted patch, 45: 2977362-45.patch, failed testing. View results →

Random ckeditor5

The last submitted patch, 45: 2977362-45.patch, failed testing. View results →

Reran tests and they passed.

The patch itself looks fine, but I think we need to consider #35 a bit more - i.e. do we really want to be going through all the revisions that a user created a long time ago and resaving them? There's also #34 and #20 about this patch causing new revisions to be created in some situations - this might need more manual testing with draft + default revisions in history and similar.

That's a good point. I don't think Drupal.org does that as I see anonymous. I actually wonder if the data is being stored correctly and when the revision page loads since it can't load the user (it was deleted) it defaults to anonymous. Wonder if we could have just display as plain text?

I have found the root cause. Node revisions are getting created when save();method is called in function _node_mass_update_helper. The code should only update uid and revision_uid fields of the node entity and shouldn't create new revisions.

Solution: Disable creation of revision on node save.

Please test my patch.

Can the MR be updated for 11.x or new one started as that’s the current development branch. Issue would be backported

Yes, let me add for 11.x as well

Switched branch to 11.x

Can issue summary be updated with new solution.

Also will need test coverage.

I found 🐛 Content moderation can wrongly set old revisions as default when they're resaved Needs review which asserts from several people the problem mentioned in #29 (and #20 I guess).

I added the data loss tag to this ticket since our node_field_data table was showing the UID as 0.

Same problem here on Drupal 9.5.10.

There are two different issues here.

• Issue 1: as described in this issue: the Revision user incorrectly appears as an anonymous user when the node author is canceled.
• Issue 2, referenced in comment this comment 🐛 Revision user incorrectly appears as anonymous user when node author is cancelled Needs work , is related to the duplication of old revisions and set as default.

The first case is related to what was pointed out in the task description and I think the proper approach is the previous patch 🐛 Revision user incorrectly appears as anonymous user when node author is cancelled Needs work . Therefore, I am recovering the this approach in the MR "2977362-incorrect-user-anonymous".

The second case, maybe is related to whether the Content Moderation module is active, which is being discussed in this issue https://www.drupal.org/project/drupal/issues/3089747 🐛 Content moderation can wrongly set old revisions as default when they're resaved Needs review and there is a proposed solution.

Cancelled -> Canceled

Tests fixed
Tested on local. RTBC.

Adding PHPUnit tests to the issue https://www.drupal.org/project/drupal/issues/3089747 🐛 Content moderation can wrongly set old revisions as default when they're resaved Needs review we found that:

• First user creates a content published
• Second user creates a translation over the same content published
• First user is canceled
• The translation is not accessible

We added the test to this branch to confirm that happens on a clear Drupal.

Upload only the PHPUnit test to see the differences with and without the fix.

The last submitted patch, 73: core-2977362-content_moderation_anonymous_revision-only_test-73.patch, failed testing. View results →

The result with the fix + PHPUnit test https://www.drupal.org/pift-ci-job/2781901 → is the translation "es" lost?

Testing Drupal\Tests\content_moderation\Functional\ModerationStateNodeTest
....E                                                               5 / 5 (100%)

Time: 01:32.262, Memory: 4.00 MB

There was 1 error:

1) Drupal\Tests\content_moderation\Functional\ModerationStateNodeTest::testUserCancel
InvalidArgumentException: Invalid translation language (es) specified.

/var/www/html/core/lib/Drupal/Core/Entity/ContentEntityBase.php:870
/var/www/html/core/modules/content_moderation/tests/src/Functional/ModerationStateNodeTest.php:310
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

The result without the fix + PHPUnit test https://www.drupal.org/pift-ci-job/2781940 → , here we are not sure why is failing in this step.

Testing Drupal\Tests\content_moderation\Functional\ModerationStateNodeTest
....E                                                               5 / 5 (100%)

Time: 01:36.983, Memory: 4.00 MB

There was 1 error:

1) Drupal\Tests\content_moderation\Functional\ModerationStateNodeTest::testUserCancel
Behat\Mink\Exception\ResponseTextException: The text "First version of the content es." was not found anywhere in the text of the current page.

/var/www/html/vendor/behat/mink/src/WebAssert.php:811
/var/www/html/vendor/behat/mink/src/WebAssert.php:262
/var/www/html/core/modules/content_moderation/tests/src/Functional/ModerationStateNodeTest.php:314
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

On my local with version 10.1.x only with the PHPUnit test, the result is:

wodby@php.container:/var/www/html $  ./vendor/phpunit/phpunit/phpunit web/core/modules/content_moderation/tests/src/Functional/ModerationStateNodeTest.php --filter=testUserCancel
PHPUnit 9.6.13 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\content_moderation\Functional\ModerationStateNodeTest
F                                                                   1 / 1 (100%)

Time: 00:42.279, Memory: 10.00 MB

There was 1 failure:

1) Drupal\Tests\content_moderation\Functional\ModerationStateNodeTest::testUserCancel
Check user on translation.
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'4'
+'0'

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:94
/var/www/html/web/core/modules/content_moderation/tests/src/Functional/ModerationStateNodeTest.php:316
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

So it is possible that this solution is not stable or needs more investigation.

We applied the changes from https://www.drupal.org/project/drupal/issues/3089747 🐛 Content moderation can wrongly set old revisions as default when they're resaved Needs review because have some relation and both patches collide.

The author was not set correctly because on the hook node_user_cancel, when the function node_mass_update is called to update the nodes, it does not take into account the langcode, so sets all revisions uid to 0.

      $nodeStorage = \Drupal::entityTypeManager()->getStorage('node');
      // Reassign node ownership.
      $revision_id = $nodeStorage->userRevisionIds($account);
      if (!empty($revision_id)) {
        node_mass_update($revision_id, ['uid' => 0], NULL, TRUE, TRUE);
      }

When a new translation is created, on the original language is also created a new revision, the method userRevisionIds($account), gets the information from the table 'node_field_revision', and if the deleted user is the author of the original revision, when a new translation is created, also in the original language will be created a new revision, so the method userRevisionIds returns that the user author of the original language is author of both translations.
Example:

MariaDB [drupal]> select nid, vid, langcode, uid from node_field_revision;
+-----+-----+----------+-----+
| nid | vid | langcode | uid |
+-----+-----+----------+-----+
|   1 |   1 | en       |   2 |
|   1 |   2 | en       |   2 |
|   1 |   2 | es       |   3 |
+-----+-----+----------+-----+
3 rows in set (0.016 sec)

We patch the function node_user_cancel to get the author of each translation filtering by the langcode.

      $query =  \Drupal::database()->select('node_field_revision', 'n');
      $query->fields('n', ['langcode']);
      $query->addExpression('GROUP_CONCAT(vid)', 'revisions');
      $query->condition('uid', $account->id());
      $query->groupBy('langcode');
      $result = $query->execute();
      $result->setFetchMode(\PDO::FETCH_ASSOC);
      $revision_list = [];
      foreach ($result->fetchAll() as $value) {
        $revision_list[$value['langcode']] = explode(',', $value['revisions']);
      }

      foreach ($revision_list as $langcode => $revisions) {
        node_mass_update($revisions, ['uid' => 0], $langcode, TRUE, TRUE, TRUE);
      }

Those are the tables node_field_data and node_field_revision before and after the patch:

• Create content moderated "en" published, with user1.
• Create a translation "es" published with user2

Before applying the patch:
Before deleting the user:

MariaDB [drupal]> select nid, vid, langcode, uid from node_field_revision;
+-----+-----+----------+-----+
| nid | vid | langcode | uid |
+-----+-----+----------+-----+
|   1 |   1 | en       |   2 |
|   1 |   2 | en       |   2 |
|   1 |   2 | es       |   3 |
+-----+-----+----------+-----+
3 rows in set (0.016 sec)

MariaDB [drupal]> select nid, vid, langcode, uid from node_field_data;
+-----+-----+----------+-----+
| nid | vid | langcode | uid |
+-----+-----+----------+-----+
|   1 |   2 | en       |   2 |
|   1 |   2 | es       |   3 |
+-----+-----+----------+-----+
2 rows in set (0.001 sec)

After delete the user:
MariaDB [drupal]> select nid, vid, langcode, uid from node_field_revision;
+-----+-----+----------+-----+
| nid | vid | langcode | uid |
+-----+-----+----------+-----+
| 1 | 1 | en | 0 |
| 1 | 2 | en | 0 |
| 1 | 2 | es | 0 |
+-----+-----+----------+-----+
3 rows in set (0.016 sec)

MariaDB [drupal]> select nid, vid, langcode, uid from node_field_data;
+-----+-----+----------+-----+
| nid | vid | langcode | uid |
+-----+-----+----------+-----+
| 1 | 2 | en | 0 |
| 1 | 2 | es | 0 |
+-----+-----+----------+-----+
2 rows in set (0.001 sec)

After applying the patch:
Before deleting the user:

MariaDB [drupal]> select nid, vid, langcode, uid from node_field_revision;
+-----+-----+----------+-----+
| nid | vid | langcode | uid |
+-----+-----+----------+-----+
|   1 |   1 | en       |   2 |
|   1 |   2 | en       |   2 |
|   1 |   2 | es       |   3 |
+-----+-----+----------+-----+
3 rows in set (0.016 sec)

MariaDB [drupal]> select nid, vid, langcode, uid from node_field_data;
+-----+-----+----------+-----+
| nid | vid | langcode | uid |
+-----+-----+----------+-----+
|   1 |   2 | en       |   2 |
|   1 |   2 | es       |   3 |
+-----+-----+----------+-----+
2 rows in set (0.001 sec)

After deleting the user:
MariaDB [drupal]> select nid, vid, langcode, uid from node_field_revision;
+-----+-----+----------+-----+
| nid | vid | langcode | uid |
+-----+-----+----------+-----+
| 1 | 1 | en | 0 |
| 1 | 2 | en | 0 |
| 1 | 2 | es | 3 |
+-----+-----+----------+-----+
3 rows in set (0.016 sec)

MariaDB [drupal]> select nid, vid, langcode, uid from node_field_data;
+-----+-----+----------+-----+
| nid | vid | langcode | uid |
+-----+-----+----------+-----+
| 1 | 2 | en | 0 |
| 1 | 2 | es | 3 |
+-----+-----+----------+-----+
2 rows in set (0.001 sec)

The tests are failing on CI, but not in local.
We executed the same tests and is not failing.

We mounted the project using ddev and require drupal core 10.1.x-dev

    "require": {
        "composer/installers": "^2.0",
        "cweagans/composer-patches": "^1.7",
        "drupal/core": "10.1.x-dev as 10.1.5",
        "drupal/core-composer-scaffold": "^10.1",
        "drupal/core-project-message": "^10.1",
        "drupal/core-recommended": "^10.1",
        "drush/drush": "^12.4"
    },

Why the results are not the same?

Drupal is run in a subdirectory for DrupalCI and GitLab CI so paths will be relative to them.

I think, from seeing the MR, if you change $this->drupalGet('/es/' . $node->toUrl()->toString()); for $this->drupalGet($node->toUrl('canonical', ['language' => 'es'])->toString());, things should work locally and on the CI systems.

Thank you @fjgarlin seems working on https://git.drupalcode.org/issue/drupal-2977362/-/jobs/235674
We tried with "$this->drupalGet('es/node/' . $node->id());",

Eduardo Morales Alberti → closed merge request !5133

Upload the patch with only the PHPUnit tests

The only PHPUnit tests patch gives the following errors:
https://www.drupal.org/pift-ci-job/2797148 →

• Translation es disappear when the author of the original content is deleted

Testing Drupal\Tests\content_moderation\Functional\ModerationStateNodeTest
....F                                                               5 / 5 (100%)

Time: 01:23.869, Memory: 4.00 MB

There was 1 failure:

1) Drupal\Tests\content_moderation\Functional\ModerationStateNodeTest::testUserCancel
User with uid 4should have 1 rows on table node_field_data and language es
Failed asserting that '0' matches expected 1.

• This error can be ignored because the method does not exists on the patch only tests

Testing Drupal\Tests\node\Kernel\NodeStorageTest
.E                                                                  2 / 2 (100%)

Time: 00:02.821, Memory: 4.00 MB

There was 1 error:

1) Drupal\Tests\node\Kernel\NodeStorageTest::testUserRevisionAuthorRevisionIds
Error: Call to undefined method Drupal\node\NodeStorage::userRevisionAuthorRevisionIds()

• When the author of the original language is removed, all revisions authors are set to anonymous

Testing Drupal\Tests\node\Kernel\NodeUserCancelTest
F                                                                   1 / 1 (100%)

Time: 00:01.495, Memory: 4.00 MB

There was 1 failure:

1) Drupal\Tests\node\Kernel\NodeUserCancelTest::testCancelReassignToAnonymous
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'2'
+'0'

Ready to review

Nightwatch failed on the merge request, but We are not sure why https://git.drupalcode.org/issue/drupal-2977362/-/pipelines/44783 and we are not sure if is related to the MR.

  ️TEST FAILURE (4m 40s):  
   - 1 assertions failed; 1905 passed
   ✖ 20) Tests/a11yTestAdmin
   – Accessibility - Admin Theme: Structure | Block (2.449s)
   → ✖ NightwatchAssertError
   aXe rule: landmark-contentinfo-is-top-level - Contentinfo landmark should not be contained in another landmark
	In element: .messages-list__item

1 nitpicky issue.

FYI you don't need test-only patch anymore there is a test-only feature in gitlab that can be ran.

Reran the nightwatch and it was indeed a random.

@smustgrave, sorry I did not understand your question about _node_mass_update_helper. Should we remove or replace the function? what do you mean?

Oh I was just noting other modules that may call this. Nope you're good to leave as is.

Thank you @smustgrave, there is something left?

Left two small nitpicky things, going to leave in review for others to comment too.

Hiding the patches and leaving a note in the issue summary the fix is in MR4713

Changes proposed by @smustgrave applied.

Circling back to this one

Ran test-only feature again

There was 1 failure:
1) Drupal\Tests\content_moderation\Functional\ModerationStateNodeTest::testUserCancel
User with uid 4should have 1 rows on table node_field_data and language es
Failed asserting that '0' matches expected 1.
/builds/issue/drupal-2977362/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:94
/builds/issue/drupal-2977362/core/modules/content_moderation/tests/src/Functional/ModerationStateNodeTest.php:347
/builds/issue/drupal-2977362/core/modules/content_moderation/tests/src/Functional/ModerationStateNodeTest.php:305
/builds/issue/drupal-2977362/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
FAILURES!

Tested following the steps and MR did address the issue.

Learned recently data loss is a critical issue. Will try and get some traction in slack

Mostly minor feedback

We will try to review the minor changes this week.
Thank you acbramley

Waiting for tests.

All points suggested by acbramley were addressed.

Eduardo Morales Alberti → changed the visibility of the branch 2977362-incorrect-user-anonymous to hidden.

Eduardo Morales Alberti → changed the visibility of the branch 2977362-revision-user-incorrectly to hidden.

Eduardo Morales Alberti → changed the visibility of the branch 2977362-10.x to hidden.

Eduardo Morales Alberti → changed the visibility of the branch 2977362- to hidden.

Eduardo Morales Alberti → changed the visibility of the branch 2977362-incorrect-user-anonymous to active.

Added a small nit and a discussion on the function name.

Some grammatical suggestions, but that should not hold this back from being reviewed by others.

Applied @BramDriesen grammar suggestions.

1) Drupal\Tests\node\Kernel\NodeUserCancelTest::testCancelReassignToAnonymous
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'2'
+'0'
/builds/issue/drupal-2977362/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:94
/builds/issue/drupal-2977362/core/modules/node/tests/src/Kernel/NodeUserCancelTest.php:77
/builds/issue/drupal-2977362/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
FAILURES!
Tests: 1, Assertions: 10, Failures: 1.

Verified the test coverage.

Went through the threads and left a checkmark on what's been addressed, which is everything.

Still verified the issue on 11.x and that the MR addresses it.

RTBC +1 for me.

Verified the fix on Drupal 11 setup as per the steps to reproduce.
-Created content using user 1.
-Edited the content using user 2.
-Deleted user 1 using user 3.
-Verified the revision of the content. Working as expected.

Needs work for @lleber's comment.

Believe all feedback has been addressed.

Added some feedback to the MR.

I merged the #45 and #57 patches, create a new patch for Drupal 10.1.4
it's work for me.

All work is being done in the MR, it's much easier for reviewers if we do not confuse things with additional patches.

Composer patches support local files, you do not need to upload them to an issue.

I tried to apply all the code modified by MR4713 in drupal 10.1.8 version and found that when selecting "Delete the account and make its content belong to the Anonymous user. This action cannot be undone. Reassign its groups to the super administrator." The created node by this user will also be deleted.Is there any new progress regarding this ticket?

I updated the patch from #116 to work with Drupal 10.3.0

See #117

Issue ✨ Allow user admins to choose the user cancellation method on the user deletion confirmation form (rather than having a single sitewide setting) Needs work could help to limit the options