Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Workflow] Introduce concept of SupportStrategyInterface#21334
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
lyrixx commentedJan 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 to710c29eCompare| ->end() | ||
| ->scalarNode('initial_place')->defaultNull()->end() | ||
| ->scalarNode('support_strategy') | ||
| ->cannotBeEmpty() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This looks wrong to me in case you are using thesupports option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
1/ cannotBeEmpty != required
2/ I have added tests on it ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oops, you're right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@lyrixx this forbids resetting it to null in a subsequent config file though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I saw your comment in the previous PR. But I don't see a valid use case here.
710c29e to3c0e49fCompare| <xsd:elementname="marking-store"type="marking_store" /> | ||
| <xsd:elementname="support"type="xsd:string"minOccurs="1"maxOccurs="unbounded" /> | ||
| <xsd:elementname="support"type="xsd:string"minOccurs="0"maxOccurs="unbounded" /> | ||
| <xsd:elementname="support-strategy"type="xsd:string"minOccurs="0"maxOccurs="1" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 withsupport.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
but this is inconsistent withall configuration we have in Symfony. We use a child element only for complex values (array nodes in the Configuration class)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ok, I will update it ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fixed.
3c0e49f to134a58bComparelyrixx commentedJan 18, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
👍 (for the initial part) |
stof commentedJan 18, 2017
👍 |
| ----- | ||
| * 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You should probably mention the newsupport_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actually, this should be added to the docs instead.
fabpot commentedJan 18, 2017
Thank you@lyrixx. |
fabpot commentedJan 18, 2017
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 tests1843012 [Workflow] Introduce concept of SupprtStrategyInterface to allow other support checks than class instance
andesk commentedJan 18, 2017
👍 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