Skip to content

[Workflow] Clarify validator API + fixed unknown "scalar" marking store #20492

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from
Closed

[Workflow] Clarify validator API + fixed unknown "scalar" marking store #20492

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9ef67a5
revised
ro0NL Nov 11, 2016
bf8f6c9
implied return void
ro0NL Nov 12, 2016
cd5e1c7
different approach
ro0NL Nov 13, 2016
5129630
feedback
ro0NL Nov 13, 2016
d8564e3
feedback
ro0NL Nov 14, 2016
b9afe01
whitespace
ro0NL Nov 14, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 7 additions & 20 deletions src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/ValidateWorkflowsPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Exception\RuntimeException;
use Symfony\Component\Workflow\Validator\DefinitionValidatorInterface;
use Symfony\Component\Workflow\Validator\SinglePlaceWorkflowValidator;
use Symfony\Component\Workflow\Validator\StateMachineValidator;
use Symfony\Component\Workflow\Validator\WorkflowValidator;

Expand All @@ -24,11 +23,6 @@
*/
class ValidateWorkflowsPass implements CompilerPassInterface
{
/**
* @var DefinitionValidatorInterface[]
*/
private $validators = array();

public function process(ContainerBuilder $container)
{
$taggedServices = $container->findTaggedServiceIds('workflow.definition');
Expand All @@ -45,7 +39,7 @@ public function process(ContainerBuilder $container)
throw new RuntimeException(sprintf('The "marking_store" for the tag "workflow.definition" of service "%s" must be set.', $id));
}

$this->getValidator($tag)->validate($definition, $tag['name']);
$this->createValidator($tag)->validate($definition, $tag['name']);
}
}
}
Expand All @@ -55,23 +49,16 @@ public function process(ContainerBuilder $container)
*
* @return DefinitionValidatorInterface
*/
private function getValidator($tag)
private function createValidator($tag)
{
if ($tag['type'] === 'state_machine') {
$name = 'state_machine';
$class = StateMachineValidator::class;
} elseif ($tag['marking_store'] === 'scalar') {
$name = 'single_place';
$class = SinglePlaceWorkflowValidator::class;
} else {
$name = 'workflow';
$class = WorkflowValidator::class;
if ('state_machine' === $tag['type']) {
return new StateMachineValidator();
}

if (empty($this->validators[$name])) {
$this->validators[$name] = new $class();
if ('single_state' === $tag['marking_store']) {
return new WorkflowValidator(true);
}

return $this->validators[$name];
return new WorkflowValidator();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this is a kind of bug :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing from 08464c9.

Copy link
Contributor Author

@ro0NL ro0NL Nov 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated PR description title.

}
10 changes: 4 additions & 6 deletions ...ator/SinglePlaceWorkflowValidatorTest.php → ...Tests/Validator/WorkflowValidatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,11 @@
namespace Symfony\Component\Workflow\Tests\Validator;

use Symfony\Component\Workflow\Definition;
use Symfony\Component\Workflow\Marking;
use Symfony\Component\Workflow\Tests\WorkflowTest;
use Symfony\Component\Workflow\Transition;
use Symfony\Component\Workflow\Validator\SinglePlaceWorkflowValidator;
use Symfony\Component\Workflow\Workflow;
use Symfony\Component\Workflow\Validator\WorkflowValidator;

class SinglePlaceWorkflowValidatorTest extends WorkflowTest
class WorkflowValidatorTest extends WorkflowTest
{
/**
* @expectedException \Symfony\Component\Workflow\Exception\InvalidDefinitionException
Expand All @@ -19,7 +17,7 @@ public function testSinglePlaceWorkflowValidatorAndComplexWorkflow()
{
$definition = $this->createComplexWorkflow();

(new SinglePlaceWorkflowValidator())->validate($definition, 'foo');
(new WorkflowValidator(true))->validate($definition, 'foo');
}

public function testSinglePlaceWorkflowValidatorAndSimpleWorkflow()
Expand All @@ -28,6 +26,6 @@ public function testSinglePlaceWorkflowValidatorAndSimpleWorkflow()
$transition = new Transition('t1', 'a', 'b');
$definition = new Definition($places, array($transition));

(new SinglePlaceWorkflowValidator())->validate($definition, 'foo');
(new WorkflowValidator(true))->validate($definition, 'foo');
}
}
2 changes: 0 additions & 2 deletions src/Symfony/Component/Workflow/Validator/DefinitionValidatorInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ interface DefinitionValidatorInterface
* @param Definition $definition
* @param string $name
*
* @return bool
*
* @throws InvalidDefinitionException on invalid definition
*/
public function validate(Definition $definition, $name);
Expand Down
41 changes: 0 additions & 41 deletions src/Symfony/Component/Workflow/Validator/SinglePlaceWorkflowValidator.php
View file
Open in desktop

This file was deleted.

2 changes: 0 additions & 2 deletions src/Symfony/Component/Workflow/Validator/StateMachineValidator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,5 @@ public function validate(Definition $definition, $name)
}
$transitionFromNames[$from][$transition->getName()] = true;
}

return true;
}
}
25 changes: 25 additions & 0 deletions src/Symfony/Component/Workflow/Validator/WorkflowValidator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,38 @@
namespace Symfony\Component\Workflow\Validator;

use Symfony\Component\Workflow\Definition;
use Symfony\Component\Workflow\Exception\InvalidDefinitionException;

/**
* @author Tobias Nyholm <tobias.nyholm@gmail.com>
*/
class WorkflowValidator implements DefinitionValidatorInterface
{
private $singlePlace;

/**
* @param bool $singlePlace
*/
public function __construct($singlePlace = false)
{
$this->singlePlace = $singlePlace;
}

public function validate(Definition $definition, $name)
{
if ($this->singlePlace) {
foreach ($definition->getTransitions() as $transition) {
if (1 < count($transition->getTos())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If (count($transition->getTos()) > 1)

Copy link
Contributor Author

@ro0NL ro0NL Nov 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was, as is, accepted before (and is actually yoda style). Not sure we should change it now..

throw new InvalidDefinitionException(
sprintf(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be inlined

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.

'The marking store of workflow "%s" can not store many places. But the transition "%s" has too many output (%d). Only one is accepted.',
$name,
$transition->getName(),
count($transition->getTos())
)
);
}
}
}
}
}