Add tests for reverting revisions where revision_uid and uid differ

This will be necessary for DX and if #218755: Support revisions in different states is to move forward. It may not be necessary if we abandon node "properties" in favor of fields everywhere.

I'll take a look at a patch.

And, for safekeeping, here are the missing columns:

Columns in {node} not in {node_revision}

* type
* langcode
* created
* changed -- which is roughly analagous to "timestamp" in {node_revision}
* tnid
* translate

Columns in {node_revision} not in {node}

* timestamp
* log

Proposal:

Add these fields to {node_revision}

* type
* created
* langcode
* tnid
* translate
* revision_uid -- the user id of the user who made this revision, not the owner of the node. A case can be made for adding "author_uid" instead, in order to make for less code change.

Ignore the following:

* log -- only relevant to revision listings.
* changed -- has a slightly different meaning in each table. But perhaps we should change "timestamp" in the {node_revision} table to match.

For consistency, I would change {node_revision}.timestamp to {node_revision}.changed.

Right now the node controller does this:

  // Ensure that uid is taken from the {node} table,
  // alias timestamp to revision_timestamp and add revision_uid.
  $query = parent::buildQuery($ids, $conditions, $revision_id);
  $fields =& $query->getFields();
  unset($fields['timestamp']);
  $query->addField('revision', 'timestamp', 'revision_timestamp');
  $fields['uid']['table'] = 'base';
  $query->addField('revision', 'uid', 'revision_uid');
  return $query;

... and even with the code comment I have no idea what's the purpose of this. It would be useful to either have a rule of how the revision properties end up in the $node object or just have different names for them.

Is this issue still needed? given:
* #218755: Support revisions in different states
* #1612014: Create an interface for revisionable entities
* #1346214: [meta] Unified Entity Field API (re #1)
* #1082292: Clean up/refactor revisions

Well, we still have a {node_revision} table...

Is there an issue to kill that table as part of the entity overhaul?

I've created the following tasks:
#1838884: Add 'created' and 'changed' columns to node_revision table
#1838862: Add revision_uid to the node table

Since this is irretrievable data-loss I'm going to bump it to major.

Steps to reproduce, logged in as user 1 works fine for this.

1. Create a node with user A (current user is fine)
2. Edit the node, change authoring information to user B, check the 'new revision' checkbox.
3. Revert to the revision created with #1.
4. Revert to the revision created with #2.

Now there's two issues:
1. user A is still listed as the author of the node, but it should be user B.
2. There is no trace of user B ever being the author of the node in the node_revisions table or anywhere else.

@jstoller do you think this needs to be done in two issues, or could we add all the needed columns here? The changes are going to be very similar so I think one issue is fine.

Please, keep in mind #1498674: Refactor node properties to multilingual and http://drupal.org/node/1722906 while working on this.

This is also somehow related: #1833334: EntityNG: integrate a dynamic property data table handling.

@catch, I thought the separate tasks might be useful, but given there's been zero movement on either of them since I created them, I have no problem with moving the work here. As long as there's someone willing to write the patch, I'm a happy camper. :-)

Another issue which may be of interest here is #1838918: Add 'published' timestamp to nodes. It's had a working patch for several week, so will hopefully be committed, though recently there were some requested changes to the update function. That patch adds a {node}.published column, but does not touch the {node_revision} table. This was by design, though I wouldn't have a problem with adding it to revisions as well, if people think that's important.

I followed up on #1838884: Add 'created' and 'changed' columns to node_revision table #1838862: Add revision_uid to the node table both. I need UI guidance.

@chx I've marked the two sub-issues duplicate, it's just one table to change the schema of and some supporting code so not sure sub-issues help.

On the UI, I don't think we need to change anything - the one exception is that where we currently show the revision uid in the UI when listing revisions, that should stay the same (but point to the new revision_uid column - i.e. keep pointing to the person who authored the revision).

