Skip to content

Commit edd5431

Browse files
lyrixxlyrixx
committed
[Workflow] Fixed support of multiple transition with the same name.
The previous behavior was underterministic because it took the first transition during the `can` and the `apply` method. But the "first" does not mean anything. Now the workflow apply all possible transitions with the same name.
1 parent 39d975b commit edd5431

File tree

3 files changed

+177
-74
lines changed

3 files changed

+177
-74
lines changed

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

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,34 @@ private function createSimpleWorkflowDefinition()
4949
// +---+ +----+ +---+ +----+ +---+
5050
}
5151

52+
private function createWorkflowWithSameNameTransition()
53+
{
54+
$places = range('a', 'c');
55+
56+
$transitions = array();
57+
$transitions[] = new Transition('a_to_bc', 'a', array('b', 'c'));
58+
$transitions[] = new Transition('b_to_c', 'b', 'c');
59+
$transitions[] = new Transition('to_a', 'b', 'a');
60+
$transitions[] = new Transition('to_a', 'c', 'a');
61+
62+
return new Definition($places, $transitions);
63+
64+
// The graph looks like:
65+
// +------------------------------------------------------------+
66+
// | |
67+
// | |
68+
// | +----------------------------------------+ |
69+
// v | v |
70+
// +---+ +---------+ +---+ +--------+ +---+ +------+
71+
// | a | --> | a_to_bc | --> | b | --> | b_to_c | --> | c | --> | to_a | -+
72+
// +---+ +---------+ +---+ +--------+ +---+ +------+ |
73+
// ^ | ^ |
74+
// | +----------------------------------+ |
75+
// | |
76+
// | |
77+
// +--------------------------------------------------------------------+
78+
}
79+
5280
private function createComplexStateMachineDefinition()
5381
{
5482
$places = array('a', 'b', 'c', 'd');

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

Lines changed: 99 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ public function testGetMarkingWithEmptyDefinition()
4848
public function testGetMarkingWithImpossiblePlace()
4949
{
5050
$subject = new \stdClass();
51-
$subject->marking = array('nope' => true);
51+
$subject->marking = array('nope' => 1);
5252
$workflow = new Workflow(new Definition(array(), array()), new MultipleStateMarkingStore());
5353

5454
$workflow->getMarking($subject);
@@ -83,18 +83,14 @@ public function testGetMarkingWithExistingMarking()
8383
$this->assertTrue($marking->has('c'));
8484
}
8585

86-
/**
87-
* @expectedException \Symfony\Component\Workflow\Exception\LogicException
88-
* @expectedExceptionMessage Transition "foobar" does not exist for workflow "unnamed".
89-
*/
9086
public function testCanWithUnexistingTransition()
9187
{
9288
$definition = $this->createComplexWorkflowDefinition();
9389
$subject = new \stdClass();
9490
$subject->marking = null;
9591
$workflow = new Workflow($definition, new MultipleStateMarkingStore());
9692

97-
$workflow->can($subject, 'foobar');
93+
$this->assertFalse($workflow->can($subject, 'foobar'));
9894
}
9995

10096
public function testCan()
@@ -136,6 +132,23 @@ public function testApplyWithImpossibleTransition()
136132
$workflow->apply($subject, 't2');
137133
}
138134

135+
public function testCanWithSameNameTransition()
136+
{
137+
$definition = $this->createWorkflowWithSameNameTransition();
138+
$workflow = new Workflow($definition, new MultipleStateMarkingStore());
139+
140+
$subject = new \stdClass();
141+
$subject->marking = null;
142+
$this->assertTrue($workflow->can($subject, 'a_to_bc'));
143+
$this->assertFalse($workflow->can($subject, 'b_to_c'));
144+
$this->assertFalse($workflow->can($subject, 'to_a'));
145+
146+
$subject->marking = array('b' => 1);
147+
$this->assertFalse($workflow->can($subject, 'a_to_bc'));
148+
$this->assertTrue($workflow->can($subject, 'b_to_c'));
149+
$this->assertTrue($workflow->can($subject, 'to_a'));
150+
}
151+
139152
public function testApply()
140153
{
141154
$definition = $this->createComplexWorkflowDefinition();
@@ -151,6 +164,59 @@ public function testApply()
151164
$this->assertTrue($marking->has('c'));
152165
}
153166

