User performing revert should be set as revision author (revision_uid)

Looks good, but we need some tests to make sure it does what it says, and make sure we don't get any future regressions.

Thanks @trwill, moving back to needs work for #7.

Looks good now, thanks @trwill.

I think this change makes sense but given the behaviour change it'd be great to have a +1 from an entity API or node maintainer.

@timmillwood actually is a node module maintainer (I didn't know we even have that again :)), so it already had that.

I also agree with the change, however, we also need to update NodeRevisionRevertTranslationForm, as that is used when reverting a translation and the two should work consistently.

My comment was wrong, I accidently looked at an 8.4 code base, the logic isn't duplicated anymore in 8.5.

However, I think we should have test coverage for that, we seem to be testing translation revision revert in \Drupal\node\Tests\NodeRevisionsTest::testRevisionTranslationRevert(), should be easy to add another line there.

As you say in #15 NodeRevisionRevertTranslationForm extends NodeRevisionRevertForm and use the same submit handler, but yes, test coverage would be good.

Also, yes, I am Node maintainer, after the shock of seeing Node didn't have a maintainer I volunteered as I seem to be in and out of Node module with Workflow issues.

Yeah, its interesting how we didn't have a node maintainer for a very long time, good thing it's not a very important piece of Drupal, right ;) Even better that it's an exception and that no other equally unimportant parts like the base system, system.module or the database layer also don't have any maintainers :)

You could almost be British with that sarcasm level :D

Looks we don't have separate users here? So you probably need to create another and log in with that for the revert.

Also, you shouldn't add additional test runs unless there is a specific reason to, like something that is PHP/database backend specific. Each test run costs ~1h hour of processing resources.

+++ b/core/modules/node/src/Tests/NodeRevisionsTest.php
@@ -179,6 +179,10 @@ public function testRevisions() {
     $reverted_node = $node_storage->load($node->id());
     $this->assertTrue(($nodes[1]->body->value == $reverted_node->body->value), 'Node reverted correctly.');
+    // Confirm the revision author is the user performing the revert.
+    $this->assertTrue($reverted_node->getRevisionUserId() == $this->loggedInUser->id(), 'Node revision author is user performing revert.');
+    // And that its not the revision author.
+    $this->assertTrue($reverted_node->getRevisionUserId() != $nodes[2]->getRevisionUserId(), 'Node revision author is not original revision author.');

Hm, I'm a bit confused.

The assert right above your code also checks for $nodes[1]. So it seems that piece expects that the reverted node is in fact $nodes[1] ?

I think it'd be good to identify the issue spotted in #26, but I expect this can be done in a follow up.

Committed and pushed f7cd4d7d94 to 8.6.x and caa2970238 to 8.5.x. Thanks!