raoul2000 / yii2-workflow

A simple workflow engine for Yii2
BSD 3-Clause "New" or "Revised" License
171 stars 48 forks source link

problem caused by running the beforeEvents when running getNextStatuses() #36

Closed cornernote closed 8 years ago

cornernote commented 8 years ago

The problem is a little complex, but I'll do my best to try to simplify it.

I want to get a list of the next statuses, running the beforeEvents to ensure they are allowed.

The flow is:

$unit = Unit::findOne(1);
// the following are just to show what's already assigned to some variables:
// $unit->status = 'unit/packed'
// $unit->package->collected = 1

// valid transitions from here should be: unit/despatch and unit/collected,
// however only unit/despatch is returned
$nextStatuses = array_keys($unit->getNextStatuses(false, true));
// the above function call triggers the two methods below to run

It should be allowed to move to unit/collected, because it has a $unit->package->collected=1, however because all the before events are triggered, the $unit->package_id is unset by the first method, therefore the second method can't detect that the package is collected and then invalidates the event.

class UnitWorkflow
{
    public static function beforeLeave_packed($unit, $event)
    {
        // if we move to despatch then unset the package
        if ($event->getEndStatus()->getId() == 'unit/despatch') {
            $unit->package_id = null;
        }
    }
    public static function beforeEnter_collected($unit, $event)
    {
        // we only allow moving to collected if the package is collected
        // the problem is the $unit->package is now empty due to the last check
        if (!$unit->package || !$unit->package->collected) {
            $event->invalidate('The package is not collected.');
        }
    }
}

Perhaps internally in getNextStatuses() you should clone the object.

Or perhaps I shouldn't be using the beforeLeave events to manipulate the model's attributes, and should instead be doing it in the model's beforeSave().

Let me know your thoughts... if this even makes sense. :)

cornernote commented 8 years ago

After thinking about it a bit more I have decided that it's best to not manipulate the model's attributes inside the workflow events. It should be done in the model's beforeSave().

I'll leave this open so you can have a read, but feel free to close it off.

raoul2000 commented 8 years ago

hi @cornernote, indeed the before family events are designed to validate that the action (in your case "leaving unit/packed" status is allowed based in the current state of your model. Modifying anything from this kind of event handler is not a good idea as in the end, the transition may be rejected (i.e. another before event handler invalidates it) but the modification would persist.

So using beforeSave() could be a good solution (other option being the after event family)

ciao