There is an issue with the way the node module handles revisions that has an adverse affect on implementing workflows using contrib modules such as workflow and workflow_ng. It really only becomes apparent when using the Revision Moderation module. An issue has been posted against the workflow module at http://drupal.org/node/171210. Several users there commented that any calls to node_save are potentially dangerous to this setup. The problem is quite simple: The node module tracks a unique version (vid) for a node and a timestamp when that revision was last updated. When generating the list of node revisions, the node module sorts by the timestamp and displays the most recently modified revision at the top of the list, making it confusing to tell what the next revision (next highest vid) is.

Steps to duplicate:

  • Install Revision Moderation module and Workflow module
  • If you are using the admin account, disable the Revision Moderation exemption for administrator
  • Enable revisions for page content type
  • Create a workflow with two states
  • Create a new page (vid=1)
  • Edit the newly created page and save as a new revision (vid=2)
  • Visit the Revisions tab for the page. You'll see the new revision at the top and the current revision highlighted below.
  • Visit the Workflow tab and change the workflow state.
  • Re-visit the Revisions tab for the page. You'll now see that the "current" revision (vid=1) is at the top of the list and the "latest" revision (vid=2) is below.

The simple fix I have been testing is to sort node revisions by vid. The node module enforces unique and strictly increasing integers for vid, so why not use them? It seems to play nicely with workflow, workflow_ng and revision_moderation. I think we need a solution where modules can call node_save when a node or it's metadata need to be updated without having strange side-effects. I haven't tested this thoroughly so I would appreciate input from other users and core members. The attached patch is against 5.10, but I have looked at CVS and this problem still persists in HEAD.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drumm’s picture

Version: 5.x-dev » 7.x-dev

Thanks for the detailed report. I generally like to see fixes like this go into the most recent affected version, in this case, all versions including 7.x, and then be backported from there. This patch does apply to HEAD. I think this is a good change, but have not thoroughly reviewed.

j0hn-smith’s picture

FileSize
1007 bytes

I made a duplicate as I didn't find this issue (#309512: node/%node/revisions revisions should be sorted by vid instead of timestamp).

Here's the simple patch (D7) from that issue.

Anonymous’s picture

Status: Needs review » Needs work

The last submitted patch failed testing.

j0hn-smith’s picture

FileSize
1007 bytes

I've made the patch again, it seems to be the same as the previous one that didn't work but I'm not familiar with CVS.

lilou’s picture

Status: Needs work » Needs review
FileSize
1.13 KB

Reroll your patch from drupal root directory.

catch’s picture

Status: Needs review » Needs work

There shouldn't be a situation where the timestamp of a revision gets changed if you revert or otherwise edit a revision, ought to create a new revision instead - that's the whole point. Also autoincrement fields shouldn't really mean anything.

j0hn-smith’s picture

From #309512: node/%node/revisions revisions should be sorted by vid instead of timestamp

"When using revision moderation you want to see revisions with a higher vid than the current node above the current node in the list regardless of timestamps. Without this change new unapproved revisions can appear lower than the current revision in the list which can be confusing."

An unapproved revision may be edited before it's approved thus updating the timestamp. Revisions are not necessarily approved in the order they were created, there can be more than one awaiting approval - sorting by vid helps by showing unapproved revisions created after the current version higher in the list and those created before lower. Admin users can edit nodes and choose not to create a new revision which will also update the timestamp.

Basically sorting by vid is more useful than sorting by timestamp, if you're only using core there isn't any noticeable difference.

catch’s picture

Admin users can edit nodes and choose not to create a new revision which will also update the timestamp.

This is a very good point, and given that, ordering by vid seems OK here. The patch also needs updating to use the new databas layer (named placeholders).

alexjarvis’s picture

Status: Needs work » Needs review
FileSize
1.1 KB

Here's a patch against D7 Alpha 2.

aspilicious’s picture

I think you need to add what's discussed in #8

alexjarvis’s picture

Unless I'm misunderstanding something this is a static query and the only field that would need a placeholder is the nid, which already has one.

cosmicdreams’s picture

I'm trying to help test this issue, but I'm confused. I don't see a 7.x version of the Revision Moderation or Workflow module that the test procedure requires.

Are these modules now apart of the 7.x branch?

If it is possible to properly test these patches on the current cvs version (7.x) of Drupal can you re-list what the proper testing procedure is?

Dries’s picture

Status: Needs review » Fixed

Looks all good to me. Committed. Thanks.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

alexjarvis’s picture

Version: 7.x-dev » 6.x-dev
Assigned: Unassigned » alexjarvis
Status: Closed (fixed) » Patch (to be ported)
FileSize
892 bytes

Backported patch to D6.

jbrauer’s picture

Status: Patch (to be ported) » Needs review

This is a bug that doesn't affect API or strings and seems that the backport to 6.x is appropriate.

netw3rker’s picture

+1 if its a bug in d7, its a bug in d6.

This may be an edge case, but I just ran into this issue with a client that doesn't have the best timezone handling on a system that exports data into drupal. this winds up creating the situation where the VID's are in the right order, but the timestamps can be off by a couple of hours depending on where and when the data was modified.

dstol’s picture

Status: Needs review » Reviewed & tested by the community

Looks good here.

alexjarvis’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.