[Workflow] Made code simpler

symfony · Nov 12, 2018 · 732f343 · 732f343
1 parent db69ccc
commit 732f343
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 27 deletions.
diff --git a/src/Symfony/Component/Workflow/Tests/StateMachineTest.php b/src/Symfony/Component/Workflow/Tests/StateMachineTest.php
@@ -87,7 +87,8 @@ public function testBuildTransitionBlockerListReturnsExpectedReasonOnBranchMerge
         // 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 @@ public function testBuildTransitionBlockerListReturnsExpectedReasonOnBranchMerge
         $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
@@ -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();

diff --git a/src/Symfony/Component/Workflow/Workflow.php b/src/Symfony/Component/Workflow/Workflow.php
@@ -92,11 +92,7 @@ public function can($subject, $transitionName)
                 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 @@ public function buildTransitionBlockerList($subject, string $transitionName): Tr
                 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 @@ public function apply($subject, $transitionName)
                 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 @@ public function getEnabledTransitions($subject)
         $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 @@ private function buildTransitionBlockerListForTransition($subject, Marking $mark
     {
         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),
+                ));
             }
         }