-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[Config] Add argument $singular to NodeBuilder::arrayNode() to decouple plurals/singulars from XML
#61718
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
$singular to NodeBuilder::arrayNode() to decouple plurals/singulars from XML
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
Show resolved
Hide resolved
| // get the parent array node builder ("firewalls") from inside the children builder | ||
| $factoryRootNode = $builder->end()->end(); | ||
| $factoryRootNode | ||
| ->fixXmlConfig('custom_authenticator') |
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.
Deprecating fixXmlConfig would first require changing how custom authenticators are configured, by avoiding the hack of getting out of the builder in this factory
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 left as an exercise for the future 😆
I agree. Such deprecation should wait for a future 8.x release happening after the EOM of 6.4 LTS (to avoid issues for bundles wanting to keep support for the 6.4 LTS until its EOM, which is something we should not discourage as that's relevant for a maintained LTS) |
07f292f to
b4e67a9
Compare
|
Comments addressed @stof |
1d3cf25 to
874f794
Compare
|
FTR, I tried this idea:
This is the patch I wrote to do so.--- a/src/Symfony/Component/Config/Definition/Builder/ArrayNodeDefinition.php
+++ b/src/Symfony/Component/Config/Definition/Builder/ArrayNodeDefinition.php
@@ -491,6 +491,23 @@ class ArrayNodeDefinition extends NodeDefinition implements ParentNodeDefinition
if (false !== $this->addDefaultChildren) {
throw new InvalidDefinitionException(\sprintf('->addDefaultChildrenIfNoneSet() is not applicable to concrete nodes at path "%s".', $path));
}
+
+ $pluralFromRemap = isset($this->normalization) ? array_column($this->normalization->remappings, 1) : [];
+
+ foreach ($this->children as $name => $child) {
+ if ($child instanceof self && isset($child->prototype)) {
+ if (!\in_array($name, $pluralFromRemap, true)) {
+ trigger_deprecation('symfony/config', '7.4', 'Config node "%s" defines a prototyped child "%s" but lacks a matching singular remapping; use the second argument of arrayNode() to define one on that child.', $path, $name);
+ }
+ }
+ }
+
+ foreach ($pluralFromRemap as $plural) {
+ $child = $this->children[$plural] ?? null;
+ if (!$child instanceof self || !isset($child->prototype)) {
+ trigger_deprecation('symfony/config', '7.4', 'Config node "%s" defines a singular remapping for "%s" but has no corresponding prototyped array child.', $path, $plural);
+ }
+ }
}But this doesn't work in practice:
Which means this PR is our current best hope to improve the situation and I don't have any follow up in mind. |
…le plurals/singulars from XML
874f794 to
5ac6e74
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
Thank you @nicolas-grekas. |
… `null` (GromNaN) This PR was merged into the 7.4 branch. Discussion ---------- [Config] Fix Yaml dumper for prototyped array with default `null` | Q | A | ------------- | --- | Branch? | 7.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | - | License | MIT The command `config:dump-reference` fails with the [`workflow.events_to_dispatch` option](https://github.com/symfony/symfony/blob/91f8966d726c8d98ab6ea56c7e31fb933e9e9d28/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php#L458-L461). The default is `null`, which is not accepted by `count()`. *Bug introduced by #61718 (5ac6e74).* Commits ------- 54db22e [Config] Fix Yaml dumper for prototyped array with default null
… `null` (GromNaN) This PR was merged into the 7.4 branch. Discussion ---------- [Config] Fix Yaml dumper for prototyped array with default `null` | Q | A | ------------- | --- | Branch? | 7.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | - | License | MIT The command `config:dump-reference` fails with the [`workflow.events_to_dispatch` option](https://github.com/symfony/symfony/blob/91f8966d726c8d98ab6ea56c7e31fb933e9e9d28/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php#L458-L461). The default is `null`, which is not accepted by `count()`. *Bug introduced by symfony/symfony#61718 (5ac6e74).* Commits ------- 54db22effb3 [Config] Fix Yaml dumper for prototyped array with default null
The capability of defining plural/singular variants for config node is used not only for XML but also for config builders generation.
The current method to deal with this concern is named
fixXmlConfig(). It's confusing to say the least. What's also confusing is that the call needs to happen on the parent node.This PR proposes to add a second argument to the
arrayNode()method, so that things could be changed like this (example taken from the attached patch):I'm not deprecating the
fixXmlConfig()method - it would be way too early.In a follow up PR, I'll try to require setting this second argument (or calling fixXmlConfig) when a prototype is defined. If possible.