Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Config] Config builders should accept booleans for array nodes that can be enabled or disabled#61858
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
78db3dd tobf1f204Compare| $allowedTypes =$this->allowedTypes ??$this->normalization->declaredTypes; | ||
| foreach ([$this->trueEquivalent,$this->falseEquivalent]as$equivalent) { | ||
| if (\is_array($equivalent) &&$equivalent) { | ||
| $allowedTypes[] = ExprBuilder::TYPE_BOOL; |
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 is the fix
the rest is code simplifications
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.
👍🏽
| ->beforeNormalization() | ||
| ->ifArray() | ||
| ->then(staticfunction ($v) { | ||
| $v += ['enabled' =>true]; |
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.
we can rely on canBeEnabled/Disabled to set this default, but acceptAndWrap should then be moved earlier to have a proper ordering of beforeNormalization calls (adding [enabled => true] to wrapped strings)
…can be enabled or disabled
bf1f204 to633169fCompare| $allowedTypes =$this->allowedTypes ??$this->normalization->declaredTypes; | ||
| foreach ([$this->trueEquivalent,$this->falseEquivalent]as$equivalent) { | ||
| if (\is_array($equivalent) &&$equivalent) { | ||
| $allowedTypes[] = ExprBuilder::TYPE_BOOL; |
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 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.
Can we add new tests to validate this new behavior?
no need for new tests, the existing updated one covers this |
GromNaN left a comment• 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.
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.
Indeed, the generated code has changed and this change is tested. Thank you for clarifying.
Thank you@nicolas-grekas. |
37029cc intosymfony:7.4Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Continuing my journey on the config component, after#51273
That could be considered as a bugfix, but it's not worth fixing on lower branches because nobody is using this missing capability - by definition.
So: this allows doing eg
->lock(false)to disable locks, as expected when the node definition has->treatFalseLike(['enabled' => false])Note that I didn't turn
->treatNullLike(['enabled' => true])into acceptingnullas I don't see the point of allowing->lock(null)when one can use->lock(true).