It has been added to invoke workflow_hook(transition pre) when creating the workflow tab form, but it's missing for node forms.

Honestly, I find it kinda counter-intuitive and not really true to the wording to pretend to execute a transition (invoking the pre hook), modules act according to that, and then you don't transition. I'd rather add a new "hook" (it's merey a parameter value) to the API as has been suggested before (#698826: Add a hook to let other modules decide, whether a state-change is allowed or not (D6 only)). I can understand the argument there that 'transition pre' was enough. It was back then, because it was only called when a transition was actually about to happen. Now, we have a different situation, because the hook is also called when there is no transition. Modules might do things in 'transition pre' they only want to do in case a transition really takes place.

Personally, I'm fine with just 'transition pre', because "my" module (og_workflow) only checks permissions anyways. Hence, a fast fix would be to also invoke hook_workflow('transition pre', ...) in workflow_form_alter() and remove invalid states as done in workflow_tab_form():

foreach ($choices as $sid => $name) {
    if ($current == $sid) {
      continue;
    }
    $result = module_invoke_all('workflow', 'transition pre', $current, $sid, $node);
    // Stop if a module says so.
    if (in_array(FALSE, $result)) {
      // if vetoed, remove from list.
      unset($choices[$sid]);
    }
  }

In the long term it might be better to introduce a new hook type as argued above.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

NancyDru’s picture

I agree with your assessment. It was expedient to use "transition pre" to deny state changes. I shall consider adding a different operation. Do you have suggestions?

 

What can we do to work more closely so that adopters of our modules are not left in the lurch?

Tim-Erwin’s picture

Well, it's a little difficult to keep the wording consistent with the other hooks, because until now they indicated actions. Now we are dealing with a question. Nonetheless, it won't be a big issue. I'd probably go with "transition permission". So simply replace the "transition pre" in workflow_tab_form() with this new one plus add

  // Invoke a callback asking for permission for the oncoming transition. Modules
  // may veto the transition by returning FALSE.
  $result = module_invoke_all('workflow', 'transition permission', $old_sid, $sid, $node, $force);

  // Stop if a module says so.
  if (in_array(FALSE, $result)) {
    watchdog('workflow', 'Transition vetoed by module (permission hook).');
    return;
  }

in workflow_execute_transition() right before the same code block with "transmission pre" (line 791 currently). I'd leave the pre block just as it is, so that also pre hooks can veto as they can now. That also ensures a secure transition into the new mechanism. (Is that what you mean with "not left in the lurch"?)

Additionally, it would be nice to have the "transition permission" call in workflow_form_alter() so that invalid states are removed in node forms, not only the workflow tab.

NancyDru’s picture

Maybe it's just me, but 'transition permission' sounds like the hook could also make a transition happen. In actuality, the hook can only disallow. I'm wondering if perhaps changing the pattern might make its purpose stand out a bit more; something like "check transition."

NancyDru’s picture

Status: Active » Needs review
FileSize
3.72 KB

See how this works for you. I'd like to get a 7.x-1.2 release out and would like this to be in it.

NancyDru’s picture

Issue tags: +1.2 release blocker

Release blocker

NancyDru’s picture

Tim-Erwin’s picture

So, just some remarks from me:

  1. I really like that you have put the logic into workflow_field_choices(). It's central and avoids duplicate code.
  2. This, however, does not cover the check during the actual transition. We need to invoke the hook also in workflow_execute_transition() like in the code sample in #2.
  3. What are you intending to do in workflow_node_view() with the little change you did there? You return when the first of the choices is the same as the current state. What's the point of that? (Btw: Workflow_allowable_transitions() in the comment should start with a lower-case "w", I guess that was not intentional :) )
  4. Actually, the check for the current state could also be moved to workflow_field_choices(), right?

As for the wording ("transition choice"): It makes sense from your perspective calling it from within workflow_field_choices(). However, how would you name that same hook when called from workflow_execute_transition()? There it does not really make sense to me. What about "transition allowed" or "check transition permission" (a little longer but very clear)?

NancyDru’s picture

2 . Workflow_execute_transition() already does "transition pre" and "transition post." By the time it's invoked, all the choices should have been made. I don't see how restricting choices at this point makes sense.

3. Good question; looks like a blonde moment. My English teacher beat it into my head to capitalize the first word of a sentence.

4. Explain?

In all deference to the creators of #698826: Add a hook to let other modules decide, whether a state-change is allowed or not (D6 only), my implementation does not allow transitions, it can only deny the transition choice. To me "transition allow" or "transition access" both give the wrong impression. So what do we call it? "transition exclude"?

Tim-Erwin’s picture

2. workflow_execute_transition() does pre and post, but if we assume that the new hook is the one with which modules can veto a transition (in addition to pre), it needs to be checked when the transition is executed. Sure, the choices that are presented to the user are already limited, but one can still forge a request with a forbidden transition. This check in workflow_execute_transition() is actually the most important one. (Forget about only displaying allowed transitions, as long as you prevent users from executing them.)

3. The important question here was: "What are you intending to do in workflow_node_view() with the little change you did there? You return when the first of the choices is the same as the current state. What's the point of that?" :)

