From c6d862474362813eea43f9bfc9e588c486265d38 Mon Sep 17 00:00:00 2001 From: Roland Franssen Date: Sun, 26 Jun 2016 07:31:53 +0000 Subject: [PATCH 1/5] minor changes --- src/Symfony/Component/Workflow/Definition.php | 4 +--- src/Symfony/Component/Workflow/Event/Event.php | 4 +--- .../Workflow/EventListener/AuditTrailListener.php | 6 +++--- .../MarkingStore/PropertyAccessorMarkingStore.php | 1 - .../Workflow/MarkingStore/ScalarMarkingStore.php | 1 - src/Symfony/Component/Workflow/Registry.php | 14 +++++++------- .../Tests/EventListener/AuditTrailListenerTest.php | 6 +++--- src/Symfony/Component/Workflow/Transition.php | 2 -- src/Symfony/Component/Workflow/Workflow.php | 2 -- 9 files changed, 15 insertions(+), 25 deletions(-) diff --git a/src/Symfony/Component/Workflow/Definition.php b/src/Symfony/Component/Workflow/Definition.php index 87fe974b15ccb..ec4234e4f87f5 100644 --- a/src/Symfony/Component/Workflow/Definition.php +++ b/src/Symfony/Component/Workflow/Definition.php @@ -21,9 +21,7 @@ class Definition { private $places = array(); - private $transitions = array(); - private $initialPlace; /** @@ -68,7 +66,7 @@ public function addPlace($place) throw new InvalidArgumentException(sprintf('The place "%s" contains invalid characters.', $place)); } - if (!count($this->places)) { + if (!$this->places) { $this->initialPlace = $place; } diff --git a/src/Symfony/Component/Workflow/Event/Event.php b/src/Symfony/Component/Workflow/Event/Event.php index a690b2b330e7a..8a307d731a3cb 100644 --- a/src/Symfony/Component/Workflow/Event/Event.php +++ b/src/Symfony/Component/Workflow/Event/Event.php @@ -22,15 +22,13 @@ class Event extends BaseEvent { private $subject; - private $marking; - private $transition; /** * Event constructor. * - * @param mixed $subject + * @param object $subject * @param Marking $marking * @param Transition $transition */ diff --git a/src/Symfony/Component/Workflow/EventListener/AuditTrailListener.php b/src/Symfony/Component/Workflow/EventListener/AuditTrailListener.php index a39c89d18c3cb..b32833f2d26df 100644 --- a/src/Symfony/Component/Workflow/EventListener/AuditTrailListener.php +++ b/src/Symfony/Component/Workflow/EventListener/AuditTrailListener.php @@ -30,19 +30,19 @@ public function __construct(LoggerInterface $logger) public function onLeave(Event $event) { foreach ($event->getTransition()->getFroms() as $place) { - $this->logger->info(sprintf('leaving "%s" for subject of class "%s"', $place, get_class($event->getSubject()))); + $this->logger->info(sprintf('Leaving "%s" for subject of class "%s".', $place, get_class($event->getSubject()))); } } public function onTransition(Event $event) { - $this->logger->info(sprintf('transition "%s" for subject of class "%s"', $event->getTransition()->getName(), get_class($event->getSubject()))); + $this->logger->info(sprintf('Transition "%s" for subject of class "%s".', $event->getTransition()->getName(), get_class($event->getSubject()))); } public function onEnter(Event $event) { foreach ($event->getTransition()->getTos() as $place) { - $this->logger->info(sprintf('entering "%s" for subject of class "%s"', $place, get_class($event->getSubject()))); + $this->logger->info(sprintf('Entering "%s" for subject of class "%s".', $place, get_class($event->getSubject()))); } } diff --git a/src/Symfony/Component/Workflow/MarkingStore/PropertyAccessorMarkingStore.php b/src/Symfony/Component/Workflow/MarkingStore/PropertyAccessorMarkingStore.php index faf1e8a6c4024..8972b69b6c2e4 100644 --- a/src/Symfony/Component/Workflow/MarkingStore/PropertyAccessorMarkingStore.php +++ b/src/Symfony/Component/Workflow/MarkingStore/PropertyAccessorMarkingStore.php @@ -23,7 +23,6 @@ class PropertyAccessorMarkingStore implements MarkingStoreInterface { private $property; - private $propertyAccessor; /** diff --git a/src/Symfony/Component/Workflow/MarkingStore/ScalarMarkingStore.php b/src/Symfony/Component/Workflow/MarkingStore/ScalarMarkingStore.php index 949661ee10627..c76fb4081c9dd 100644 --- a/src/Symfony/Component/Workflow/MarkingStore/ScalarMarkingStore.php +++ b/src/Symfony/Component/Workflow/MarkingStore/ScalarMarkingStore.php @@ -23,7 +23,6 @@ class ScalarMarkingStore implements MarkingStoreInterface, UniqueTransitionOutputInterface { private $property; - private $propertyAccessor; /** diff --git a/src/Symfony/Component/Workflow/Registry.php b/src/Symfony/Component/Workflow/Registry.php index 2f94288ebb9c5..619ed5f66a5b1 100644 --- a/src/Symfony/Component/Workflow/Registry.php +++ b/src/Symfony/Component/Workflow/Registry.php @@ -23,19 +23,19 @@ class Registry /** * @param Workflow $workflow - * @param string $classname + * @param string $className */ - public function add(Workflow $workflow, $classname) + public function add(Workflow $workflow, $className) { - $this->workflows[] = array($workflow, $classname); + $this->workflows[] = array($workflow, $className); } public function get($subject, $workflowName = null) { $matched = null; - foreach ($this->workflows as list($workflow, $classname)) { - if ($this->supports($workflow, $classname, $subject, $workflowName)) { + foreach ($this->workflows as list($workflow, $className)) { + if ($this->supports($workflow, $className, $subject, $workflowName)) { if ($matched) { throw new InvalidArgumentException('At least two workflows match this subject. Set a different name on each and use the second (name) argument of this method.'); } @@ -50,9 +50,9 @@ public function get($subject, $workflowName = null) return $matched; } - private function supports(Workflow $workflow, $classname, $subject, $name) + private function supports(Workflow $workflow, $className, $subject, $name) { - if (!$subject instanceof $classname) { + if (!$subject instanceof $className) { return false; } diff --git a/src/Symfony/Component/Workflow/Tests/EventListener/AuditTrailListenerTest.php b/src/Symfony/Component/Workflow/Tests/EventListener/AuditTrailListenerTest.php index 0422009f0ee24..319a9119f5e02 100644 --- a/src/Symfony/Component/Workflow/Tests/EventListener/AuditTrailListenerTest.php +++ b/src/Symfony/Component/Workflow/Tests/EventListener/AuditTrailListenerTest.php @@ -34,9 +34,9 @@ public function testItWorks() $workflow->apply($object, 't1'); $expected = array( - 'leaving "a" for subject of class "stdClass"', - 'transition "t1" for subject of class "stdClass"', - 'entering "b" for subject of class "stdClass"', + 'Leaving "a" for subject of class "stdClass".', + 'Transition "t1" for subject of class "stdClass".', + 'Entering "b" for subject of class "stdClass".', ); $this->assertSame($expected, $logger->logs); diff --git a/src/Symfony/Component/Workflow/Transition.php b/src/Symfony/Component/Workflow/Transition.php index 30cc5eca47d82..7e44f0b315a89 100644 --- a/src/Symfony/Component/Workflow/Transition.php +++ b/src/Symfony/Component/Workflow/Transition.php @@ -20,9 +20,7 @@ class Transition { private $name; - private $froms; - private $tos; /** diff --git a/src/Symfony/Component/Workflow/Workflow.php b/src/Symfony/Component/Workflow/Workflow.php index 0bf637e3d7ebf..9787f7ea45709 100644 --- a/src/Symfony/Component/Workflow/Workflow.php +++ b/src/Symfony/Component/Workflow/Workflow.php @@ -109,7 +109,6 @@ public function can($subject, $transitionName) } $transition = $transitions[$transitionName]; - $marking = $this->getMarking($subject); return $this->doCan($subject, $marking, $transition); @@ -161,7 +160,6 @@ public function apply($subject, $transitionName) public function getEnabledTransitions($subject) { $enabled = array(); - $marking = $this->getMarking($subject); foreach ($this->definition->getTransitions() as $transition) { From 3c95e5f81077af1ac476cf1f9f6e8b3874d4fa0e Mon Sep 17 00:00:00 2001 From: Roland Franssen Date: Sun, 26 Jun 2016 07:42:10 +0000 Subject: [PATCH 2/5] removed safeguard --- src/Symfony/Component/Workflow/Workflow.php | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/Symfony/Component/Workflow/Workflow.php b/src/Symfony/Component/Workflow/Workflow.php index 9787f7ea45709..438d5b3b67e27 100644 --- a/src/Symfony/Component/Workflow/Workflow.php +++ b/src/Symfony/Component/Workflow/Workflow.php @@ -59,10 +59,6 @@ public function getMarking($subject) { $marking = $this->markingStore->getMarking($subject); - if (!$marking instanceof Marking) { - throw new LogicException(sprintf('The value returned by the MarkingStore is not an instance of "%s" for workflow "%s".', Marking::class, $this->name)); - } - // check if the subject is already in the workflow if (!$marking->getPlaces()) { if (!$this->definition->getInitialPlace()) { @@ -134,7 +130,6 @@ public function apply($subject, $transitionName) // We can shortcut the getMarking method in order to boost performance, // since the "can" method already checks the Marking state $marking = $this->markingStore->getMarking($subject); - $transition = $this->definition->getTransitions()[$transitionName]; $this->leave($subject, $transition, $marking); From 7bbf948d2499c0d5dfa5a0259cd3f9bbc3ea81d6 Mon Sep 17 00:00:00 2001 From: Roland Franssen Date: Sun, 26 Jun 2016 07:57:53 +0000 Subject: [PATCH 3/5] fixed tests --- .../Workflow/Tests/DefinitionTest.php | 6 ++--- .../Component/Workflow/Tests/RegistryTest.php | 2 +- .../Workflow/Tests/TransitionTest.php | 2 +- .../Component/Workflow/Tests/WorkflowTest.php | 24 ++++--------------- 4 files changed, 10 insertions(+), 24 deletions(-) diff --git a/src/Symfony/Component/Workflow/Tests/DefinitionTest.php b/src/Symfony/Component/Workflow/Tests/DefinitionTest.php index 4a4465fd64a10..63d196d872c08 100644 --- a/src/Symfony/Component/Workflow/Tests/DefinitionTest.php +++ b/src/Symfony/Component/Workflow/Tests/DefinitionTest.php @@ -28,7 +28,7 @@ public function testSetInitialPlace() } /** - * @expectedException Symfony\Component\Workflow\Exception\LogicException + * @expectedException \Symfony\Component\Workflow\Exception\LogicException * @expectedExceptionMessage Place "d" cannot be the initial place as it does not exist. */ public function testSetInitialPlaceAndPlaceIsNotDefined() @@ -50,7 +50,7 @@ public function testAddTransition() } /** - * @expectedException Symfony\Component\Workflow\Exception\LogicException + * @expectedException \Symfony\Component\Workflow\Exception\LogicException * @expectedExceptionMessage Place "c" referenced in transition "name" does not exist. */ public function testAddTransitionAndFromPlaceIsNotDefined() @@ -61,7 +61,7 @@ public function testAddTransitionAndFromPlaceIsNotDefined() } /** - * @expectedException Symfony\Component\Workflow\Exception\LogicException + * @expectedException \Symfony\Component\Workflow\Exception\LogicException * @expectedExceptionMessage Place "c" referenced in transition "name" does not exist. */ public function testAddTransitionAndToPlaceIsNotDefined() diff --git a/src/Symfony/Component/Workflow/Tests/RegistryTest.php b/src/Symfony/Component/Workflow/Tests/RegistryTest.php index 719886dd1c03f..41bfbab8b99e3 100644 --- a/src/Symfony/Component/Workflow/Tests/RegistryTest.php +++ b/src/Symfony/Component/Workflow/Tests/RegistryTest.php @@ -44,7 +44,7 @@ public function testGetWithSuccess() } /** - * @expectedException Symfony\Component\Workflow\Exception\InvalidArgumentException + * @expectedException \Symfony\Component\Workflow\Exception\InvalidArgumentException * @expectedExceptionMessage At least two workflows match this subject. Set a different name on each and use the second (name) argument of this method. */ public function testGetWithMultipleMatch() diff --git a/src/Symfony/Component/Workflow/Tests/TransitionTest.php b/src/Symfony/Component/Workflow/Tests/TransitionTest.php index fbf9b38b23bb6..f79ba81a48b26 100644 --- a/src/Symfony/Component/Workflow/Tests/TransitionTest.php +++ b/src/Symfony/Component/Workflow/Tests/TransitionTest.php @@ -7,7 +7,7 @@ class TransitionTest extends \PHPUnit_Framework_TestCase { /** - * @expectedException Symfony\Component\Workflow\Exception\InvalidArgumentException + * @expectedException \Symfony\Component\Workflow\Exception\InvalidArgumentException * @expectedExceptionMessage The transition "foo.bar" contains invalid characters. */ public function testValidateName() diff --git a/src/Symfony/Component/Workflow/Tests/WorkflowTest.php b/src/Symfony/Component/Workflow/Tests/WorkflowTest.php index 58e9ecc9ead15..16af12f86eda3 100644 --- a/src/Symfony/Component/Workflow/Tests/WorkflowTest.php +++ b/src/Symfony/Component/Workflow/Tests/WorkflowTest.php @@ -6,7 +6,6 @@ use Symfony\Component\Workflow\Definition; use Symfony\Component\Workflow\Event\GuardEvent; use Symfony\Component\Workflow\Marking; -use Symfony\Component\Workflow\MarkingStore\MarkingStoreInterface; use Symfony\Component\Workflow\MarkingStore\PropertyAccessorMarkingStore; use Symfony\Component\Workflow\MarkingStore\ScalarMarkingStore; use Symfony\Component\Workflow\Transition; @@ -15,7 +14,7 @@ class WorkflowTest extends \PHPUnit_Framework_TestCase { /** - * @expectedException Symfony\Component\Workflow\Exception\LogicException + * @expectedException \Symfony\Component\Workflow\Exception\LogicException * @expectedExceptionMessage The marking store (Symfony\Component\Workflow\MarkingStore\ScalarMarkingStore) of workflow "unnamed" can not store many places. But the transition "t1" has too many output (2). Only one is accepted. */ public function testConstructorWithUniqueTransitionOutputInterfaceAndComplexWorkflow() @@ -35,20 +34,7 @@ public function testConstructorWithUniqueTransitionOutputInterfaceAndSimpleWorkf } /** - * @expectedException Symfony\Component\Workflow\Exception\LogicException - * @expectedExceptionMessage The value returned by the MarkingStore is not an instance of "Symfony\Component\Workflow\Marking" for workflow "unnamed". - */ - public function testGetMarkingWithInvalidStoreReturn() - { - $subject = new \stdClass(); - $subject->marking = null; - $workflow = new Workflow(new Definition(), $this->getMock(MarkingStoreInterface::class)); - - $workflow->getMarking($subject); - } - - /** - * @expectedException Symfony\Component\Workflow\Exception\LogicException + * @expectedException \Symfony\Component\Workflow\Exception\LogicException * @expectedExceptionMessage The Marking is empty and there is no initial place for workflow "unnamed". */ public function testGetMarkingWithEmptyDefinition() @@ -61,7 +47,7 @@ public function testGetMarkingWithEmptyDefinition() } /** - * @expectedException Symfony\Component\Workflow\Exception\LogicException + * @expectedException \Symfony\Component\Workflow\Exception\LogicException * @expectedExceptionMessage Place "nope" is not valid for workflow "unnamed". */ public function testGetMarkingWithImpossiblePlace() @@ -104,7 +90,7 @@ public function testGetMarkingWithExistingMarking() } /** - * @expectedException Symfony\Component\Workflow\Exception\LogicException + * @expectedException \Symfony\Component\Workflow\Exception\LogicException * @expectedExceptionMessage Transition "foobar" does not exist for workflow "unnamed". */ public function testCanWithUnexistingTransition() @@ -141,7 +127,7 @@ public function testCanWithGuard() } /** - * @expectedException Symfony\Component\Workflow\Exception\LogicException + * @expectedException \Symfony\Component\Workflow\Exception\LogicException * @expectedExceptionMessage Unable to apply transition "t2" for workflow "unnamed". */ public function testApplyWithImpossibleTransition() From 8d9bc359e1ccba022cacf7f4e929391e489f5e48 Mon Sep 17 00:00:00 2001 From: Roland Franssen Date: Tue, 28 Jun 2016 16:32:36 +0000 Subject: [PATCH 4/5] opinionated --- src/Symfony/Component/Workflow/Definition.php | 2 +- .../Component/Workflow/Tests/WorkflowTest.php | 13 +++++++++++++ src/Symfony/Component/Workflow/Workflow.php | 4 ++++ 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/Symfony/Component/Workflow/Definition.php b/src/Symfony/Component/Workflow/Definition.php index ec4234e4f87f5..11c04ef52458c 100644 --- a/src/Symfony/Component/Workflow/Definition.php +++ b/src/Symfony/Component/Workflow/Definition.php @@ -66,7 +66,7 @@ public function addPlace($place) throw new InvalidArgumentException(sprintf('The place "%s" contains invalid characters.', $place)); } - if (!$this->places) { + if (!count($this->places)) { $this->initialPlace = $place; } diff --git a/src/Symfony/Component/Workflow/Tests/WorkflowTest.php b/src/Symfony/Component/Workflow/Tests/WorkflowTest.php index 16af12f86eda3..8d340fda933e9 100644 --- a/src/Symfony/Component/Workflow/Tests/WorkflowTest.php +++ b/src/Symfony/Component/Workflow/Tests/WorkflowTest.php @@ -33,6 +33,19 @@ public function testConstructorWithUniqueTransitionOutputInterfaceAndSimpleWorkf new Workflow($definition, new ScalarMarkingStore()); } + /** + * @expectedException \Symfony\Component\Workflow\Exception\LogicException + * @expectedExceptionMessage The value returned by the MarkingStore is not an instance of "Symfony\Component\Workflow\Marking" for workflow "unnamed". + */ + public function testGetMarkingWithInvalidStoreReturn() + { + $subject = new \stdClass(); + $subject->marking = null; + $workflow = new Workflow(new Definition(), $this->getMock(MarkingStoreInterface::class)); + + $workflow->getMarking($subject); + } + /** * @expectedException \Symfony\Component\Workflow\Exception\LogicException * @expectedExceptionMessage The Marking is empty and there is no initial place for workflow "unnamed". diff --git a/src/Symfony/Component/Workflow/Workflow.php b/src/Symfony/Component/Workflow/Workflow.php index 438d5b3b67e27..641fc9e8d80a0 100644 --- a/src/Symfony/Component/Workflow/Workflow.php +++ b/src/Symfony/Component/Workflow/Workflow.php @@ -59,6 +59,10 @@ public function getMarking($subject) { $marking = $this->markingStore->getMarking($subject); + if (!$marking instanceof Marking) { + throw new LogicException(sprintf('The value returned by the MarkingStore is not an instance of "%s" for workflow "%s".', Marking::class, $this->name)); + } + // check if the subject is already in the workflow if (!$marking->getPlaces()) { if (!$this->definition->getInitialPlace()) { From a741518d505702623d47b150af6f413c70a22470 Mon Sep 17 00:00:00 2001 From: Roland Franssen Date: Tue, 28 Jun 2016 20:16:55 +0000 Subject: [PATCH 5/5] whoopsie daisy --- src/Symfony/Component/Workflow/Tests/WorkflowTest.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Symfony/Component/Workflow/Tests/WorkflowTest.php b/src/Symfony/Component/Workflow/Tests/WorkflowTest.php index 8d340fda933e9..22dc1927b8f28 100644 --- a/src/Symfony/Component/Workflow/Tests/WorkflowTest.php +++ b/src/Symfony/Component/Workflow/Tests/WorkflowTest.php @@ -6,6 +6,7 @@ use Symfony\Component\Workflow\Definition; use Symfony\Component\Workflow\Event\GuardEvent; use Symfony\Component\Workflow\Marking; +use Symfony\Component\Workflow\MarkingStore\MarkingStoreInterface; use Symfony\Component\Workflow\MarkingStore\PropertyAccessorMarkingStore; use Symfony\Component\Workflow\MarkingStore\ScalarMarkingStore; use Symfony\Component\Workflow\Transition;