Permalink
Browse files

bug #29141 [Workflow] Fixed bug of buildTransitionBlockerList when ma…

…ny transition are enabled (Tetragramat, lyrixx)

This PR was merged into the 4.1 branch.

Discussion
----------

[Workflow] Fixed bug of buildTransitionBlockerList when many transition are enabled

| Q             | A
| ------------- | ---
| Branch?       | 4.1
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #28429 #28432 #28493
| License       | MIT
| Doc PR        |

<!--
Write a short README entry for your feature/bugfix here (replace this comment block.)
This will help people understand your PR and can be used as a start of the Doc PR.
Additionally:
 - Bug fixes must be submitted against the lowest branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against the master branch.
-->

Commits
-------

732f343 [Workflow] Made code simpler
db69ccc method buildTransitionBlockerList returns TransitionBlockerList of expected transition
  • Loading branch information...
lyrixx committed Nov 13, 2018
2 parents ab9b40d + 732f343 commit 28cd88e26199f273dda051175ae16392631d23ae
@@ -3,7 +3,10 @@
namespace Symfony\Component\Workflow\Tests;
use PHPUnit\Framework\TestCase;
use Symfony\Component\EventDispatcher\EventDispatcher;
use Symfony\Component\Workflow\Event\GuardEvent;
use Symfony\Component\Workflow\StateMachine;
use Symfony\Component\Workflow\TransitionBlocker;
class StateMachineTest extends TestCase
{
@@ -38,4 +41,70 @@ public function testCanWithMultipleTransition()
$this->assertTrue($net->can($subject, 't2'));
$this->assertTrue($net->can($subject, 't3'));
}
public function testBuildTransitionBlockerList()
{
$definition = $this->createComplexStateMachineDefinition();
$net = new StateMachine($definition);
$subject = new \stdClass();
$subject->marking = 'a';
$this->assertTrue($net->buildTransitionBlockerList($subject, 't1')->isEmpty());
$subject->marking = 'd';
$this->assertTrue($net->buildTransitionBlockerList($subject, 't1')->isEmpty());
$subject->marking = 'b';
$this->assertFalse($net->buildTransitionBlockerList($subject, 't1')->isEmpty());
}
public function testBuildTransitionBlockerListWithMultipleTransitions()
{
$definition = $this->createComplexStateMachineDefinition();
$net = new StateMachine($definition);
$subject = new \stdClass();
$subject->marking = 'b';
$this->assertTrue($net->buildTransitionBlockerList($subject, 't2')->isEmpty());
$this->assertTrue($net->buildTransitionBlockerList($subject, 't3')->isEmpty());
}
public function testBuildTransitionBlockerListReturnsExpectedReasonOnBranchMerge()
{
$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 \stdClass();
// There may be multiple transitions with the same name. Make sure that transitions
// that are not 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
// blocker list contains guard blocker instead blockedByMarking
$subject->marking = '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
$subject->marking = '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());
}
}
@@ -195,6 +195,32 @@ public function testBuildTransitionBlockerListReturnsUndefinedTransition()
$workflow->buildTransitionBlockerList($subject, '404 Not Found');
}
public function testBuildTransitionBlockerList()
{
$definition = $this->createComplexWorkflowDefinition();
$subject = new \stdClass();
$subject->marking = null;
$workflow = new Workflow($definition, new MultipleStateMarkingStore());
$this->assertTrue($workflow->buildTransitionBlockerList($subject, 't1')->isEmpty());
$this->assertFalse($workflow->buildTransitionBlockerList($subject, 't2')->isEmpty());
$subject->marking = array('b' => 1);
$this->assertFalse($workflow->buildTransitionBlockerList($subject, 't1')->isEmpty());
$this->assertFalse($workflow->buildTransitionBlockerList($subject, 't2')->isEmpty());
$subject->marking = array('b' => 1, 'c' => 1);
$this->assertFalse($workflow->buildTransitionBlockerList($subject, 't1')->isEmpty());
$this->assertTrue($workflow->buildTransitionBlockerList($subject, 't2')->isEmpty());
$subject->marking = array('f' => 1);
$this->assertFalse($workflow->buildTransitionBlockerList($subject, 't5')->isEmpty());
$this->assertTrue($workflow->buildTransitionBlockerList($subject, 't6')->isEmpty());
}
public function testBuildTransitionBlockerListReturnsReasonsProvidedByMarking()
{
$definition = $this->createComplexWorkflowDefinition();
@@ -37,6 +37,17 @@ public function add(TransitionBlocker $blocker): void
$this->blockers[] = $blocker;
}
public function has(string $code): bool
{
foreach ($this->blockers as $blocker) {
if ($code === $blocker->getCode()) {
return true;
}
}
return false;
}
public function clear(): void
{
$this->blockers = array();
@@ -119,7 +119,15 @@ public function buildTransitionBlockerList($subject, string $transitionName): Tr
$transitionBlockerList = $this->buildTransitionBlockerListForTransition($subject, $marking, $transition);
if ($transitionBlockerList->isEmpty()) {
continue;
return $transitionBlockerList;
}
// 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 (!$transitionBlockerList->has(TransitionBlocker::BLOCKED_BY_MARKING)) {
return $transitionBlockerList;
}
}

0 comments on commit 28cd88e

Please sign in to comment.