4. This refers to "$current == key($choices)" in workflow_node_view(). If I'm not mistaken, the intention behind this is to not display the workflow form, if no transition is available. workflow_field_choices() should only return valid transitions anyways (which excludes transitions to the current state), thus the check could be changed to

if (count($choices) == 0) {
    return;
}

No transitions, no form. Of course, you might wanna make sure the current state is not in the choices in workflow_field_choices(). That clear now?




Aaaaand concerning the hook name lets have a look at how you explain what this hook does:

Modules may veto a choice by returning FALSE.

Modules can say "this transition is OK" by returning something different than FALSE and they can say "I don't wanna allow this transition" by returning FALSE. It's not just "I want this or that transition not to be displayed" but "... not to be executed". And that's why you don't display it in the first place, but that's just convenience. It's really about whether the execution of a transition is allowed.

By the way: Since the modules are supposed to return FALSE for a veto (which is good), the hook name must be positive. With a negative hook name like "exclude" I would assume I need to return TRUE in order to exclude. A positive hook name like "permitted" would suggest to return TRUE for permitted and FALSE for denied/exclude.

NancyDru’s picture

2. How would they forge the request? And if they go that far, why would they not just use the "force" parameter and bypass the checks altogether>

3. The change has been reverted.

4. The standard workflow state change form displays all the states that are available, plus the current state, which is chosen by default. Because of that design, workflow_field_choices() must return the current state. This was not my design, but I do sort of understand it. It may have been required when the form was attached to the comment form.

Modules may return TRUE (or null), but that is the default unless someone returns FALSE. Thus the only thing that really matters is FALSE. "Permitted" works for me.

NancyDru’s picture

What I have is now committed. Your concerns are now the only thing keeping 7.x-1.2 from being created.

"transition permitted"

Tim-Erwin’s picture

2. That's simple, present he user with e.g. 2 and 3 as the state they may transition to. Well, the clever user just sends you a 4 in a hand-crafted request which you would execute without checking again. They cannot use the $force parameter because that is not set by the form but internally in the module. Right before the transition is executed is the most important part for checking if it is allowed.

4. Well, I can understand that. Although, personally I would change that design (which should be fairly easy and not too extensive). Showing the current state is a presentation decision, returning the choices a user can make is separate from that. Semantically workflow_field_choices() should return the available choices, not more. Anyway, it works...

"transition permitted" is cool :)

NancyDru’s picture

4. I will always entertain a patch. My only concern, and presumably your own, is not breaking any of the many add-on modules, such as Workflow Extensions and OG Workflow.

2. Hmm, I see what you mean. The 4 will be checked to see if it is set by the admin as an allowable transition, so we do have that. Now, if this user is clever enough to send a forged request, are they not also likely to be crafty enough to code the hook?

I'm not trying to be obstinate here, I am just concerned about performance. It seems to me that if there is checking to be done, "transition pre" should catch that.

It would be nice to have the module owner weigh in on this.

Tim-Erwin’s picture

I don't really understand what you mean with

The 4 will be checked to see if it is set by the admin as an allowable transition, so we do have that.

When will the 4 be checked? Right now it won't be checked before executing the transition.

You should not worry about performance here. Performance must mainly be watched for read operations, write operations (like to perform a transition) don't occur very often and we can afford the extra 20ms to execute the hook.

And concerning "transition pre" catching that, you are right in the case you require modules to check that. However, the modules would then look something like

hook_transition($op) {
    switch($op) {
        case 'transition permitted':
            return checkIsTransitionAllowed();
        case 'transition pre':
            if (!checkIsTransitionAllowed()) {
                return FALSE;
            }
            else {
                return doSomethingBeforeTransitioning();
            }
    }
}

They would have the check twice. That's not dramatic, but also not straight forward. And you really need to make clear in the API that you require to check for permission not only in the permitted hook, but also in the pre hook.


It is nice, that pre hooks can veto transitions but IMHO it should not be mandatory when there is also a permitted hook. And really, the performance will not suffer from calling a hook that maybe two modules implement in a write operation.

It would be nice to have the module owner weigh in on this.

Do you mean jvandyk should say what he thinks about this?

NancyDru’s picture

FileSize
597 bytes

The state #4 is referenced in your #12.

Yes, it would be nice to hear from John.

See the attached patch

NancyDru’s picture

Status: Needs review » Fixed

Committed.

Tim-Erwin’s picture

That looks good, thank you!

Tim-Erwin’s picture

Status: Fixed » Needs review
FileSize
1.34 KB

Whoops, same issue now as we had before: The $force parameter is not checked. Rather than passing it to the modules, you could just not ask for permission when $force is TRUE. See attached patch.

NancyDru’s picture

Status: Needs review » Fixed

Thanks. Committed with attribution.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

johnv’s picture