Using revision moderation, the latest revision of a node is not necessarily the published revision. However when the workflow tab is submitted, node_save() is called. This creates a new revision and causes it to be published. The new revision should not be created, because the workflow applies to the nid, not the specific revision. And the new revision being published breaks revision moderation completely.

The one-liner patch below fixes the problem. Although I've tested it only for my simple case. Can anyone think of a compelling reason to call node_save when the workflow tab is submitted? If I learn more, I'll report it here.

This work is sponsored by Medem.com. We are evaluating the workflow module for their needs, but will not use it unless this issue can be fixed.

Index: workflow.module
===================================================================
--- workflow.module	(revision 301)
+++ workflow.module	(working copy)
@@ -194,8 +194,11 @@
   $node->workflow_scheduled_date = $form_values['workflow_scheduled_date'];
   $node->workflow_scheduled_hour = $form_values['workflow_scheduled_hour'];
   
-  // call node save to make sure all saving properties run on this node
-  node_save($node);
+  // Don't call node_save(), as that has too much effect on the node.  In
+  // particular it breaks the revision_moderation module by creating a new
+  // revision and publishing it.  Instead, call our hook_nodeapi so we
+  // complete our save operations.
+  workflow_nodeapi($node, 'update');
 
   return 'node/' . $node->nid;
 }
CommentFileSizeAuthor
#4 171210.diff587 bytesDave Cohen
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

moshe weitzman’s picture

lots of modules might want to notice a workflow change and send email, post to a web service, etc. they can't do so without firing hook_nodeapi('update').

maybe there is some other solution out there?

Dave Cohen’s picture

Thanks Moshe for the feedback. I'm looking into this further, and now I'll argue that the call to node_save as it is today is a bug. Ready?

Say we have a node (nid==1) with two revisions, vid==1 and vid==2. Further, we are using revision_moderation, and vid==1 is the active revision. With this setup, we go to the node's workflow tab, and change the workflow state.

Workflow is working with revision vid==1 of the node, and calls node_save(). The node object has not been populated exactly as it would have been by the node form, and one of the "missing" parameters is node->revision. This causes the node module to not create a new row in node_revisions when the node is saved. This is almost what we want, except it does update the node_revisions table row where vid==1. And it updated the timestamp. Effectively making so that vid==1 is the latest revision, and vid==2 predates it.

I haven't quite wrapped my head around all of this, so please correct me if I'm in error.

But I have the feeling that calling node_save is really not what we need here. I think it might be more correct to call node_invoke($node, 'update') and node_invoke_nodeapi($node, 'update'), as node_save does. However, it feels unclean for workflow to be calling those node module APIs.

What might be the cleanest to introduce a hook_workflow function where modules can be notified of this sort of change. In fact, I just learned its already there, calling 'transition pre' and 'transition post' operations. So now I'm back to thinking my patch is OK, and the modules you speak of should be using hook_workflow.

My head is spinning!

emdalton’s picture

Whatever needs to be done to make Workflow, Actions, and Revision Moderation work, we're willing to help fund it! Please contact me if interested. More at http://drupal.org/node/162392 and http://drupal.org/node/165686.

Dave Cohen’s picture

Status: Active » Needs review
FileSize
587 bytes

Without disregarding Moshe's comments, I think the code is much better off with this patch than without it. At least whenever revisions are enabled.

Moshe (or anyone), do you know of specific modules that do rely on nodeapi update operation to respond to workflow changes? That is, is this really breaking anything?

To emdalton, soon I will blog about my experience and what level of success I've achieved with workflow and revision moderation.

Dave Cohen’s picture

I still believe this patch is worth committing.

And, I've written up more thoughts about Revision Moderation with Workflows and Actions.

emdalton’s picture

Revision moderation is now supported in Views, which allows RSS notification and RSS feeds can be converted to email. This is a limited solution that may not meet everyone's needs, but it works well for us. More at http://drupal.org/node/118666

xmattus’s picture

I used Dave's patch and it did exactly want I needed. I was experiencing the same behavior described in in post #2. I'm not using any modules that are broken by calling hook_nodeapi instead of node_save here (at least not that I've noticed).

To be fair, my setup isn't that complex (I just have Actions fire off emails on certain state changes) other than having a multi-stage editing and review process but I think I've achieved harmony of the Actions, Workflow, and Revision Moderation modules. Whew. :)

liquidcms’s picture

mostly subscribing..

i have a couple issues with an existing system that i am just starting to work on:

- i believe we use node_save call to ensure that the node path alias is modified when wfs change is made; so not sure i can remove it;

- but we also use revisions and i see a "bug" from using node_save that current rev of the node has it's owner set to current owner when a WFS change is done. Not sure of general view on this; but i don't think rev uid should change just because state has changed.

to fix that i added this code to restore the uid of the rev:

  // call node save to make sure all saving properties run on this node  
  //    but this will change uid of the current revision to current user ;so let's save uid to write back after save  
  $rev_uid = db_result(db_query("SELECT uid from {node_revisions} WHERE vid = %d", $node->vid));
  node_save($node);
  
  // and then set owner back
  db_query('UPDATE {node_revisions} SET uid = %d WHERE vid = %d', $rev_uid, $node->vid);

  return 'node/' . $node->nid;
liquidcms’s picture

Also, perhaps a change that has been made to module since intial post of this issue; but i dont see a rev created when WF state is changed.

Dave Cohen’s picture

In my original post, I wrote that my client would not use workflow if this issue was not fixed. For the record, we are using workflow, with my patch applied.

In using workflow and revisions where the active revision is not necessarily the most recent, I've come to think about things in different terms:

The normal parts of a node, like title and body, are associated with a vid (revision) and these things are "part of" the revision. A change to any of these things requires a new revision of the node.

Unlike these aforementioned "normal" parts of a node, there are things associated with the nid (the node, as opposed to the revision). The workflow state is an example (and perhaps the best example). I view these things as "containing" the node. That is, think of each workflow state as a bucket that contains nodes. You can move the nodes from one bucket to another without changing any of the normal parts of the node. I believe, when moving the node from one bucket to another, Drupal should not create a new revision or change which revision is active, and that is what this bug is all about. Perhaps its worth noting that, unless a third party module does something unusual, the new revision created when changing workflow state duplicates all the node's data, changing nothing except the vid.

If, as Moshe points out, modules need to be notified of workflow changes, I do not believe that either calling nodeapi with op=update or calling node_save are the appropriate way to do it.

liquidcms’s picture

Just want to continue this with a slightly different aspect.

I agree with Dave, and have now also gotten agreement from my client, a WFS change should not create a rev. This shouldn't be considered a node edit - it isn't. It is just changing the state of the node, not the node itself. Maybe kind of simplistic way of putting it - but i think the same thing Dave is saying.

To that end there are multiple places that a WFS change can occur - we use 3: the node edit page from fieldset that WF module adds, the WF tab and from views bulk operations.

As i have suggested earlier - i dont think modifying WFS from the WF Tab causes a rev (not using Dave's patch; i just think thats normal operation of the module - and i think the correct operation). However, doing the exact same WFS change on the node/edit page does cause a rev (even though nothing else has been modified). This is both wrong (imho) and inconsistent.

I agree its nice to be able to make change at the same time an edit is being made - but since this breaks our initial principal (WF change shouldnt cause rev) i think it is better to remove this from node/edit. I have done this by simply adding a form_alter to uset $form['workflow']; but still think it shouldnt be there at all (or fixed to somehow not cause a rev on node submit which is likely tough).

Haven't checked out what happens in view bulk operations.. thats next.

Dave Cohen’s picture

My understanding is that changing workflow state on the workflow tab calls node_save(), which does create a new revision, and that's the original problem of this thread. That new revision becomes the "current" revision, which breaks revision moderation.

It is true that a new revision is created when the node edit form is submitted. I'm OK with that, even if nothing else has changed. I think its acceptable to create a new revision whenever that form is submitted, even if no changes at all were made. It's too onerous to try to determine whether or not changes have actually been made.

liquidcms’s picture

Well perhaps i am a bit of a purist then. Functional requirements come before UI. And i think going in theory is correct (and more important than what is easier to code) - a wfs change shouldnt cause a rev.

i am also pretty sure that a rev doesn't not get created when we submit from the tab - we have modified the module a bit; but didn't think anything that would affect this (and node_save() call is still there) - but perhaps i should re-test??

so 2nd principal in my purist approach would be that WFS change shouldn't do one thing when done 1 place and something else when done somewhere else.

so, for the moment, our simplest solution remains to be: remove fieldset from node/edit.

Dave Cohen’s picture

I think the call to node_save() will create a new revision only when the content type is configured to create new revision by default.

Also I think if you remove the workflow fieldset from node edit, a user could still bring up the node edit form, submit without changes, and a new revision will be made. So you're removing the temptation to change just the workflow via that form. But the effect that you don't like will still be there.

dericknwq’s picture

Although I am not having the same problems, my issue was that certain fields (cck select fields to be exact) got saved twice with the second one having only the first character saved. E.g. 'Male' gets saved as 'Male' first then again as 'M'. I got that from devel's query log.

My solution was to simply comment everything alway and use drupal_execute(). I thought the correct way was to just call drupal_execute('application_node_form', $form_values); but that seems to cause an error in node_form(). Anyways, this works perfectly for me so it might work for you as well. Can anyone shed a light on whether this is correct?

<?php
function workflow_tab_form_submit($form_id, $form_values) {
  /*
  $node = $form_values['node'];
  
  // mockup a node so we don't need to repeat the code for processing this
  $node->workflow = $form_values['workflow'];
  $node->workflow_comment = $form_values['workflow_comment'];
  $node->workflow_scheduled = $form_values['workflow_scheduled'];
  $node->workflow_scheduled_date = $form_values['workflow_scheduled_date'];
  $node->workflow_scheduled_hour = $form_values['workflow_scheduled_hour'];
  
  // call node save to make sure all saving properties run on this node
  node_save($node);
  */

  // derick: START *****
  drupal_execute('application_node_form', $form_values, $form_values['node']);
  // derick: END *****

  return 'node/' . $node->nid;
}
?>
Anonymous’s picture

I too have been bitten by this problem. I saw how much code was being poured into all the workflow/action based modules and came to the conclusion that Drupal core is doing something wrong. After all, why should any module that needs to update a node or it's metadata have to tip-toe around node_save()? In light of this, I have opened an issue against the node module here: http://drupal.org/node/300714 In my particular situation, changing the sort order of the node_revisions page fixes my woes. If other users here could test this patch out and provide feedback, I would be grateful.

Bastlynn’s picture

Status: Needs review » Closed (won't fix)

Hi,

With the release of Drupal 7, Drupal 5 is no longer receiving security updates, reviews, or development from many contributed modules. Since 5 is now considered a depreciated version, you really should seriously look into upgrading to Drupal 6 or 7. The newer versions of Drupal work better, have more support, and will be safer (literally! security patches!) for your website. We are currently working on a new release for Workflow to Drupal 7. In light of that, further support for Drupal 5 issues is infeasible at the moment. Please consider upgrading to Drupal 6 or 7 in the near future - you'll be glad you did.

- Bastlynn