Bug in Transition guard logic

acorncom commented 10 years ago

Howdy, I've been using the state machine setup heavily for a project I've been working on (thanks very much, it's been quite useful). Think I ran into a bug in logic related to how guards on transitions work.

The code in question is in the StateMachine can method:

public function can($transition)
{
    $transition = $transition instanceof TransitionInterface ? $transition : $this->getTransition($transition);

    // this if statement is the code in question
    if (null !== $transition->getGuard()) {
        return call_user_func($transition->getGuard());
    }

    if (!in_array($transition->getName(), $this->getCurrentState()->getTransitions())) {
        return false;
    }

    $event = new TransitionEvent($this->getCurrentState(), $transition, $this);
    $this->dispatcher->dispatch(FiniteEvents::TEST_TRANSITION, $event);
    $this->dispatcher->dispatch(FiniteEvents::TEST_TRANSITION . '.' . $transition->getName(), $event);

    return !$event->isRejected();
}

Below is the config in question that I'm sending in to the ArrayLoader:

return array(
    ...
    'transitions' => array(
        ...
        self::TRANSITION_BUY_TO_REVISED => array(
            'from' => array(self::STATE_BUY),
            'to' => self::STATE_REVISED,
            'guard' => array($this, 'guardWhoCanRevise'),
        ),
        ...
    ),
);

And the callback:

public function guardWhoCanRevise()
{
    // only this specific user is allowed to make the transition from BUY to REVISED
    if($this->user_id == Auth::user()->id) {
        return true;
    }

    return false;
}

Expected behavior

That the guard callback will apply in addition to the from / to logic

// if the logged in user (Auth::user()->id) doesn't match, this returns false properly
$model->can(ModelName::TRANSITION_BUY_TO_PLANNING);

// if my model isn't in the BUY state, then this query returns true if my logged in user matches
// even though my from/to logic dictates that it should return false
$model->can(ModelName::TRANSITION_BUY_TO_PLANNING);

Bug fix

If we change the can method as below, then the from/to states will act as an additional filter in addition to the guard instead of only if the guard doesn't exist.

public function can($transition)
{
    $transition = $transition instanceof TransitionInterface ? $transition : $this->getTransition($transition);

    if (null !== $transition->getGuard()) {
-        return call_user_func($transition->getGuard());
+        if(!$return = call_user_func($transition->getGuard())) {
+            return false;
+        }
    }

    if (!in_array($transition->getName(), $this->getCurrentState()->getTransitions())) {
        return false;
    }
    ...
}

I can send in a pull request (though I may need help writing the tests) if this is indeed a bug that you want to fix and not how you intend the state machine to work. :-)