Coming here from #542290: Make it easier to create draft revisions in core.

This module currently uses the same approach as revision_moderation did in D6 to handle new revisions in moderation - manually updating the vid in hook_node_update().

As you know this doesn't work in Drupal 7 due to Field API's model of always keeping the latest revision.

I ran into this on http://drupal.org/node/282122#comment-1927444 and that comment has working code (at least it worked on the time), which instead of munging the vid, clones the 'active' revision, saves the new one, then adds the active revision back to the top of the list. It is a workaround, but it's arguably no more of a workaround than changing vids around with direct database queries.

It should be possible to do that workflow without a core patch, but if it's not we could try to add helper functions (such as node_delete_revision() from that patch) to core to make it easier.

Comments

rdeboer’s picture

rdeboer
Location Melbourne
commented

Gee, thanks, I think....

The patch you mention is rather long, so I'm not sure what bits to use... I suspect something like the code below, which I lifter from that patch.
This code seems to indicate that instead of manipulating vids we're going to create a superfluous revision, delete it and then manipulate taxonomies and timestamps?
Why is that so much better? Either way it's going to be a hack (and hacks are by definition ugly) to work around the real problem, which is this D7 core issue.
Or am I missing something?

And will the code from your patch work when the node has attachments?

+  node_save($node);
+
+  // If the node is published, and there is already an existing revision
+  // we need to ensure that the data from the current active revision is used
+  // in both the {node} and field API tables.
+  if ($node->status && $node->old_vid) {
+    // Since we have already saved a new revision, load the original again.
+    $current = node_load($node->nid, $node->old_vid);
+
+    // Set taxonomy terms to tids to resolve inconsistencies between loaded
+    // and saved data.
+    if (module_exists('taxonomy')) {
+      $current->taxonomy = array_keys($current->taxonomy);
+    }
+    $current->revision = TRUE;
+
+    // Set both the updated and revision timestamps to those of the revision
+    // we are re-saving to ensure it is listed correctly on the revisions tab.
+    $current->timestamp_updated = $current->timestamp_revision = $original->changed;
+    node_save($current);
+
+    // We now have a duplicate copy of this revision, so delete the old one.
+    node_delete_revision($original->nid, $original->vid);
+  }
catch’s picture

catch
he/him
Primary language English
commented
Title: Stop using vid munging for pending revisions » Avoid using vid munging for pending revisions

That's the section of code, I think that will mostly work in current Drupal 7, except you should not need any special taxonomy handling and it may need a bit of updating. It's a very old patch and the issue turned out to be a disaster in the end, but the field API issue in core reminded me I'd had the same problem.

There's no node_delete_revision() in core, but the patch adds that, it'd be a small helper function.

I'm retitling this, I didn't mean "stop" as in "stop!", just "see if it might be better to avoid", sorry if it came off a bit abrupt :(

On the core bug, I think it's 50/50:

- doing a direct update on the revision ID doesn't allow other modules to react to the change. A module that did some kind of write-through caching on node_save() could get into trouble with the direct manipulaton of the node table, and the $node object is going to be different as it gets passed to other hooks (unless you're changing that along with the db_update(), I'd need to read through again to check but in a hurry now).

- As bjaspan pointed out on that issue, core has never really enforced the vid rule either way, and since field API is not strictly coupled to the node API, it's entitled to not support revision munging.

- the patches on that issue so far, rather than changing the Field API behaviour (which can't really happen for D7), add all kinds of strange things to the {node} table which are much worse than either what this module currently does or the hack I'm proposing, I'm mainly trying to avoid those being committed in a hurry. The trick used here made complete sense in D6 but it's not going to be possible to just have that work in D7 without trade-offs somewhere.

johnpitcairn’s picture

johnpitcairn commented

Sub. I have a module that extends Revisioning and uses the same technique in D6.

rdeboer’s picture

rdeboer
Location Melbourne
commented
Version: 7.x-1.0-beta5 » 7.x-1.0-beta10
Assigned: Unassigned » rdeboer

While waiting for the storm to clear I have implemented a couple of very localised workarounds that seem to do the trick, still in the spirit of the D6 approach.
Not saying the solution will be permanent, am happy to adopt a better one soon, but it seems to work well enough for now and approaching stability.
I want to get 7.x-1.0 working reliably and released ASAP.

rdeboer’s picture

rdeboer
Location Melbourne
commented
Version: 7.x-1.0-beta10 » 7.x-1.0
Status: Active » Closed (works as designed)

All seems to be working fine. Closing.