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.
- Create a node with user A (current user is fine)
- Edit the node, change authoring information to user B, check the 'new revision' checkbox.
- Revert to the revision created with #1.
- Revert to the revision created with #2.
Now there's two issues:
- User A is still listed as the author of the node, but it should be user B.
- 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
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. |
Comment | File | Size | Author |
---|---|---|---|
#35 | node-author-revisions-1528028-35.patch | 6.28 KB | jhedstrom |
Comments
Comment #1
agentrickardThis 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.
Comment #2
agentrickardAnd, 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.
Comment #3
jstollerFor consistency, I would change {node_revision}.timestamp to {node_revision}.changed.
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commentedRight now the node controller does this:
... 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.Comment #5
mitchell CreditAttribution: mitchell commentedIs 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
Comment #6
agentrickardWell, we still have a {node_revision} table...
Is there an issue to kill that table as part of the entity overhaul?
Comment #7
jstollerI've created the following tasks:
#1838884: Add 'created' and 'changed' columns to node_revision table
#1838862: Add revision_uid to the node table
Comment #8
catchSince 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.
Comment #9
plachPlease, 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.
Comment #10
jstoller@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.
Comment #11
chx CreditAttribution: chx commentedI 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.
Comment #12
catch@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).
Comment #13
catchTagging 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.
Comment #14
jstollerFor 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?
Comment #15
catchNode type I think could be left off.
Comment #16
redndahead CreditAttribution: redndahead commentedCan 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.
Comment #17
plachAre 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.
Comment #18
catchYes 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.
Comment #19
BerdirThose tests are needed, because the mapping there is currently quite broken I think. See also #2031183: Improve test coverage for node authored on widget
Comment #20
BerdirWill try to work on this.
Given that this is just about writing tests now, I don't think this is a beta blocker anymore?
Comment #21
BerdirRemoving tag for now and moving to the node component.
Comment #22
ZenDoodles CreditAttribution: ZenDoodles commentedComment #23
mgiffordComment #24
tibbsa CreditAttribution: tibbsa commentedWorking on tests...
Comment #25
tibbsa CreditAttribution: tibbsa commentedI 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!
Comment #26
tibbsa CreditAttribution: tibbsa commentedReady for consideration by other reviewers; added beta phase evaluation to issue summary.
Comment #27
tibbsa CreditAttribution: tibbsa commentedComment #28
jhedstromThe tests in #25 are looking good.
Some coding standard nitpicks:
array (
should just bearray(
.Comment needs to end with a period.
Whitespace/indentation issues.
Comment #29
jhedstromSince #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.
Comment #30
jhedstromRerolled #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 thenode_revision
table.Comment #31
jhedstromI 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.
Comment #33
jhedstromThe old patch still referenced the now-removed
Drupal\Core\Utility\String
class.Comment #35
jhedstromThe two fails were due to #2383015: Revert back is redundant landing.
Comment #36
jhedstromComment #42
brenk28 CreditAttribution: brenk28 commentedLooks 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
Comment #50
smustgrave CreditAttribution: smustgrave at Mobomo commentedDoes appear to be a duplicate of #2977362: Revision user incorrectly appears as anonymous user when node author is canceled could anyone confirm?