[Workflow] Introduce concept of SupportStrategyInterface #21334
Conversation
lyrixx
commented
Jan 18, 2017
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | yes |
| Tests pass? | yes |
| Fixed tickets | #20751 |
| License | MIT |
| Doc PR | - |
…r support checks than class instance
d377344 to
710c29e
Compare
| @@ -293,7 +292,12 @@ private function addWorkflowSection(ArrayNodeDefinition $rootNode) | |||
| ->end() | |||
| ->end() | |||
| ->end() | |||
| ->scalarNode('initial_place')->defaultNull()->end() | |||
| ->scalarNode('support_strategy') | |||
| ->cannotBeEmpty() | |||
There was a problem hiding this comment.
This looks wrong to me in case you are using the supports option.
There was a problem hiding this comment.
1/ cannotBeEmpty != required
2/ I have added tests on it ;)
There was a problem hiding this comment.
@lyrixx this forbids resetting it to null in a subsequent config file though
There was a problem hiding this comment.
I saw your comment in the previous PR. But I don't see a valid use case here.
710c29e to
3c0e49f
Compare
| @@ -232,7 +232,8 @@ | |||
| <xsd:complexType name="workflow"> | |||
| <xsd:sequence> | |||
| <xsd:element name="marking-store" type="marking_store" /> | |||
| <xsd:element name="support" type="xsd:string" minOccurs="1" maxOccurs="unbounded" /> | |||
| <xsd:element name="support" type="xsd:string" minOccurs="0" maxOccurs="unbounded" /> | |||
| <xsd:element name="support-strategy" type="xsd:string" minOccurs="0" maxOccurs="1" /> | |||
There was a problem hiding this comment.
this should be an attribute, not a child element, as having several ones does not make any sense
There was a problem hiding this comment.
minOccurs="0" maxOccurs="1" it's already bounded. so it's not possible to have many element.
I did not set an attribute to be consistent with support.
There was a problem hiding this comment.
but this is inconsistent with all configuration we have in Symfony. We use a child element only for complex values (array nodes in the Configuration class)
3c0e49f to
134a58b
Compare
|
👍 (for the initial part) |
|
👍 |
| ----- | ||
|
|
||
| * Deprecated class name support in `WorkflowRegistry::add()` as second parameter. | ||
| Wrap the class name in an instance of ClassInstanceSupportStrategy instead. |
There was a problem hiding this comment.
You should probably mention the new support_strategy option as well (not sure if it makes sense to do it here as it's related to the config in FrameworkBundle).
There was a problem hiding this comment.
Actually, this should be added to the docs instead.
|
Thank you @lyrixx. |
|
Thank you @andesk for the initial work on this. |
…ce (andesk, lyrixx) This PR was merged into the 3.3-dev branch. Discussion ---------- [Workflow] Introduce concept of SupportStrategyInterface | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #20751 | License | MIT | Doc PR | - Commits ------- 134a58b [Workflow] Fixed code and tests 1843012 [Workflow] Introduce concept of SupprtStrategyInterface to allow other support checks than class instance
|
👍 Thank you for taking over, @lyrixx :) |
This PR was merged into the 3.3-dev branch. Discussion ---------- [Workflow] sync the changelog | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20751, #21334, #21933, #21935, #21950 | License | MIT | Doc PR | Commits ------- 98a18ee [Workflow] sync the changelog