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] [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), + )); } }