From a608797165a245bfcc67310b30065bbb5f8ffe4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Pineau?= Date: Tue, 4 Dec 2018 19:19:25 +0100 Subject: [PATCH] [Workflow] Move code from ValidateWorkflowsPass to the FrameworkExtension --- .../FrameworkExtension.php | 37 +++- .../FrameworkBundle/FrameworkBundle.php | 2 - .../Fixtures/php/workflow_not_valid.php | 30 +++ .../Fixtures/xml/workflow_not_valid.xml | 22 +++ .../Fixtures/xml/workflows.xml | 6 +- .../Fixtures/yml/workflow_not_valid.yml | 11 ++ .../FrameworkExtensionTest.php | 11 +- .../PhpFrameworkExtensionTest.php | 174 ++++++++++++++++++ .../ValidateWorkflowsPass.php | 2 + .../ValidateWorkflowsPassTest.php | 32 ---- 10 files changed, 281 insertions(+), 46 deletions(-) create mode 100644 src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/php/workflow_not_valid.php create mode 100644 src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/xml/workflow_not_valid.xml create mode 100644 src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/yml/workflow_not_valid.yml delete mode 100644 src/Symfony/Component/Workflow/Tests/DependencyInjection/ValidateWorkflowsPassTest.php diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php index a38518b2e9..96b779feaf 100644 --- a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php +++ b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php @@ -603,20 +603,15 @@ class FrameworkExtension extends Extension // Create places $places = array_column($workflow['places'], 'name'); + $initialPlace = $workflow['initial_place'] ?? null; // Create a Definition $definitionDefinition = new Definition(Workflow\Definition::class); $definitionDefinition->setPublic(false); $definitionDefinition->addArgument($places); $definitionDefinition->addArgument($transitions); - $definitionDefinition->addArgument($workflow['initial_place'] ?? null); + $definitionDefinition->addArgument($initialPlace); $definitionDefinition->addArgument($metadataStoreDefinition); - $definitionDefinition->addTag('workflow.definition', [ - 'name' => $name, - 'type' => $type, - 'marking_store' => $workflow['marking_store']['type'] ?? null, - 'single_state' => 'method' === ($workflow['marking_store']['type'] ?? null) && ($workflow['marking_store']['arguments'][0] ?? false), - ]); // Create MarkingStore if (isset($workflow['marking_store']['type'])) { @@ -641,6 +636,34 @@ class FrameworkExtension extends Extension $container->setDefinition(sprintf('%s.definition', $workflowId), $definitionDefinition); $container->registerAliasForArgument($workflowId, WorkflowInterface::class, $name.'.'.$type); + // Validate Workflow + $validator = null; + switch (true) { + case 'state_machine' === $workflow['type']: + $validator = new Workflow\Validator\StateMachineValidator(); + break; + case 'method' === ($workflow['marking_store']['type'] ?? null): + $singlePlace = $workflow['marking_store']['arguments'][0] ?? false; + $validator = new Workflow\Validator\WorkflowValidator($singlePlace); + break; + case 'single_state' === ($workflow['marking_store']['type'] ?? null): + $validator = new Workflow\Validator\WorkflowValidator(true); + break; + case 'multiple_state' === ($workflow['marking_store']['type'] ?? false): + $validator = new Workflow\Validator\WorkflowValidator(false); + break; + } + if ($validator) { + $realDefinition = (new Workflow\DefinitionBuilder($places)) + ->addTransitions(array_map(function (Reference $ref) use ($container): Workflow\Transition { + return $container->get((string) $ref); + }, $transitions)) + ->setInitialPlace($initialPlace) + ->build() + ; + $validator->validate($realDefinition, $name); + } + // Add workflow to Registry if ($workflow['supports']) { foreach ($workflow['supports'] as $supportedClassName) { diff --git a/src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php b/src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php index 7f86b9e4bb..78ddaeb3d8 100644 --- a/src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php +++ b/src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php @@ -54,7 +54,6 @@ use Symfony\Component\Translation\DependencyInjection\TranslatorPass; use Symfony\Component\Translation\DependencyInjection\TranslatorPathsPass; use Symfony\Component\Validator\DependencyInjection\AddConstraintValidatorsPass; use Symfony\Component\Validator\DependencyInjection\AddValidatorInitializersPass; -use Symfony\Component\Workflow\DependencyInjection\ValidateWorkflowsPass; /** * Bundle. @@ -115,7 +114,6 @@ class FrameworkBundle extends Bundle $container->addCompilerPass(new DataCollectorTranslatorPass()); $container->addCompilerPass(new ControllerArgumentValueResolverPass()); $container->addCompilerPass(new CachePoolPass(), PassConfig::TYPE_BEFORE_OPTIMIZATION, 32); - $this->addCompilerPassIfExists($container, ValidateWorkflowsPass::class); $container->addCompilerPass(new CachePoolClearerPass(), PassConfig::TYPE_AFTER_REMOVING); $container->addCompilerPass(new CachePoolPrunerPass(), PassConfig::TYPE_AFTER_REMOVING); $this->addCompilerPassIfExists($container, FormPass::class); diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/php/workflow_not_valid.php b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/php/workflow_not_valid.php new file mode 100644 index 0000000000..5d6ed54d67 --- /dev/null +++ b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/php/workflow_not_valid.php @@ -0,0 +1,30 @@ +loadFromExtension('framework', [ + 'workflows' => [ + 'my_workflow' => [ + 'type' => 'state_machine', + 'supports' => [ + FrameworkExtensionTest::class, + ], + 'places' => [ + 'first', + 'middle', + 'last', + ], + 'transitions' => [ + 'go' => [ + 'from' => [ + 'first', + ], + 'to' => [ + 'middle', + 'last', + ], + ], + ], + ], + ], +]); diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/xml/workflow_not_valid.xml b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/xml/workflow_not_valid.xml new file mode 100644 index 0000000000..df299c89de --- /dev/null +++ b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/xml/workflow_not_valid.xml @@ -0,0 +1,22 @@ + + + + + + + Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\FrameworkExtensionTest + + + + + first + middle + last + + + + diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/xml/workflows.xml b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/xml/workflows.xml index fe32756f2e..862ecae17c 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/xml/workflows.xml +++ b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/xml/workflows.xml @@ -29,12 +29,12 @@ approved_by_journalist - wait_for_spellcheker - approved_by_spellchker + wait_for_spellchecker + approved_by_spellchecker approved_by_journalist - approved_by_spellchker + approved_by_spellchecker published diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/yml/workflow_not_valid.yml b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/yml/workflow_not_valid.yml new file mode 100644 index 0000000000..123505de97 --- /dev/null +++ b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/yml/workflow_not_valid.yml @@ -0,0 +1,11 @@ +framework: + workflows: + my_workflow: + type: state_machine + supports: + - Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\FrameworkExtensionTest + places: [first, middle, last] + transitions: + go: + from: first + to: [last, middle ] diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php index b38439349a..1afe5f199c 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php +++ b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php @@ -215,7 +215,6 @@ abstract class FrameworkExtensionTest extends TestCase $workflowDefinition->getArgument(0), 'Places are passed to the workflow definition' ); - $this->assertSame(['workflow.definition' => [['name' => 'article', 'type' => 'workflow', 'marking_store' => 'multiple_state', 'single_state' => false]]], $workflowDefinition->getTags()); $this->assertCount(4, $workflowDefinition->getArgument(1)); $this->assertSame('draft', $workflowDefinition->getArgument(2)); @@ -237,7 +236,6 @@ abstract class FrameworkExtensionTest extends TestCase $stateMachineDefinition->getArgument(0), 'Places are passed to the state machine definition' ); - $this->assertSame(['workflow.definition' => [['name' => 'pull_request', 'type' => 'state_machine', 'marking_store' => 'single_state', 'single_state' => false]]], $stateMachineDefinition->getTags()); $this->assertCount(9, $stateMachineDefinition->getArgument(1)); $this->assertSame('start', $stateMachineDefinition->getArgument(2)); @@ -272,6 +270,15 @@ abstract class FrameworkExtensionTest extends TestCase $this->assertGreaterThan(0, \count($registryDefinition->getMethodCalls())); } + /** + * @expectedException \Symfony\Component\Workflow\Exception\InvalidDefinitionException + * @expectedExceptionMessage A transition from a place/state must have an unique name. Multiple transitions named "go" from place/state "first" where found on StateMachine "my_workflow". + */ + public function testWorkflowAreValidated() + { + $this->createContainerFromFile('workflow_not_valid'); + } + /** * @expectedException \Symfony\Component\Config\Definition\Exception\InvalidConfigurationException * @expectedExceptionMessage "type" and "service" cannot be used together. diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/PhpFrameworkExtensionTest.php b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/PhpFrameworkExtensionTest.php index ec39372b1d..19bad8e825 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/PhpFrameworkExtensionTest.php +++ b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/PhpFrameworkExtensionTest.php @@ -56,4 +56,178 @@ class PhpFrameworkExtensionTest extends FrameworkExtensionTest ]); }); } + + /** + * @expectedException \Symfony\Component\Workflow\Exception\InvalidDefinitionException + * @expectedExceptionMessage A transition from a place/state must have an unique name. Multiple transitions named "a_to_b" from place/state "a" where found on StateMachine "article". + */ + public function testWorkflowValidationStateMachine() + { + $this->createContainerFromClosure(function ($container) { + $container->loadFromExtension('framework', [ + 'workflows' => [ + 'article' => [ + 'type' => 'state_machine', + 'supports' => [ + __CLASS__, + ], + 'places' => [ + 'a', + 'b', + 'c', + ], + 'transitions' => [ + 'a_to_b' => [ + 'from' => ['a'], + 'to' => ['b', 'c'], + ], + ], + ], + ], + ]); + }); + } + + /** + * @expectedException \Symfony\Component\Workflow\Exception\InvalidDefinitionException + * @expectedExceptionMessage The marking store of workflow "article" can not store many places. But the transition "a_to_b" has too many output (2). Only one is accepted. + */ + public function testWorkflowValidationMethodSingle() + { + $this->createContainerFromClosure(function ($container) { + $container->loadFromExtension('framework', [ + 'workflows' => [ + 'article' => [ + 'type' => 'workflow', + 'marking_store' => [ + 'type' => 'method', + 'arguments' => [ + true, + ], + ], + 'supports' => [ + __CLASS__, + ], + 'places' => [ + 'a', + 'b', + 'c', + ], + 'transitions' => [ + 'a_to_b' => [ + 'from' => ['a'], + 'to' => ['b', 'c'], + ], + ], + ], + ], + ]); + }); + } + + public function testWorkflowValidationMethodNotSingle() + { + $this->createContainerFromClosure(function ($container) { + $container->loadFromExtension('framework', [ + 'workflows' => [ + 'article' => [ + 'type' => 'workflow', + 'marking_store' => [ + 'type' => 'method', + 'arguments' => [ + false, + ], + ], + 'supports' => [ + __CLASS__, + ], + 'places' => [ + 'a', + 'b', + 'c', + ], + 'transitions' => [ + 'a_to_b' => [ + 'from' => ['a'], + 'to' => ['b', 'c'], + ], + ], + ], + ], + ]); + }); + + // the test ensures that the validation does not fail (i.e. it does not throw any exceptions) + $this->addToAssertionCount(1); + } + + public function testWorkflowValidationMultipleState() + { + $this->createContainerFromClosure(function ($container) { + $container->loadFromExtension('framework', [ + 'workflows' => [ + 'article' => [ + 'type' => 'workflow', + 'marking_store' => [ + 'type' => 'multiple_state', + ], + 'supports' => [ + __CLASS__, + ], + 'places' => [ + 'a', + 'b', + 'c', + ], + 'transitions' => [ + 'a_to_b' => [ + 'from' => ['a'], + 'to' => ['b', 'c'], + ], + ], + ], + ], + ]); + }); + + // the test ensures that the validation does not fail (i.e. it does not throw any exceptions) + $this->addToAssertionCount(1); + } + + /** + * @expectedException \Symfony\Component\Workflow\Exception\InvalidDefinitionException + * @expectedExceptionMessage The marking store of workflow "article" can not store many places. But the transition "a_to_b" has too many output (2). Only one is accepted. + */ + public function testWorkflowValidationSingleState() + { + $this->createContainerFromClosure(function ($container) { + $container->loadFromExtension('framework', [ + 'workflows' => [ + 'article' => [ + 'type' => 'workflow', + 'marking_store' => [ + 'type' => 'single_state', + ], + 'supports' => [ + __CLASS__, + ], + 'places' => [ + 'a', + 'b', + 'c', + ], + 'transitions' => [ + 'a_to_b' => [ + 'from' => ['a'], + 'to' => ['b', 'c'], + ], + ], + ], + ], + ]); + }); + + // the test ensures that the validation does not fail (i.e. it does not throw any exceptions) + $this->addToAssertionCount(1); + } } diff --git a/src/Symfony/Component/Workflow/DependencyInjection/ValidateWorkflowsPass.php b/src/Symfony/Component/Workflow/DependencyInjection/ValidateWorkflowsPass.php index 9294a1a7fb..3ef4af2580 100644 --- a/src/Symfony/Component/Workflow/DependencyInjection/ValidateWorkflowsPass.php +++ b/src/Symfony/Component/Workflow/DependencyInjection/ValidateWorkflowsPass.php @@ -19,6 +19,8 @@ use Symfony\Component\Workflow\Validator\WorkflowValidator; /** * @author Tobias Nyholm + * + * @deprecated since Symfony 4.3 */ class ValidateWorkflowsPass implements CompilerPassInterface { diff --git a/src/Symfony/Component/Workflow/Tests/DependencyInjection/ValidateWorkflowsPassTest.php b/src/Symfony/Component/Workflow/Tests/DependencyInjection/ValidateWorkflowsPassTest.php deleted file mode 100644 index c9cb840249..0000000000 --- a/src/Symfony/Component/Workflow/Tests/DependencyInjection/ValidateWorkflowsPassTest.php +++ /dev/null @@ -1,32 +0,0 @@ -register('definition1', WorkflowDefinition::class) - ->addArgument(['a', 'b', 'c']) - ->addArgument([ - new Definition(Transition::class, ['t1', 'a', 'b']), - new Definition(Transition::class, ['t2', 'a', 'c']), - ]) - ->addTag('workflow.definition', ['name' => 'wf1', 'type' => 'state_machine', 'marking_store' => 'foo']); - - (new ValidateWorkflowsPass())->process($container); - - $workflowDefinition = $container->get('definition1'); - - $this->assertSame(['a' => 'a', 'b' => 'b', 'c' => 'c'], $workflowDefinition->getPlaces()); - $this->assertEquals([new Transition('t1', 'a', 'b'), new Transition('t2', 'a', 'c')], $workflowDefinition->getTransitions()); - } -}