Skip to content

Commit cc400f8

Browse files
lyrixxlyrixx
committed
bug #34569 [Workflow] Apply the same logic of precedence between the apply() and the buildTransitionBlockerList() method (lyrixx)
This PR was merged into the 4.3 branch. Discussion ---------- [Workflow] Apply the same logic of precedence between the apply() and the buildTransitionBlockerList() method | Q | A | ------------- | --- | Branch? | 4.3 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #34489 | License | MIT | Doc PR | Commits ------- 2ff3496 [Workflow] Apply the same logic of precedence between the apply() and the buildTransitionBlockerList() method
2 parents 2beeea9 + 2ff3496 commit cc400f8

File tree

2 files changed

+62
-22
lines changed

2 files changed

+62
-22
lines changed

‎src/Symfony/Component/Workflow/Tests/StateMachineTest.php

+32-6
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
use PHPUnit\Framework\TestCase;
66
use Symfony\Component\EventDispatcher\EventDispatcher;
77
use Symfony\Component\Workflow\Event\GuardEvent;
8+
use Symfony\Component\Workflow\Exception\NotEnabledTransitionException;
89
use Symfony\Component\Workflow\StateMachine;
910
use Symfony\Component\Workflow\TransitionBlocker;
1011

@@ -84,27 +85,52 @@ public function testBuildTransitionBlockerListReturnsExpectedReasonOnBranchMerge
8485
$subject = new Subject();
8586

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

90-
// Test if when you are in place "a"trying transition "t1" then returned
91+
// Test if when you are in place "a" and trying to apply "t1" then it returns
9192
// blocker list contains guard blocker instead blockedByMarking
9293
$subject->setMarking('a');
9394
$transitionBlockerList = $net->buildTransitionBlockerList($subject, 't1');
9495
$this->assertCount(1, $transitionBlockerList);
9596
$blockers = iterator_to_array($transitionBlockerList);
96-
9797
$this->assertSame('Transition blocker of place a', $blockers[0]->getMessage());
9898
$this->assertSame('blocker', $blockers[0]->getCode());
9999

100-
// Test if when you are in place "d" trying transition "t1" then
101-
// returned blocker list contains guard blocker instead blockedByMarking
100+
// Test if when you are in place "d" and trying to apply "t1" then
101+
// it returns blocker list contains guard blocker instead blockedByMarking
102102
$subject->setMarking('d');
103103
$transitionBlockerList = $net->buildTransitionBlockerList($subject, 't1');
104104
$this->assertCount(1, $transitionBlockerList);
105105
$blockers = iterator_to_array($transitionBlockerList);
106-
107106
$this->assertSame('Transition blocker of place d', $blockers[0]->getMessage());
108107
$this->assertSame('blocker', $blockers[0]->getCode());
109108
}
109+
110+
public function testApplyReturnsExpectedReasonOnBranchMerge()
111+
{
112+
$definition = $this->createComplexStateMachineDefinition();
113+
114+
$dispatcher = new EventDispatcher();
115+
$net = new StateMachine($definition, null, $dispatcher);
116+
117+
$dispatcher->addListener('workflow.guard', function (GuardEvent $event) {
118+
$event->addTransitionBlocker(new TransitionBlocker(sprintf('Transition blocker of place %s', $event->getTransition()->getFroms()[0]), 'blocker'));
119+
});
120+
121+
$subject = new Subject();
122+
123+
// There may be multiple transitions with the same name. Make sure that all transitions
124+
// that are enabled by the marking are evaluated.
125+
// see https://github.com/symfony/symfony/issues/34489
126+
127+
try {
128+
$net->apply($subject, 't1');
129+
$this->fail();
130+
} catch (NotEnabledTransitionException $e) {
131+
$blockers = iterator_to_array($e->getTransitionBlockerList());
132+
$this->assertSame('Transition blocker of place a', $blockers[0]->getMessage());
133+
$this->assertSame('blocker', $blockers[0]->getCode());
134+
}
135+
}
110136
}

‎src/Symfony/Component/Workflow/Workflow.php

+30-16
Original file line numberDiff line numberDiff line change
@@ -159,25 +159,47 @@ public function apply($subject, $transitionName/*, array $context = []*/)
159159

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

162-
$transitionBlockerList = null;
163-
$applied = false;
164-
$approvedTransitionQueue = [];
162+
$transitionExist = false;
163+
$approvedTransitions = [];
164+
$bestTransitionBlockerList = null;
165165

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

171-
$transitionBlockerList = $this->buildTransitionBlockerListForTransition($subject, $marking, $transition);
172-
if (!$transitionBlockerList->isEmpty()) {
171+
$transitionExist = true;
172+
173+
$tmpTransitionBlockerList = $this->buildTransitionBlockerListForTransition($subject, $marking, $transition);
174+
175+
if ($tmpTransitionBlockerList->isEmpty()) {
176+
$approvedTransitions[] = $transition;
177+
continue;
178+
}
179+
180+
if (!$bestTransitionBlockerList) {
181+
$bestTransitionBlockerList = $tmpTransitionBlockerList;
173182
continue;
174183
}
175-
$approvedTransitionQueue[] = $transition;
184+
185+
// We prefer to return transitions blocker by something else than
186+
// marking. Because it means the marking was OK. Transitions are
187+
// deterministic: it's not possible to have many transitions enabled
188+
// at the same time that match the same marking with the same name
189+
if (!$tmpTransitionBlockerList->has(TransitionBlocker::BLOCKED_BY_MARKING)) {
190+
$bestTransitionBlockerList = $tmpTransitionBlockerList;
191+
}
192+
}
193+
194+
if (!$transitionExist) {
195+
throw new UndefinedTransitionException($subject, $transitionName, $this);
176196
}
177197

178-
foreach ($approvedTransitionQueue as $transition) {
179-
$applied = true;
198+
if (!$approvedTransitions) {
199+
throw new NotEnabledTransitionException($subject, $transitionName, $this, $bestTransitionBlockerList);
200+
}
180201

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

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

196-
if (!$transitionBlockerList) {
197-
throw new UndefinedTransitionException($subject, $transitionName, $this);
198-
}
199-
200-
if (!$applied) {
201-
throw new NotEnabledTransitionException($subject, $transitionName, $this, $transitionBlockerList);
202-
}
203-
204218
return $marking;
205219
}
206220

0 commit comments

Comments
 (0)