From db69ccc18580c78cebc7afeff2bff9871da5383d Mon Sep 17 00:00:00 2001 From: Lukas Mencl Date: Mon, 17 Sep 2018 18:47:00 +0200 Subject: [PATCH 1/2] method buildTransitionBlockerList returns TransitionBlockerList of expected transition --- .../Workflow/Tests/StateMachineTest.php | 67 +++++++++++++++++++ .../Component/Workflow/Tests/WorkflowTest.php | 26 +++++++ src/Symfony/Component/Workflow/Workflow.php | 38 +++++++---- 3 files changed, 119 insertions(+), 12 deletions(-) diff --git a/src/Symfony/Component/Workflow/Tests/StateMachineTest.php b/src/Symfony/Component/Workflow/Tests/StateMachineTest.php index 5ece3c36d0..da8156b8fb 100644 --- a/src/Symfony/Component/Workflow/Tests/StateMachineTest.php +++ b/src/Symfony/Component/Workflow/Tests/StateMachineTest.php @@ -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,68 @@ class StateMachineTest extends TestCase $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" applying 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" applying 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()); + } } diff --git a/src/Symfony/Component/Workflow/Tests/WorkflowTest.php b/src/Symfony/Component/Workflow/Tests/WorkflowTest.php index 87f4800248..75ceb1f108 100644 --- a/src/Symfony/Component/Workflow/Tests/WorkflowTest.php +++ b/src/Symfony/Component/Workflow/Tests/WorkflowTest.php @@ -195,6 +195,32 @@ class WorkflowTest extends TestCase $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(); diff --git a/src/Symfony/Component/Workflow/Workflow.php b/src/Symfony/Component/Workflow/Workflow.php index 21676e0fc8..38dd961aba 100644 --- a/src/Symfony/Component/Workflow/Workflow.php +++ b/src/Symfony/Component/Workflow/Workflow.php @@ -92,7 +92,11 @@ class Workflow implements WorkflowInterface continue; } - $transitionBlockerList = $this->buildTransitionBlockerListForTransition($subject, $marking, $transition); + try { + $transitionBlockerList = $this->buildTransitionBlockerListForTransition($subject, $marking, $transition); + } catch (NotEnabledTransitionException $e) { + $transitionBlockerList = $e->getTransitionBlockerList(); + } if ($transitionBlockerList->isEmpty()) { return true; @@ -116,10 +120,13 @@ class Workflow implements WorkflowInterface continue; } - $transitionBlockerList = $this->buildTransitionBlockerListForTransition($subject, $marking, $transition); + try { + $transitionBlockerList = $this->buildTransitionBlockerListForTransition($subject, $marking, $transition); - if ($transitionBlockerList->isEmpty()) { - continue; + return $transitionBlockerList; + } catch (NotEnabledTransitionException $e) { + // a transition with the same name is defined for other places too + $transitionBlockerList = $e->getTransitionBlockerList(); } } @@ -146,11 +153,15 @@ class Workflow implements WorkflowInterface continue; } - $transitionBlockerList = $this->buildTransitionBlockerListForTransition($subject, $marking, $transition); - if (!$transitionBlockerList->isEmpty()) { - continue; + try { + $transitionBlockerList = $this->buildTransitionBlockerListForTransition($subject, $marking, $transition); + } catch (NotEnabledTransitionException $e) { + $transitionBlockerList = $e->getTransitionBlockerList(); + } + + if ($transitionBlockerList->isEmpty()) { + $approvedTransitionQueue[] = $transition; } - $approvedTransitionQueue[] = $transition; } foreach ($approvedTransitionQueue as $transition) { @@ -191,7 +202,12 @@ class Workflow implements WorkflowInterface $marking = $this->getMarking($subject); foreach ($this->definition->getTransitions() as $transition) { - $transitionBlockerList = $this->buildTransitionBlockerListForTransition($subject, $marking, $transition); + try { + $transitionBlockerList = $this->buildTransitionBlockerListForTransition($subject, $marking, $transition); + } catch (NotEnabledTransitionException $e) { + $transitionBlockerList = $e->getTransitionBlockerList(); + } + if ($transitionBlockerList->isEmpty()) { $enabledTransitions[] = $transition; } @@ -236,9 +252,7 @@ class Workflow implements WorkflowInterface { foreach ($transition->getFroms() as $place) { if (!$marking->has($place)) { - return new TransitionBlockerList(array( - TransitionBlocker::createBlockedByMarking($marking), - )); + throw new NotEnabledTransitionException($subject, $transition->getName(), $this, new TransitionBlockerList(array(TransitionBlocker::createBlockedByMarking($marking)))); } } From 732f34357219d79925066862c32dc2bacf59f31f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Pineau?= Date: Thu, 8 Nov 2018 14:40:02 +0100 Subject: [PATCH 2/2] [Workflow] Made code simpler --- .../Workflow/Tests/StateMachineTest.php | 6 ++- .../Workflow/TransitionBlockerList.php | 11 +++++ src/Symfony/Component/Workflow/Workflow.php | 44 ++++++++----------- 3 files changed, 34 insertions(+), 27 deletions(-) diff --git a/src/Symfony/Component/Workflow/Tests/StateMachineTest.php b/src/Symfony/Component/Workflow/Tests/StateMachineTest.php index da8156b8fb..7f76c4a70b 100644 --- a/src/Symfony/Component/Workflow/Tests/StateMachineTest.php +++ b/src/Symfony/Component/Workflow/Tests/StateMachineTest.php @@ -87,7 +87,8 @@ class StateMachineTest extends TestCase // 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" applying transition "t1" then returned blocker list contains guard blocker instead blockedByMarking + // 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); @@ -96,7 +97,8 @@ class StateMachineTest extends TestCase $this->assertSame('Transition blocker of place a', $blockers[0]->getMessage()); $this->assertSame('blocker', $blockers[0]->getCode()); - // Test if when you are in place "d" applying transition "t1" then returned blocker list contains guard blocker instead blockedByMarking + // 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); diff --git a/src/Symfony/Component/Workflow/TransitionBlockerList.php b/src/Symfony/Component/Workflow/TransitionBlockerList.php index 34f9437cda..b7ef5a157c 100644 --- a/src/Symfony/Component/Workflow/TransitionBlockerList.php +++ b/src/Symfony/Component/Workflow/TransitionBlockerList.php @@ -37,6 +37,17 @@ final class TransitionBlockerList implements \IteratorAggregate, \Countable $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(); diff --git a/src/Symfony/Component/Workflow/Workflow.php b/src/Symfony/Component/Workflow/Workflow.php index 38dd961aba..11d82daa01 100644 --- a/src/Symfony/Component/Workflow/Workflow.php +++ b/src/Symfony/Component/Workflow/Workflow.php @@ -92,11 +92,7 @@ class Workflow implements WorkflowInterface continue; } - try { - $transitionBlockerList = $this->buildTransitionBlockerListForTransition($subject, $marking, $transition); - } catch (NotEnabledTransitionException $e) { - $transitionBlockerList = $e->getTransitionBlockerList(); - } + $transitionBlockerList = $this->buildTransitionBlockerListForTransition($subject, $marking, $transition); if ($transitionBlockerList->isEmpty()) { return true; @@ -120,13 +116,18 @@ class Workflow implements WorkflowInterface continue; } - try { - $transitionBlockerList = $this->buildTransitionBlockerListForTransition($subject, $marking, $transition); + $transitionBlockerList = $this->buildTransitionBlockerListForTransition($subject, $marking, $transition); + if ($transitionBlockerList->isEmpty()) { + 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; - } catch (NotEnabledTransitionException $e) { - // a transition with the same name is defined for other places too - $transitionBlockerList = $e->getTransitionBlockerList(); } } @@ -153,15 +154,11 @@ class Workflow implements WorkflowInterface continue; } - try { - $transitionBlockerList = $this->buildTransitionBlockerListForTransition($subject, $marking, $transition); - } catch (NotEnabledTransitionException $e) { - $transitionBlockerList = $e->getTransitionBlockerList(); - } - - if ($transitionBlockerList->isEmpty()) { - $approvedTransitionQueue[] = $transition; + $transitionBlockerList = $this->buildTransitionBlockerListForTransition($subject, $marking, $transition); + if (!$transitionBlockerList->isEmpty()) { + continue; } + $approvedTransitionQueue[] = $transition; } foreach ($approvedTransitionQueue as $transition) { @@ -202,12 +199,7 @@ class Workflow implements WorkflowInterface $marking = $this->getMarking($subject); foreach ($this->definition->getTransitions() as $transition) { - try { - $transitionBlockerList = $this->buildTransitionBlockerListForTransition($subject, $marking, $transition); - } catch (NotEnabledTransitionException $e) { - $transitionBlockerList = $e->getTransitionBlockerList(); - } - + $transitionBlockerList = $this->buildTransitionBlockerListForTransition($subject, $marking, $transition); if ($transitionBlockerList->isEmpty()) { $enabledTransitions[] = $transition; } @@ -252,7 +244,9 @@ class Workflow implements WorkflowInterface { foreach ($transition->getFroms() as $place) { if (!$marking->has($place)) { - throw new NotEnabledTransitionException($subject, $transition->getName(), $this, new TransitionBlockerList(array(TransitionBlocker::createBlockedByMarking($marking)))); + return new TransitionBlockerList(array( + TransitionBlocker::createBlockedByMarking($marking), + )); } }