Quoting catch from http://drupal.org/node/1082292#comment-5855588:

For example we need separate columns for node->uid (primary author) and node->revision_uid (last edited) - since these are completely different values and have been the cause of serious bugs and misconceptions in various versions of core.

Comments

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:

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

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

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

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.

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

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.

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.

Title:Have the node_revision table actually store all the information in the node tableAdd 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.

Those tests are needed, because the mapping there is currently quite broken I think. See also #2031183: Problems editing nodes if datetime.module is not enabled

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?

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

Removing tag for now and moving to the node component.