Skip to content

[Workflow] Apply the same logic of precedence between the apply() and the buildTransitionBlockerList() method #34569

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 25, 2019
Merged

[Workflow] Apply the same logic of precedence between the apply() and the buildTransitionBlockerList() method #34569

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 32 additions & 6 deletions src/Symfony/Component/Workflow/Tests/StateMachineTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use PHPUnit\Framework\TestCase;
use Symfony\Component\EventDispatcher\EventDispatcher;
use Symfony\Component\Workflow\Event\GuardEvent;
use Symfony\Component\Workflow\Exception\NotEnabledTransitionException;
use Symfony\Component\Workflow\StateMachine;
use Symfony\Component\Workflow\TransitionBlocker;

Expand Down Expand Up @@ -84,27 +85,52 @@ public function testBuildTransitionBlockerListReturnsExpectedReasonOnBranchMerge
$subject = new Subject();

// There may be multiple transitions with the same name. Make sure that transitions
// that are not enabled by the marking are evaluated.
// that are enabled by the marking are evaluated.
// see https://github.com/symfony/symfony/issues/28432

// Test if when you are in place "a"trying transition "t1" then returned
// Test if when you are in place "a" and trying to apply "t1" then it returns
// blocker list contains guard blocker instead blockedByMarking
$subject->setMarking('a');
$transitionBlockerList = $net->buildTransitionBlockerList($subject, 't1');
$this->assertCount(1, $transitionBlockerList);
$blockers = iterator_to_array($transitionBlockerList);

$this->assertSame('Transition blocker of place a', $blockers[0]->getMessage());
$this->assertSame('blocker', $blockers[0]->getCode());

// Test if when you are in place "d" trying transition "t1" then
// returned blocker list contains guard blocker instead blockedByMarking
// Test if when you are in place "d" and trying to apply "t1" then
// it returns blocker list contains guard blocker instead blockedByMarking
$subject->setMarking('d');
$transitionBlockerList = $net->buildTransitionBlockerList($subject, 't1');
$this->assertCount(1, $transitionBlockerList);
$blockers = iterator_to_array($transitionBlockerList);

$this->assertSame('Transition blocker of place d', $blockers[0]->getMessage());
$this->assertSame('blocker', $blockers[0]->getCode());
}

public function testApplyReturnsExpectedReasonOnBranchMerge()
{
$definition = $this->createComplexStateMachineDefinition();

$dispatcher = new EventDispatcher();
$net = new StateMachine($definition, null, $dispatcher);

$dispatcher->addListener('workflow.guard', function (GuardEvent $event) {
$event->addTransitionBlocker(new TransitionBlocker(sprintf('Transition blocker of place %s', $event->getTransition()->getFroms()[0]), 'blocker'));
});

$subject = new Subject();

// There may be multiple transitions with the same name. Make sure that all transitions
// that are enabled by the marking are evaluated.
// see https://github.com/symfony/symfony/issues/34489

try {
$net->apply($subject, 't1');
$this->fail();
} catch (NotEnabledTransitionException $e) {
$blockers = iterator_to_array($e->getTransitionBlockerList());
$this->assertSame('Transition blocker of place a', $blockers[0]->getMessage());
$this->assertSame('blocker', $blockers[0]->getCode());
}
}
}
46 changes: 30 additions & 16 deletions src/Symfony/Component/Workflow/Workflow.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,25 +159,47 @@ public function apply($subject, $transitionName/*, array $context = []*/)

$marking = $this->getMarking($subject);

$transitionBlockerList = null;
$applied = false;
$approvedTransitionQueue = [];
$transitionExist = false;
$approvedTransitions = [];
$bestTransitionBlockerList = null;

foreach ($this->definition->getTransitions() as $transition) {
if ($transition->getName() !== $transitionName) {
continue;
}

$transitionBlockerList = $this->buildTransitionBlockerListForTransition($subject, $marking, $transition);
if (!$transitionBlockerList->isEmpty()) {
$transitionExist = true;

$tmpTransitionBlockerList = $this->buildTransitionBlockerListForTransition($subject, $marking, $transition);

if ($tmpTransitionBlockerList->isEmpty()) {
$approvedTransitions[] = $transition;
continue;
}

if (!$bestTransitionBlockerList) {
$bestTransitionBlockerList = $tmpTransitionBlockerList;
continue;
}
$approvedTransitionQueue[] = $transition;

// We prefer to return transitions blocker by something else than
// marking. Because it means the marking was OK. Transitions are
// deterministic: it's not possible to have many transitions enabled
// at the same time that match the same marking with the same name
if (!$tmpTransitionBlockerList->has(TransitionBlocker::BLOCKED_BY_MARKING)) {
$bestTransitionBlockerList = $tmpTransitionBlockerList;
}
}

if (!$transitionExist) {
throw new UndefinedTransitionException($subject, $transitionName, $this);
}

foreach ($approvedTransitionQueue as $transition) {
$applied = true;
if (!$approvedTransitions) {
throw new NotEnabledTransitionException($subject, $transitionName, $this, $bestTransitionBlockerList);
}

foreach ($approvedTransitions as $transition) {
$this->leave($subject, $transition, $marking);

$context = $this->transition($subject, $transition, $marking, $context);
Expand All @@ -193,14 +215,6 @@ public function apply($subject, $transitionName/*, array $context = []*/)
$this->announce($subject, $transition, $marking);
}

if (!$transitionBlockerList) {
throw new UndefinedTransitionException($subject, $transitionName, $this);
}

if (!$applied) {
throw new NotEnabledTransitionException($subject, $transitionName, $this, $transitionBlockerList);
}

return $marking;
}

Expand Down