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] AddArrayNodeDefinition::acceptAndWrap() to list alternative types that should be accepted and wrapped in an array#51273
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
7b1952c toe6694e6Compare This comment was marked as outdated.
This comment was marked as outdated.
Uh oh!
There was an error while loading.Please reload this page.
e6694e6 to1b84880Compare| if (isset($this->normalization)) { | ||
| $node->setNormalizationClosures($this->normalization->before); | ||
| $node->setNormalizedTypes($this->normalization->declaredTypes); | ||
| $node->setNormalizedTypes($this->allowedTypes ??$this->normalization->declaredTypes); |
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.
Why is$this->normalization->declaredTypes filled normally ? and why do we need to add this new$this->allowedTypes property instead of using the existing feature (which gets ignored entirely) ?
nicolas-grekasSep 17, 2025 • 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.
I was wondering the same until I (re)discovered#44166, which is based on that: sniffing for allowed types in what I'd call an indirect approach. I'm here proposing a more explicit approach, which wins over the implicit one when used.
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.
overriding entirely the types inferred from the normalization looks wrong to me (unless we want to deprecate that behavior).
And the suggested API does not play well with nodes supporting multiple normalization rules.
nicolas-grekasSep 18, 2025 • 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.
It has to be this way, seeL1532 in theConfiguration class for an example: implicit types are loose and too many times we have to fallback tomixed (->always(),->ifTrue($fn), etc.). Being explicit need to override the implicit collection of possible types - as not all types managed by normalizers have to be accepted at the config builder level (because of the XML loader mainly).
Which means this has to be the way to go.
I renamed the new method->acceptAndWrap() instead of->castToArray(). This is both more explicit and more accurate.
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Config/Definition/Builder/ArrayNodeDefinition.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
1b84880 to83c0b30Compare83c0b30 to48a78edCompareArrayNodeDefinition::acceptAndWrap() to list alternative types that should be accepted and wrapped in an array37153cf to73b4c37CompareThanks for the review@GromNaN - PR updated. |
| ExprBuilder::TYPE_STRING =>is_string(...), | ||
| ExprBuilder::TYPE_BOOL =>is_bool(...), | ||
| ExprBuilder::TYPE_NULL =>is_null(...), | ||
| ExprBuilder::TYPE_BACKED_ENUM =>staticfn ($v) =>$vinstanceof \BackedEnum, |
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.
That's right, let's keep that for a future improvement.
Enable more specific typesRequiressymfony/symfony#51273
Thank you@nicolas-grekas. |
a30f387 intosymfony:7.4Uh oh!
There was an error while loading.Please reload this page.
… from Kernel for testing (Kocal)This PR was merged into the 2.x branch.Discussion---------- Remove explicit configuration `twig.exception_controller` from Kernel for testing| Q | A| -------------- | ---| Bug fix? | yes| New feature? | no <!-- please update src/**/CHANGELOG.md files -->| Deprecations? | no <!-- if yes, also update UPGRADE-*.md and src/**/CHANGELOG.md -->| Documentation? | no <!-- required for new features, or documentation updates -->| Issues | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->| License | MITRelated tosymfony/symfony#51273 (comment)Fixhttps://github.com/symfony/ux/actions/runs/17999302594/job/51204828176?pr=3102Commits-------f46109a Remove explicit configuration `twig.exception_controller` from Kernel for testing
…nodes that can be enabled or disabled (nicolas-grekas)This PR was merged into the 7.4 branch.Discussion----------[Config] Config builders should accept booleans for array nodes that can be enabled or disabled| Q | A| ------------- | ---| Branch? | 7.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Issues | -| License | MITContinuing my journey on the config component, after#51273That 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 accepting `null` as I don't see the point of allowing `->lock(null)` when one can use `->lock(true)`.Commits-------633169f [Config] Config builders should accept booleans for array nodes that can be enabled or disabled
Uh oh!
There was an error while loading.Please reload this page.
With#58450, we lost some type info when generating config builders, by preventing#44166 to work correctly.
This PR improves the description of config trees by adding an
ArrayNodeDefinition::acceptAndWrap()method that allows one to declare which alternative types should be accepted at array-node levels. This declaration then triggers the wrapping of such cases into arrays, and also hints the config builder generator so that it can generate more accurate types for configurator methods.