Added XML support for Workflow configuration #20458
Conversation
wouterj
commented
Nov 9, 2016
wouterj
commented
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | - |
| License | MIT |
| Doc PR | symfony/symfony-docs#6871 |
|
@wouterj I never used the XML config, but previous tests were OK. So I'm not 100% sure about this PR. Did you test it ? Should we add more tests ? |
|
@lyrixx I couldn't find any tests for workflow config (did a very quick search though). Prototyped arrays are represented by multiple occurences of the same element in XML. For example: workflows:
blogpost: ...
pull_request: ...<workflow name="blogpost">...</workflow>
<workflow name="pull_request">...</workflow>As you can see, this difference in format means the Config processor has to be aware of singular and plural versions of all prototyped config names (workflows vs workflow). This is done by adding these |
|
@wouterj Could you rebase please? |
|
👍 after rebasing |
|
I rebased this PR and also updated the XML schema definition. The changes made by @wouterj were technically not needed with the definition before, but having it this way is more inline with how we define XML configs elsewhere. ping @symfony/deciders |
|
|
| @@ -235,12 +235,16 @@ private function addWorkflowSection(ArrayNodeDefinition $rootNode) | |||
| ->arrayNode('workflows') | |||
There was a problem hiding this comment.
missing ->fixXmlConfig('workflow') on the root node IMO, as having a workflows node for the list of workflows does not match the way things are done (and btw, it will cause issues when you define 1 vs several workflows in XML currently AFAIK. Please test this case)
There was a problem hiding this comment.
The issue is the xsd:all node which we use as the top-level node of the config node. Afaik this doesn't allow us to specifiy several workflow nodes. That's why the workflow nodes are wrapped inside the workflows node and calling fixXmlConfig() isn't necessary for the workflow config node.
There was a problem hiding this comment.
then please write a test which defined multiple workflows in the same file too. I'm quite sure that the current setup is broken (it works by chance here when there is a single workflow, due to the way XML gets parsed, which is different when there is a single child with a given tag vs multiple childs, which is precisely why we fix the XML config for prototyped nodes)
There was a problem hiding this comment.
You're right. Tests added and XSD updated accordingly.
| </xsd:sequence> | ||
| <xsd:attribute name="type" type="xsd:string" use="optional" /> | ||
| <xsd:attribute name="service" type="xsd:string" use="optional" /> |
There was a problem hiding this comment.
use="optional" is the default in XSD, and we always omit it.
There was a problem hiding this comment.
You're right. I reverted this change and made the name attributes of the workflow and transition required instead.
xabbuh
force-pushed
the
patch-12
branch
2 times, most recently
from
1a61ab0
to
4dd1622
Compare
| <framework:place>last</framework:place> | ||
| <framework:transition name="foo"> | ||
| <framework:from>a</framework:from> | ||
| <framework:to>a</framework:to> |
There was a problem hiding this comment.
As you have 2 workflows, I would profit from it to cover the case of multiple from/to and multiple transitions too
|
@lyrixx the DI config does not seem to allow configuring the initial place, according to the XSD schema. Is it expected ? |
|
I want to merge #20490 first, then could you add the new initial_place, in the fixtures? |
…(HeahDude) This PR was merged into the 3.2-dev branch. Discussion ---------- [FrameworkBundle] [Workflow] fixed initial place config | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20458 (comment) | License | MIT | Doc PR | ~ Commits ------- 91237c9 [FrameworkBundle] [Workflow] Fixed initial place config
|
@lyrixx updated here |
|
👍 |
1 similar comment
|
👍 |
|
Thank you @wouterj. |
…abbuh) This PR was merged into the 3.2-dev branch. Discussion ---------- Added XML support for Workflow configuration | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | symfony/symfony-docs#6871 Commits ------- 94a7e7e [Workflow] streamline XML schema definition 6381caa Added XML support for Workflow configuration
