[Workflow] improve workflow config validation #20482
Conversation
xabbuh
commented
Nov 10, 2016
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | n/a |
| License | MIT |
| Doc PR | n/a |
|
We should add some tests too. Status: Needs work |
c13df4d to
2133aba
Compare
|
Tests added Status: Needs review |
| ->thenInvalid('"type" and "service" cannot be used together.') | ||
| ->end() | ||
| ->validate() | ||
| ->ifTrue(function ($v) { return isset($v['arguments']) && isset($v['service']); }) |
There was a problem hiding this comment.
you should check whether arguments is an empty array or no actually, to account for people repeating the default value of the node explicitly (needed if they overwrite the definition in a subsequent file). An empty array should not trigger the exception.
There was a problem hiding this comment.
Good catch. However, what do you think about adding a constraint to the arguments node instead forbidding to configure it as an empty list?
There was a problem hiding this comment.
An empty array is a valid value (and it is the default value of the node btw, as it is a prototyped array node)
There was a problem hiding this comment.
I cannot confirm that. requiresAtLeastOneElement() works as expected when playing with the fixtures I added in this PR.
| </framework:marking-store> | ||
| <framework:supports>Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\FrameworkExtensionTest</framework:supports> | ||
| <framework:places>first</framework:places> | ||
| <framework:places>last</framework:places> |
There was a problem hiding this comment.
the XML should be improved. This should be place, not places.
The Configuration class actually misses the necessary fixXmlConfig calls for workflows, places and transitions.
There was a problem hiding this comment.
see #20458 which addresses most of the requested changes
| <framework:supports>Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\FrameworkExtensionTest</framework:supports> | ||
| <framework:places>first</framework:places> | ||
| <framework:places>last</framework:places> | ||
| <framework:transitions> |
There was a problem hiding this comment.
this node should not exist in the XML. It is not consistent with the way things work elsewhere.
| <framework:workflow name="my_workflow"> | ||
| <framework:marking-store> | ||
| <framework:type>multiple_state</framework:type> | ||
| <framework:service>workflow_service</framework:service> |
There was a problem hiding this comment.
these should be attributes, not subnodes
| http://symfony.com/schema/dic/symfony http://symfony.com/schema/dic/symfony/symfony-1.0.xsd"> | ||
|
|
||
| <framework:config> | ||
| <framework:workflows> |
|
If you are not comfortable with the XML config format, I can submit a separate PR instead to fix it. |
2133aba to
945f618
Compare
|
|
||
| return $v; | ||
| }) | ||
| ->ifTrue(function ($v) { return !isset($v['type']) && !isset($v['service']); }) |
There was a problem hiding this comment.
I my previous PR, I added that too. But I realized it's not mandatory. If nothing is configured, we don't give a MarkingStore to the workflow, and so the workflow will use the default MarkingStore. So it makes the configuration simpler.
There was a problem hiding this comment.
You are right. I reverted this change.
945f618 to
329fdb0
Compare
|
@xabbuh I have merged other PRs relative to the configuration. Could you rebase this one? Thanks ;) |
|
Indeed. I forget the other one. I prefer to wait another +1 there before the merge. |
329fdb0 to
3f94d16
Compare
|
I merged #20458 so please update your XML fixtures |
3f94d16 to
b1fb2a4
Compare
|
I rebased and updated the XML fixtures. |
|
👍 |
|
Thank you @xabbuh. |
This PR was merged into the 3.2-dev branch. Discussion ---------- [Workflow] improve workflow config validation | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Commits ------- b1fb2a4 [Workflow] improve workflow config validation