167+
public function testApplyWithSameNameTransition()
168+
{
169+
$subject = new \stdClass();
170+
$subject->marking = null;
171+
$definition = $this->createWorkflowWithSameNameTransition();
172+
$workflow = new Workflow($definition, new MultipleStateMarkingStore());
173+
174+
$marking = $workflow->apply($subject, 'a_to_bc');
175+
176+
$this->assertFalse($marking->has('a'));
177+
$this->assertTrue($marking->has('b'));
178+
$this->assertTrue($marking->has('c'));
179+
180+
$marking = $workflow->apply($subject, 'to_a');
181+
182+
$this->assertTrue($marking->has('a'));
183+
$this->assertFalse($marking->has('b'));
184+
$this->assertFalse($marking->has('c'));
185+
186+
$marking = $workflow->apply($subject, 'a_to_bc');
187+
$marking = $workflow->apply($subject, 'b_to_c');
188+
189+
$this->assertFalse($marking->has('a'));
190+
$this->assertFalse($marking->has('b'));
191+
$this->assertTrue($marking->has('c'));
192+
193+
$marking = $workflow->apply($subject, 'to_a');
194+
195+
$this->assertTrue($marking->has('a'));
196+
$this->assertFalse($marking->has('b'));
197+
$this->assertFalse($marking->has('c'));
198+
}
199+
200+
public function testApplyWithSameNameTransition2()
201+
{
202+
$subject = new \stdClass();
203+
$subject->marking = array('a' => 1, 'b' => 1);
204+
205+
$places = range('a', 'd');
206+
$transitions = array();
207+
$transitions[] = new Transition('t', 'a', 'c');
208+
$transitions[] = new Transition('t', 'b', 'd');
209+
$definition = new Definition($places, $transitions);
210+
$workflow = new Workflow($definition, new MultipleStateMarkingStore());
211+
212+
$marking = $workflow->apply($subject, 't');
213+
214+
$this->assertFalse($marking->has('a'));
215+
$this->assertFalse($marking->has('b'));
216+
$this->assertTrue($marking->has('c'));
217+
$this->assertTrue($marking->has('d'));
218+
}
219+
154220
public function testApplyWithEventDispatcher()
155221
{
156222
$definition = $this->createComplexWorkflowDefinition();
@@ -198,17 +264,36 @@ public function testGetEnabledTransitions()
198264

199265
$this->assertEmpty($workflow->getEnabledTransitions($subject));
200266

201-
$subject->marking = array('d' => true);
267+
$subject->marking = array('d' => 1);
202268
$transitions = $workflow->getEnabledTransitions($subject);
203269
$this->assertCount(2, $transitions);
204270
$this->assertSame('t3', $transitions[0]->getName());
205271
$this->assertSame('t4', $transitions[1]->getName());
206272

207-
$subject->marking = array('c' => true, 'e' => true);
273+
$subject->marking = array('c' => 1, 'e' => 1);
208274
$transitions = $workflow->getEnabledTransitions($subject);
209275
$this->assertCount(1, $transitions);
210276
$this->assertSame('t5', $transitions[0]->getName());
211277
}
278+
279+
public function testGetEnabledTransitionsWithSameNameTransition()
280+
{
281+
$definition = $this->createWorkflowWithSameNameTransition();
282+
$subject = new \stdClass();
283+
$subject->marking = null;
284+
$workflow = new Workflow($definition, new MultipleStateMarkingStore());
285+
286+
$transitions = $workflow->getEnabledTransitions($subject);
287+
$this->assertCount(1, $transitions);
288+
$this->assertSame('a_to_bc', $transitions[0]->getName());
289+
290+
$subject->marking = array('b' => 1, 'c' => 1);
291+
$transitions = $workflow->getEnabledTransitions($subject);
292+
$this->assertCount(3, $transitions);
293+
$this->assertSame('b_to_c', $transitions[0]->getName());
294+
$this->assertSame('to_a', $transitions[1]->getName());
295+
$this->assertSame('to_a', $transitions[2]->getName());
296+
}
212297
}
213298

