Problem/Motivation

Prior to #2057401: Make the node entity database schema sensible and the implementation of fully revisioned node fields, data loss was possible when reverting between revisions with different node authors. This has been a long-standing source of confusion and bugs through multiple versions of Drupal core. Notwithstanding that this should now be "fixed", tests should be implemented to ensure that node and revision author information is in fact preserved across revisions and reverts.

Per comment #8 by catch below, the typical problem scenario arises when changing author information across revisions and then reverting:

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.

Proposed resolution

The underlying need for revisioning author information has been addressed in core, but we should add tests to ensure there is no future regression.

Remaining tasks

The test proposed needs to be reviewed.

User interface changes

None -- this affects automated testing only.

API changes

None -- this affects automated testing only.

Contributor tasks needed

Task Novice task? Contributor instructions Complete?
Update the issue summary Instructions Done (#25)
Add automated tests Instructions Proposed (#25)
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task
Issue priority Normal/not critical, because it adds automated tests (as future-proofing against a bug that has already been fixed).
Unfrozen changes Unfrozen because it only changes automated tests.
Prioritized changes This is a prioritized issue because the main goal is reducing fragility and avoiding future regressions.
Disruption No disruption is expected by these changes.
Files: 

Comments

agentrickard’s picture

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.

agentrickard’s picture

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.

jstoller’s picture

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

Damien Tournoud’s picture

Right now the node controller does this:

<?php
 
// 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.

mitchell’s picture

agentrickard’s picture

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

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

jstoller’s picture

catch’s picture

Priority:Normal» Major

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.

plach’s picture

jstoller’s picture

@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.

chx’s picture

catch’s picture

@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).

catch’s picture

Issue tags:+beta blocker

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.

jstoller’s picture

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?

catch’s picture

Node type I think could be left off.

redndahead’s picture

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.

plach’s picture

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.

catch’s picture

Title:Have the node_revision table actually store all the information in the node table» Add tests for reverting revisions where revision_uid and uid differ
Category:bug» task

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.

Berdir’s picture

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

Berdir’s picture

Assigned:Unassigned» Berdir
Issue summary:View changes

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?

Berdir’s picture

Component:entity system» node system
Issue tags:-beta blocker

Removing tag for now and moving to the node component.

ZenDoodles’s picture

mgifford’s picture

Assigned:Berdir» Unassigned
tibbsa’s picture

Assigned:Unassigned» tibbsa

Working on tests...

tibbsa’s picture

Priority:Major» Normal
Issue summary:View changes
Status:Active» Needs review
Issue tags:-Needs issue summary update
StatusFileSize
new6.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,956 pass(es).
[ View ]

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!

tibbsa’s picture

Assigned:tibbsa» Unassigned
Issue summary:View changes

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

tibbsa’s picture

Issue summary:View changes
jhedstrom’s picture

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.

jhedstrom’s picture

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.

jhedstrom’s picture

Issue tags:-Needs tests, -Needs issue summary update
StatusFileSize
new5.88 KB
new6.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,198 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

jhedstrom’s picture

I added #2495297: 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.

Status:Needs review» Needs work

The last submitted patch, 30: node-author-revisions-1528028-30.patch, failed testing.

jhedstrom’s picture

Status:Needs work» Needs review
StatusFileSize
new1.4 KB
new6.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,129 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 33: node-author-revisions-1528028-33.patch, failed testing.

jhedstrom’s picture

Status:Needs work» Needs review
StatusFileSize
new1.32 KB
new6.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,211 pass(es).
[ View ]

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