[Workflow] Clarify validator API + fixed unknown "scalar" marking store #20492
Conversation
|
I agree that it is not really intuitive to throw an exception in case of an invalid definition, but return |
| @@ -23,7 +23,7 @@ | |||
| * @param Definition $definition | |||
| * @param string $name | |||
| * | |||
| * @return bool | |||
| * @return void | |||
|
So.. what about removing the Looks like a premature optimization to me.. and im not even sure validators should resemble a semi same inheritance tree. |
|
Back to 1 workflow validator and 1 state machine validator seems most intuitive to me =/ |
|
You should not validate every workflows as a single place workflow, this feels wrong to me. |
|
Not sure i understand.. right now it's optin. |
|
Ok haven't look at the code yet :p |
| @@ -59,19 +58,24 @@ private function getValidator($tag) | |||
| { | |||
| if ($tag['type'] === 'state_machine') { | |||
There was a problem hiding this comment.
Please use yoda condition
| @@ -59,19 +58,24 @@ private function getValidator($tag) | |||
| { | |||
| if ($tag['type'] === 'state_machine') { | |||
| $name = 'state_machine'; | |||
| $class = StateMachineValidator::class; | |||
| } elseif ($tag['marking_store'] === 'scalar') { | |||
There was a problem hiding this comment.
That would be a bug fix right?
There was a problem hiding this comment.
As you're already at bug fixing :)
| } elseif ($tag['marking_store'] === 'scalar') { | ||
| $name = 'single_place'; | ||
| $class = SinglePlaceWorkflowValidator::class; | ||
| $name = 'workflow_single_place'; |
There was a problem hiding this comment.
You should rename this workflow_single_state for consistency
| foreach ($definition->getTransitions() as $transition) { | ||
| if (1 < count($transition->getTos())) { | ||
| throw new InvalidDefinitionException( | ||
| sprintf( |
| public function validate(Definition $definition, $name) | ||
| { | ||
| if ($this->singlePlace) { | ||
| foreach ($definition->getTransitions() as $transition) { | ||
| if (1 < count($transition->getTos())) { |
There was a problem hiding this comment.
If (count($transition->getTos()) > 1)
There was a problem hiding this comment.
This was, as is, accepted before (and is actually yoda style). Not sure we should change it now..
| } | ||
|
|
||
| return $this->validators[$name]; | ||
| switch ($name) { |
There was a problem hiding this comment.
It's a bit cumbersome to have a if / else then the same switch
And IMHO, we could remove the cache layer to make code simpler. it's a premature optimization that does not bring real perf improvement .
|
👍 (I removed the Bug label, since it's not a bug. Could you edit your PR description?) |
| if ($tag['type'] === 'state_machine') { | ||
| $name = 'state_machine'; | ||
| $class = StateMachineValidator::class; | ||
| } elseif ($tag['marking_store'] === 'scalar') { |
There was a problem hiding this comment.
Actually this is a kind of bug :)
There was a problem hiding this comment.
Updated PR description title.
ro0NL
changed the title
|
Good catch, thanks @ro0NL. |
See also https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/ValidateWorkflowsPass.php#L48