Implement subsequent runs control.
crea - May 26, 2009 - 07:48
| Project: | Workflow |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs review |
Description
Currently workflow module acts on nodeapi hook. However, that hook maybe run more than once during one request. Simple examples: workflow state change that triggers action that updates the node; workflow state change that triggers workflow-ng/Rules action that updates the node. There can be many more examples: just look at issue queue (probably, all "duplicate" stuff is related to this).
Workflow module is flawed in a way it doesn't have proper control ( or it is not properly documented) for subsequent runs.
Workflow module's nodeapi hook should be aware of subsequent execution and terminate early.
This applies to both 6.x and 5.x
#1
subscribing
#2
This patch is against dev version. It should fix stupid behavior with duplicate comments and subsequent runs.
#3
Ignore patch from #2, it contains stupid syntax error
This is better patch
#4
Coming from #370111: Rules integration for Workflow.
Here is a shorter version of the patch.
* comment unsetting is done *after* the hook is called (comment is still available from the node in hook implementations)
* the workflow variable of the node is set to the new state after a transition. This should avoid double execution of a state transition.
#5
Hmmm, are you sure unsetting comment *after* the hook is called fixes the problem ? I think this will be same as before: Rules event will be able to invoke hook_nodeapi('update', $node) where $node will contain uncleared comment and it will cause double run. The whole point of removing comment before additional actions was to prevent those actions to cause double run.
#6
It works fine for me, I tested some variations.
The Rules integration from #370111: Rules integration for Workflow does not make use of hook_workflow() and the comment is unset *before* the Rules integration is called. I decided to leave the comment in the node to not break existing hook implementations that rely on it.
Could you do some testing and describe an example where you get double runs?
#7
Did you check this patch with other integration modules ? There is hook_workflow() invocation, and because your patch unsets comment after it, any module that implements it and updates the node in it, can bring doubles. Order is like this:
Workflow comment is added -> hook_nodeapi('update') is run -> hook_workflow() is run with uncleared workflow_comment -> any integration module updates the node (with still uncleared workflow_comment) -> hook_nodeapi('update') is run -> You get double.
#8
How Workflow is going to detect "empty" subsequent runs with your patch ? With my patch it was clear: no comment and no state change = "empty" run with early termination.
#9
New patch: considering comment #7, unsetting the comment is now done *before* the hook invocation. Thanks for the explanation!
In my patch it works the same as in your patch: "empty" subsequent runs stop at the same place as in your patch, because a written comment is cleared immediately after it was written to the history.
#10
Well, I think actually we can provide workflow comment in node object. It must be stored in different property, that doesn't affect Workflow module itself, so if subsequent hook_nodeapi('update') happen, workflow already knows that comment is cleared and terminates, because it checks workflow_comment and not our new property.
I guess this just needs little more thought...
#11
The problem that I see at the moment: we cannot detect whether the hook_nodeapi('update') call is a subsequent run or a completely different node update call. In my opinion, adding more complex mechanisms would blow up the code and make it even more difficult to manage. I suggest to stick with the simple unset of the comment, it works and fulfills our needs.
It also makes sense to provide the comment within the node in the first hook invocation ("transition pre") and to remove/unset it afterwards in hook "transition post".
#12
I think a simple solution like that from #9 would suffice as it's a quite common approach to just unset the variable once it has been processed.
So I gave the patch from #9 a test together with #370111: Rules integration for Workflow - it's working fine, no subsequent runs appeared in my tests.
#13
Tested the patch for a certain time and it works great (together with #370111 ).
However - if you make a state transition (e.g. using the workflow tab) which triggers a rules' action to modify the node again (where transition without state change occur) a new transition is added with identical comment.
If you decide to unset the comment in case ($old_sid == $sid) we should think about unsetting the comment after state transition (_workflow_node_to_state), before trigger (module_invoke_all('workflow'.. ), too. This solves my problem. However the rules trigger will then lack the comment (attached to node). But i think this would be ok. If not we need to introduce a different subsequent runs control strategy (such as attaching a value to the node).
Any inputs appreciated. Will provide patch then.