Tagging with beta blocker - if we want to get this into 8.x, it'd ideally go in before we have to support minor updates from 8-8. Tempted to move this to critical since we lose the node author when restoring revisions (if it's different from the revision author) which is nasty and strange data loss, but it's a long standing bug across several major releases so dunno.

For columns like {node}.type that truly apply at a node level, does it still make sense to duplicate the data in {node_revision}? Is it within the realm of possibilities that some contrib module might want to enable revisions of different types? And if not, are there any other benefits to adding such a column?

Node type I think could be left off.

Can we get a summary of what is needed for this issue? It seems many if not all of these columns have been added to hook_schema unless I'm misunderstanding this issue.

Are we sure this still makes sense after #2057401: Make the node entity database schema sensible? Right now all node base fields except revision metadata are revisionable.

Yes I think you're right that this fixed it - these are in node_field_data now. We might want to keep this open for adding some tests that reverting a revision restores the correct uid.

Those tests are needed, because the mapping there is currently quite broken I think. See also #2031183: Improve test coverage for node authored on widget

Will try to work on this.

Given that this is just about writing tests now, I don't think this is a beta blocker anymore?

Removing tag for now and moving to the node component.

Working on tests...

I have updated the issue summary, and attach a proposed patch adding tests to ensure that node and revision author information is properly preserved across reversions.

This is my first attempt at writing tests, so rip away at it!

Ready for consideration by other reviewers; added beta phase evaluation to issue summary.

The tests in #25 are looking good.

Some coding standard nitpicks:

1. +++ b/core/modules/node/src/Tests/NodeRevisionsAuthorTest.php
   @@ -0,0 +1,157 @@
   +    $user_permissions = array (
   

   array ( should just be array(.

2. +++ b/core/modules/node/src/Tests/NodeRevisionsAuthorTest.php
   @@ -0,0 +1,157 @@
   +    // Create initial node (author: $user1)
   

   Comment needs to end with a period.

3. +++ b/core/modules/node/src/Tests/NodeRevisionsAuthorTest.php
   @@ -0,0 +1,157 @@
   +                      'The revised node has the correct author.');
   ...
   +		      'The revised node has the correct revision author.');
   ...
   +			   array (
   +                             '!nid' => $node->id(),
   +                             '!orignid' => $this->originalNode->getRevisionid()
   +                           ));
   ...
   +		       array('@type' => 'Basic page', '%title' => $this->originalNode->label(),
   +                              '%revision-date' => format_date($this->originalNode->getRevisionCreationTime()))), 'Revision reverted.');
   ...
   +			   array (
   +                             '!nid' => $reverted_node->id(),
   +                             '!revisednid' => $this->revisedNode->getRevisionid()
   +                           ));
   ...
   +		       array('@type' => 'Basic page', '%title' => $this->revisedNode->label(),
   +                              '%revision-date' => format_date($this->revisedNode->getRevisionCreationTime()))), 'Revision re-reverted.');
   

   Whitespace/indentation issues.

Since #1838862: Add revision_uid to the node table was marked as a duplicate, this is no longer just about adding tests, but also about tracking revision_uid.

Rerolled #25 taking feedback from #28 into account.

I think the issue summary is actually fine. My comment in ##2 didn't take into account that we are already tracking revision_uid in the node_revision table.

I added #2495297: [regression] Display revision_uid editor name if different from uid on node revision overview page, which is related to this in that these tests ensure the current functionality.

The old patch still referenced the now-removed Drupal\Core\Utility\String class.

The two fails were due to #2383015: Revert back is redundant landing.

Looks like this has not yet been committed. Not sure if this is directly related to what is trying to be tested here, but I am seeing an issue with uid and revision_uid: #2977362: Revision user incorrectly appears as anonymous user when node author is cancelled

Does appear to be a duplicate of #2977362: Revision user incorrectly appears as anonymous user when node author is canceled could anyone confirm?