Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
If the workflow transition is not permitted for the given transition and FORCE is false, the state of the workflow field should not be changed.
Proposed resolution
This method should first check if the transition was allowed before changing the value of the workflow field if FORCE = false.
Comment | File | Size | Author |
---|---|---|---|
#14 | workflow_2724199_13.patch | 1.11 KB | johnv |
Comments
Comment #2
bohemier CreditAttribution: bohemier commentedIt seems that the call to workflow_entity_field_save takes care of the transition and field value. Not sure if the fix should be in workflow_entity_field_save or workflow_execute_transition. But something like this could be done in workflow_execute_transition:
Comment #3
johnvIf the workflow transition is not permitted for the given transition, how do you try to make the transition? The widgets will hide the transition. Do you try to do this programmatically?
It seems you are right. At some point, workflow_entity_field_save was introduced, but the checks are not copied.
Comment #4
bohemier CreditAttribution: bohemier commentedHello Johnv
Yes this is done programmatically, through action/triggers. The state change of one workflow field will change the workflow field state of other nodes in my use case.
Thanks!
Comment #5
johnvYou would want to use the WorkflowState::getOptions function to get the valid options upfront.
Comment #6
bohemier CreditAttribution: bohemier commentedOK thanks for the info. What do you think about this fix though? Passing the $force boolean leads to think that this method will properly handle the transition which is not the case if force is FALSE...
Comment #7
johnvThere is a problem to fix. The problem with the fix is that it tries to execute the transition. I prefer to isolate the checks and apply them.
I need to refind the commit which lost the functionality. Not sure how to do that without scanning through all commits.. any help is appreciated.
Comment #8
bohemier CreditAttribution: bohemier commentedI would be glad to help... If I understand you, I should not rely on $transition->execute($force) to validate the transition but should call WorkflowState::getOptions beforehand to check the validity? Is that what the commit you're looking for was about?
Comment #9
johnvIndeed. It is a semantic thing. $transition->execute($force) implies that you are actually changing the transition. But you merely want to test it.
OTOH, after a second look I think the proposed solution is OK. Because you ARE executing the transition. But I need to check the old code.
Comment #10
bohemier CreditAttribution: bohemier commentedI think this function presumes that the transition is going to be executed. The proposed change is to bail out if the transition does not go through.
Comment #11
johnvBelow issues are related:
#2805217: Transition's "force" property is not taken into account when executed.
#2724199: workflow_execute_transition() does not respect FORCE parameter
Comment #12
johnvComment #14
johnvI created a proper patch for your solution.
The FORCE parameter is not respected, due to the backwards compatibility with 7.x-1.2. But that is only for workflow_node.
However, that is not a valid reason for workflow_field, which does not use transaction->execute(). Apparently I had some problems, looking at the comment :
// @todo: use $new_sid = $transition->execute() without generating infinite loops.
It could be those problems have been resolved already. But I do not have the time to test all situations (Gosh, I wish I created automated tests....)
Comment #15
johnvIt could be possible to re-use or isolate the testing code from transition->execute(). Both Transition and ScheduledTransition should be tested.
Comment #16
johnv