Arguments are swapped in module_invoke_all('workflow', 'transition permitted')

No, the parameters are not inverted.

I think I miss-understood "the parameters are always inverted", I would look to the API docs to see what the correct order is supposed to be.

I think you are right: the old_sid and new_sid seem inverted in the case of 'transition permitted'
These are all the hook-calls in the workflow :

01 workflow.module:  $result = module_invoke_all('workflow', 'transition permitted', $new_sid, $old_sid, $node, $field);
02 workflow.module:  $result = module_invoke_all('workflow', 'transition pre',       $old_sid, $new_sid, $node, $force, $field);
03 workflow.module:  $result = module_invoke_all('workflow', 'transition post',      $old_sid, $new_sid, $node, $force, $field);
04 workflow.module:  $result = module_invoke_all('workflow', 'transition pre',       $old_sid, $new_sid, $node, $force, $field);
05 workflow.module:            module_invoke_all('workflow', 'transition post',      $old_sid, $new_sid, $node, $force, $field);
06 workflow.module:  $result = module_invoke_all('workflow', 'transition permitted', $current_sid, $transition->state_id, $node, $field = NULL);
07 workflow.module:            module_invoke_all('workflow', 'workflow delete',      $wid, NULL, NULL, FALSE);
08 workflow.module:            module_invoke_all('workflow', 'transition delete',    $tid, NULL, NULL, FALSE);
09 workflow.module:            module_invoke_all('workflow', 'state delete',         $sid, NULL, NULL, FALSE);
10 workflow_access.features.inc: module_invoke($module, 'workflow_access_features_default_settings');
11 workflow_revert.module:      $result      = module_invoke_all('workflow', 'transition permitted', $variables['sid'], $variables['old_sid'], $node);

I'm not sure what the order is in line 06. Lines 01 and 11 use the 'wrong' order.

I think you should use the wrong order for 'transition permitted'. Changing the order in the module code at this moment will break existing sites upon update.
This should be a task for version 7.x-2.0 or 8.x-1.0.

Yes, that seems to be the case. Perhaps other maintainers have some extra info.

I found these 3 recent commits regarding the hook. Didn't read the issues yet, but we may just correct the calls, and inform the participants of those issues (since they probably have implemented this hook).
29-may-2013: #2001980: Workflow Revert should check state change
08-jun-2013: #1997242: ask other modules for permission in node forms
a D6 request from 2010: #698826: Add a hook to let other modules decide, whether a state-change is allowed or not (D6 only)

Make absolutely sure that they are "backwards" and not just look that way in the situation. I have definitely implemented those hooks, as has the contributed Workflow Revert. I would think I would have noticed them being backwards, but I am blond...

OK, that's nice, because the guy who requested this implementation of the hook (Tim-Erwin), is the developer of OG Workflow module.
I'll send him a message, to join the discussion. I suppose chances a small that other people have implemented the hook in the last 4 months.

Oh, indeed. We use all the hooks here.

Yep, this hook is really helpful for og_workflow although we are currently using the arguments in wrong order as andrea.vivaldi already mentioned. The number of sites using that module in dev version is rather small, so I'd like to leave the implementation as it is and wait for the workflow module to change the invocation. We can simply require workflow dev version, that should not be to big an issue and seems to be the direction you would also follow.

Hello,
for wf_required_fields i had to do
this change in order for it to work. Will this be affected by the change that is proposed here? I am also using hook_workflow_veto_workflow($op, $old_state, $new_state, $node) in a custom module of mine, is that going to be affected as well?

@k.skarlatos, we are only discussing to change the interface of the 'transition permitted' hook. Although the current status is not incorrect, the order of new_sid and old_sid is different from the other hook implementations, hence confusing.
Since the hook is rather new (introduced in 7.x-1.2), not much websites should be involved.
So your patch is not affected - it is using 'transition pre'.

PS. You'll find that the new hooks have extra arguments. Please follow:
#1938222: Re-architect Workflow module for 8.x .... or not ?
#2019345: Create a 'Workflow Field' with Widget, Formatter, Fieldtype

ok, thank you for the clarification!

I finally looked at the code. I may still be a bit confused. In workflow_execute_transition(), the parameters are backwards because the transition is already underway.

At any rate, my code that implements the hook seems to work correctly, both before and during a transition.

"Issue #2102193 by johnv: Fixed documentation of hook_workflow() in workflow.api.inc" is committed here in 7.x-2.x.

I too will check again if the interface is consistent.
I have another doubt about this hook: whenever the possible choices/options/transitions are calculated, this hook is called for every state. This seems very inefficient. The alternative is not as clean: to send the old_state, and an array of the possible new states.

Current situation:
$result = module_invoke_all('workflow', 'transition permitted', $new_sid, $old_sid, $node, $field);
Possible alternative:
$result = module_invoke_all('workflow', 'transition permitted', $old_sid, array $new_sids, $node, $field);

Yes. If you go back and look at the conversation when that was added, I had concerns about performance. If there is a better way, we should definitely pursue it.

Hi, I confirm the bug. The arguments are swapped.

Actually I'm working with the implementation of hook_workflow, with the operation 'transition permitted'. I have several states, in detail I have 2 states A and B with a transition A>B. The hook with 'transition permitted' is called 4 times with these old > new states:

1. A > A, A > B,
2. A > A, A > B,
3. A > A, A > B,
4. B > A, B > B,

The last time from workflow_execute_transition with swapped arguments.

So,
the call from workflow_field_choices is correct.
the call from workflow_execute_transition is swapped.
the call from workflow_revert is swapped, but that migth be explicitly.

I swapped the incorrect call in the 2.x branch.
The commit is here.

I still doubt about the performance of the call when calculating the options in the Workflow form. But that's another thing.