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

Some rough conflicts in here that need resolving.

Reroled and refactored the fix.
Updated tests.
Also, provided patch files for different versions,

P.S. There is also a related bug described here 🐛 Skip changing 'revision translation affected' field if the old revision is syncing Needs work . Without that patch, a revision_translation_affected property might be changed, which can cause the issue when we see a revision of the other(unexpected) language in the revisions list.

Steps to reproduce:

• User A - created a node EN - revision 1
• User B - created a translation ES - revision 2
• User A - updated a translation EN - revsion 3
• Revision list:
  • EN - revisions 1 and 3
  • ES - revision 2
• Remove User B
• Revision list:
  • EN - revisions 1, 2, 3
  • ES - revision 2

We need to move these new methods somewhere else, NodeStorage is in the process of being deprecated. 📌 Create a new NodeRepository for the code from Drupal\node\NodeStorage Active

We also should be using entity queries instead of direct db queries.

Hi @acbramley
I understand your point, but:
- That ticket is still in progress, so it doesn't make sense to move the functions elsewhere for now, because it's still a big question which of the tasks will be solved first.
Since there will be a merge conflict here, the task that will merge last will fix it. So, we can ignore it for now.
- A similar opinion as above, since all class methods currently use db queries, it should be the same here. Their refactoring will be part of #3396062 or will happen here if that task is merged first.
Especially since we can't use entity queries to get these values. It will at least be select queries

Conclusion: Your comment cannot be a blocker for this ticket.

Going to agree with with 127, but can leave in review for others

so it doesn't make sense to move the functions elsewhere and refactor them for now

It doesn't make sense to add new methods to an interface/class that we know is being deprecated. We can postpone this issue on that one instead if you like.

since all class methods currently use db queries, it should be the same here

That code is ancient and is part of the reason why that class is being deprecated, it's not compatible with all db engines. We should do it properly first since we're introducing new functionality, not leave it to a follow up to refactor again.

@acbramley

We need to move these new methods somewhere else

It doesn't make sense to add new methods to an interface/class that we know is being deprecated. We can postpone this issue on that one instead if you like.

Where exactly to move?
My opinion is that new methods must be in the same place where revisionIds() is located, which is logical, but I'd be happy to hear another suggestion.
And I'm not sure it's right to postpone a critical issue because of a minor one.

That code is ancient and is part of the reason why that class is being deprecated, it's not compatible with all db engines. We should do it properly first since we're introducing new functionality, not leave it to a follow up to refactor again.

But it's not a deprecated class yet, we just have a ticket to make it so, and it's a big question when that ticket will be resolved. And which of the tickets will be solved first.
If this one is merged first, then that one will be refactored according to 11.x. If that one will be merged first, then this one will be refactored.
For example, we had to refactor this one after merging the 📌 OOP hooks using event dispatcher Needs review

So I see the following ways:
1. Keep it as it is, cuz the other ("normal" priority) refactoring ticket cannot block the "critical" one, this issue breaks the content on the site and must be fixed asap. Later in the context of #3396062 the code will be refactored
2. To locate new methods in the same place as a revisionIds() method, we can make a separate branch as a combination of #2977362 and #3396062 + keep the current one. The two branches should always be up to date. But I don't think this is the right way to combine the two tickets, because it could delay the resolving of a critical issue, but this is a possible way.

I would like to hear other opinions. Maybe even in the form of a vote :)