214299
class EventDispatcherMock implements \Symfony\Component\EventDispatcher\EventDispatcherInterface
@@ -223,21 +308,27 @@ public function dispatch($eventName, \Symfony\Component\EventDispatcher\Event $e
223308
public function addListener($eventName, $listener, $priority = 0)
224309
{
225310
}
311+
226312
public function addSubscriber(\Symfony\Component\EventDispatcher\EventSubscriberInterface $subscriber)
227313
{
228314
}
315+
229316
public function removeListener($eventName, $listener)
230317
{
231318
}
319+
232320
public function removeSubscriber(\Symfony\Component\EventDispatcher\EventSubscriberInterface $subscriber)
233321
{
234322
}
323+
235324
public function getListeners($eventName = null)
236325
{
237326
}
327+
238328
public function getListenerPriority($eventName, $listener)
239329
{
240330
}
331+
241332
public function hasListeners($eventName = null)
242333
{
243334
}

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

Lines changed: 50 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -89,15 +89,18 @@ public function getMarking($subject)
8989
* @param string $transitionName A transition
9090
*
9191
* @return bool true if the transition is enabled
92-
*
93-
* @throws LogicException If the transition does not exist
9492
*/
9593
public function can($subject, $transitionName)
9694
{
97-
$transitions = $this->getTransitions($transitionName);
98-
$marking = $this->getMarking($subject);
95+
$transitions = $this->getEnabledTransitions($subject, $this->getMarking($subject));
96+
97+
foreach ($transitions as $transition) {
98+
if ($transitionName === $transition->getName()) {
99+
return true;
100+
}
101+
}
99102

100-
return null !== $this->getTransitionForSubject($subject, $marking, $transitions);
103+
return false;
101104
}
102105

103106
/**
@@ -113,22 +116,36 @@ public function can($subject, $transitionName)
113116
*/
114117
public function apply($subject, $transitionName)
115118
{
116-
$transitions = $this->getTransitions($transitionName);
117-
$marking = $this->getMarking($subject);
119+
$transitions = $this->getEnabledTransitions($subject, $this->getMarking($subject));
118120

119-
if (null === $transition = $this->getTransitionForSubject($subject, $marking, $transitions)) {
120-
throw new LogicException(sprintf('Unable to apply transition "%s" for workflow "%s".', $transitionName, $this->name));
121-
}
121+
// We can shortcut the getMarking method in order to boost performance,
122+
// since the "getEnabledTransitions" method already checks the Marking
123+
// state
124+
$marking = $this->markingStore->getMarking($subject);
122125

123-
$this->leave($subject, $transition, $marking);
126+
$applied = false;
124127

125-
$this->transition($subject, $transition, $marking);
128+
foreach ($transitions as $transition) {
129+
if ($transitionName !== $transition->getName()) {
130+
continue;
131+
}
126132

127-
$this->enter($subject, $transition, $marking);
133+
$applied = true;
128134

129-
$this->markingStore->setMarking($subject, $marking);
135+
$this->leave($subject, $transition, $marking);
130136

131-
$this->announce($subject, $transition, $marking);
137+
$this->transition($subject, $transition, $marking);
138+
139+
$this->enter($subject, $transition, $marking);
140+
141+
$this->markingStore->setMarking($subject, $marking);
142+
143+
$this->announce($subject, $transition, $marking);
144+
}
145+
146+
if (!$applied) {
147+
throw new LogicException(sprintf('Unable to apply transition "%s" for workflow "%s".', $transitionName, $this->name));
148+
}
132149

133150
return $marking;
134151
}
@@ -146,7 +163,7 @@ public function getEnabledTransitions($subject)
146163
$marking = $this->getMarking($subject);
147164

148165
foreach ($this->definition->getTransitions() as $transition) {
149-
if (null !== $this->getTransitionForSubject($subject, $marking, array($transition))) {
166+
if ($this->doCan($subject, $marking, $transition)) {
150167
$enabled[] = $transition;
151168
}
152169
}
@@ -167,6 +184,21 @@ public function getDefinition()
167184
return $this->definition;
168185
}
169186

187+
private function doCan($subject, Marking $marking, Transition $transition)
188+
{
189+
foreach ($transition->getFroms() as $place) {
190+
if (!$marking->has($place)) {
191+
return false;
192+
}
193+
}
194+
195+
if (true === $this->guardTransition($subject, $marking, $transition)) {
196+
return false;
197+
}
198+
199+
return true;
200+
}
201+
170202
/**
171203
* @param object $subject
172204
* @param Marking $marking
@@ -246,56 +278,8 @@ private function announce($subject, Transition $initialTransition, Marking $mark
246278

247279
$event = new Event($subject, $marking, $initialTransition);
248280

249-
foreach ($this->definition->getTransitions() as $transition) {
250-
if (null !== $this->getTransitionForSubject($subject, $marking, array($transition))) {
251-
$this->dispatcher->dispatch(sprintf('workflow.%s.announce.%s', $this->name, $transition->getName()), $event);
252-
}
253-
}
254-
}
255-
256-
/**
257-
* @param $transitionName
258-
*
259-
* @return Transition[]
260-
*/
261-
private function getTransitions($transitionName)
262-
{
263-
$transitions = $this->definition->getTransitions();
264-
265-
$transitions = array_filter($transitions, function (Transition $transition) use ($transitionName) {
266-
return $transitionName === $transition->getName();
267-
});
268-
269-
if (!$transitions) {
270-
throw new LogicException(sprintf('Transition "%s" does not exist for workflow "%s".', $transitionName, $this->name));
271-
}
272-
273-
return $transitions;
274-
}
275-
276-
/**
277-
* Return the first Transition in $transitions that is valid for the
278-
* $subject and $marking. null is returned when you cannot do any Transition
279-
* in $transitions on the $subject.
280-
*
281-
* @param object $subject
282-
* @param Marking $marking
283-
* @param Transition[] $transitions
284-
*
285-
* @return Transition|null
286-
*/
287-
private function getTransitionForSubject($subject, Marking $marking, array $transitions)
288-
{
289-
foreach ($transitions as $transition) {
290-
foreach ($transition->getFroms() as $place) {
291-
if (!$marking->has($place)) {
292-
continue 2;
293-
}
294-
}
295-
296-
if (true !== $this->guardTransition($subject, $marking, $transition)) {
297-
return $transition;
298-
}
281+
foreach ($this->getEnabledTransitions($subject) as $transition) {
282+
$this->dispatcher->dispatch(sprintf('workflow.%s.announce.%s', $this->name, $transition->getName()), $event);
299283
}
300284
}
301285
}

0 commit comments

Comments
 (0)