Skip to content

Conversation

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Sep 10, 2025

Q A
Branch? 7.4
Bug fix? no
New feature? yes
Deprecations? no
Issues -
License MIT

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):

        $rootNode
-           ->fixXmlConfig('role', 'role_hierarchy')
            ->children()
-               ->arrayNode('role_hierarchy')
+               ->arrayNode('role_hierarchy', 'role')

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.

@carsonbot carsonbot added this to the 7.4 milestone Sep 10, 2025
@OskarStark OskarStark changed the title [Config] Add argument $singular to NodeBuilder::arrayNode() to decouple plurals/singulars from XML [Config] Add argument $singular to NodeBuilder::arrayNode() to decouple plurals/singulars from XML Sep 10, 2025
// get the parent array node builder ("firewalls") from inside the children builder
$factoryRootNode = $builder->end()->end();
$factoryRootNode
->fixXmlConfig('custom_authenticator')
Copy link
Member

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

Copy link
Member Author

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 😆

@stof
Copy link
Member

stof commented Sep 10, 2025

I'm not deprecating the fixXmlConfig() - it would be way too early.

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)

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Sep 10, 2025

Comments addressed @stof
I had to add support for defaultNull on ArrayNode (we used VariableNode as a hack around this limitation for now)

@nicolas-grekas nicolas-grekas force-pushed the config-plurals branch 2 times, most recently from 1d3cf25 to 874f794 Compare September 10, 2025 14:27
@nicolas-grekas
Copy link
Member Author

FTR, I tried this idea:

When defining a config tree, any prototyped array node should have a corresponding fixXmlConfig on the parent node. And reciprocally, the plural form given to fixXmlConfig should have a corresponding prototyped array in the children.

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:

  • We do have nodes that are defined as prototypes but are only accepted as attributes (thus strings) in the XML This works thanks to a beforeNormalization hook - an example of that is the framework.trusted_hosts setting.
  • And we do have array nodes with non-prototype children that do have plural/singular mappings - an example is the framework.workflows tree.

Which means this PR is our current best hope to improve the situation and I don't have any follow up in mind.

@OskarStark

This comment was marked as outdated.

@fabpot
Copy link
Member

fabpot commented Sep 12, 2025

Thank you @nicolas-grekas.

@fabpot fabpot merged commit a6f969d into symfony:7.4 Sep 12, 2025
8 of 12 checks passed
@nicolas-grekas nicolas-grekas deleted the config-plurals branch September 12, 2025 15:09
nicolas-grekas added a commit that referenced this pull request Sep 22, 2025
… `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
symfony-splitter pushed a commit to symfony/config that referenced this pull request Sep 22, 2025
… `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
@fabpot fabpot mentioned this pull request Oct 27, 2025
@fabpot fabpot mentioned this pull request Oct 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants