Skip to content
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

[Config] useAttributeAsKey with second parameter. #25359

Closed
matt-usurp opened this issue Dec 6, 2017 · 4 comments
Closed

[Config] useAttributeAsKey with second parameter. #25359

matt-usurp opened this issue Dec 6, 2017 · 4 comments

Comments

@matt-usurp
Copy link

Q A
Bug report? yes
Feature request? no
BC Break report? no/maybe
RFC? no
Symfony version 4.0.1

So this might be a bug or might result in a documentation update as I find the config component documentation a little miss-leading in this area.

https://symfony.com/doc/current/components/config/definition.html#array-nodes

Here there is mention of the useAttributeAsKey() method, so I check this out and in the code there is a documentation against the method itself stating that:

If you'd like "'id' => 'my_name'" to still be present in the resulting
array, then you can set the second argument of this method to false.

https://github.com/symfony/symfony/blob/4.0/src/Symfony/Component/Config/Definition/Builder/ArrayNodeDefinition.php#L229-L230

So I would expect that if I take the example from the above link and set false against the second parameter as mentioned in the method documentation:

$node
    ->fixXmlConfig('connection')
    ->children()
        ->arrayNode('connections')
            ->useAttributeAsKey('name', false)
            ->arrayPrototype()
                ->children()
                    ->scalarNode('table')->end()
                    ->scalarNode('user')->end()
                    ->scalarNode('password')->end()
                ->end()
            ->end()
        ->end()
    ->end()
;

I would expect the following output to include the name key:

Array(
    [sf_connection] => Array(
        [name] => 'sf_connection'
        [table] => 'symfony'
        [user] => 'root'
        [password] => null
    )
)

I could argue the key would be transformed to become a map but really all that is important here (and as far as the documentation states) is that the useAttributeAsKey('some_name', false) will make sure the some_name is present in the resulting array output.

Note that there is no test for this case so I think it might have never worked or the documentation was being a little too ambitious.

I checked through the history (be it very quick) and this method hasn't changed since 2.8 (and probably before that but I didn't check further back). Reference can be found here https://github.com/symfony/symfony/blob/2.8/src/Symfony/Component/Config/Definition/Builder/ArrayNodeDefinition.php#L183

Anyone know if this is intended or was removed?

@Simperfit
Copy link
Contributor

Could you add a failing test case so we can see the issue and maybe help you to fix it ?

@matt-usurp
Copy link
Author

So I looked into writing a test and potentially fixing this (if it were a bug). But actually while looking at the tests surrounding this method it is clear that what I expected this method to do and what is actually does is different.

I expected that when using useAttributeAsKey('name', false) and supply this:

some_array:
  my_key:
    table: symfony

I would expect the following output:

Array( // some_array
    [my_key] => Array(
        [name] => 'my_key'
        [table] => 'symfony'
    )
)

Note that the key that key was added to the body array even though I didn't manually supply it.

Anyway after looking in to it I am good with closing this. Although I think the above might be a good feature potentially the documentation around the method (especially on the site) should be clearer.

@carsonbot
Copy link

Hey, thanks for your report!
There has not been a lot of activity here for a while. Is this bug still relevant? Have you managed to find a workaround?

@xabbuh
Copy link
Member

xabbuh commented Dec 20, 2020

Let's close this old issue as nobody else seems to have the same need.

@xabbuh xabbuh closed this as completed Dec 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants