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] Added new validator to make sure each place has unique translation names#21271
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 left a comment
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 commentedJan 13, 2017
Note: It should be merge in 3.2 and not master. |
| useSymfony\Component\DependencyInjection\Exception\RuntimeException; | ||
| useSymfony\Component\Workflow\Validator\DefinitionValidatorInterface; | ||
| useSymfony\Component\Workflow\Validator\StateMachineValidator; | ||
| useSymfony\Component\Workflow\Validator\UniqueTransitionNameValidator; |
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.
missing composer.json update I guess, for ^3.2.3
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.
There is no reference to symfony/workflow at all in the composer.json of the framework bundle. Should I still add one?
ro0NL commentedJan 14, 2017
What about moving it to |
Nyholm commentedJan 14, 2017
Thank you@ro0NL. You are correct. The StateMachineValidator has this test already but it is missing in the WorkflowValidator. I will merge this test to the Workflow validator. |
fabpot commentedJan 15, 2017
Looks like a new feature to me, not a bug fix. I think it should be merged in 3.3 instead. |
Nyholm commentedJan 16, 2017
This PR makes sure the Definition is not invalid. That is why I marked it as a "bugfix". But one could argue that all PRs to the Validator as bugfixes... Anyhow. Since this check exist for theStateMachineValidator, I've updated the PR to include this check in the Workflow validator only. |
nicolas-grekas commentedJan 16, 2017
Generally : any new visible interface => feature. |
lyrixx commentedJan 16, 2017
For me it's a bug fix too, because if the definition is not valid, the workflow will not work correctly. |
fabpot commentedJan 18, 2017
Thank you@Nyholm. |
… unique translation names (Nyholm)This PR was submitted for the master branch but it was merged into the 3.2 branch instead (closes#21271).Discussion----------[Workflow] Added new validator to make sure each place has unique translation names| Q | A| ------------- | ---| Branch? | 3.2| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR | n/aA definition where a place that has transitions with the same name is an invalid definition.```yamlinvalid1: places: ['a', 'b', 'c'] transitions: t1: from: a to: b t1: from: a to: cvalid1: places: ['a', 'b', 'c'] transitions: t1: from: a to: b t2: from: a to: cvalid2: places: ['a', 'b', 'c'] transitions: t1: from: a to: b t1: from: b to: cvalid3: places: ['a', 'b', 'c', 'd'] transitions: t1: from: ['a', 'b'] to: d t2: from: ['a', 'b'] to: c```FYI@lyrixxCommits-------eece8ad [Workflow] Added new validator to make sure each place has unique translation names
A definition where a place that has transitions with the same name is an invalid definition.
FYI@lyrixx