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] Don't use VariableNode for ArrayNode#39328
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
| ->end() | ||
| ->variableNode('events_to_dispatch') | ||
| ->defaultValue(null) | ||
| ->arrayNode('events_to_dispatch') |
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.
anarrayNode must use either->children orprototype for it to work fine (otherwise, it will be an array node usingchildren() but with no allowed children, which is useless)
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.
Btw, once you change that to a proper prototype, thevalidate function can then be moved to the prototype instead of looping over the array, which would report the invalid value to a more precise location
stof commentedDec 5, 2020
@Nyholm your implementation forbids setting |
lyrixx commentedDec 5, 2020 • 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.
Hello, We wanted something a bit complexe here. We want to be able to
This is why we ended with the current code. IIRC, we have tests for this. So If you achieve to make the code simpler, without having to edit the fixtures, it will be nice :) When I finished#37815, I search for simpler code (may be not enought) but I can not find a better way. |
Nyholm commentedDec 5, 2020
I see. Thank you for the context and the review. I was dumping configuration reference and saw that the |
Nyholm commentedDec 5, 2020
Im closing this in favor of#39336 |
…r VariableNode with array example (Nyholm)This PR was merged into the 4.4 branch.Discussion----------[Config] YamlReferenceDumper: No default value required for VariableNode with array example| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | yes| New feature? | no| Deprecations? || Tickets || License | MIT| Doc PR |This willfix#39328 in a better way.A `VariableNode` may have an array as value and also a default value as "null". (Like the workflow component).When we dump the config, we generate invalid yaml:``` workflows: enabled: false workflows: # Prototype name: # Select which Transition events should be dispatched for this Workflow events_to_dispatch: null # Examples: - workflow.enter - workflow.transition```With this PR, we will remove the `null` default value from the dumped config.#SymfonyHackdayCommits-------9104fd4 [Config] YamlReferenceDumper: No default value required for VariableNode with array example
Uh oh!
There was an error while loading.Please reload this page.
This change could break some applications that have defined:
They should just remove the
events_to_dispatchline or define it as an empty array.I don't see why it was defined as a VariableNode in the first place. It was introduced here#37815, maybe@lyrixx can help me here?
#SymfonyHackday