Skip to content

Duplicate keys in Yaml fail silently #19526

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

Closed
alexpott opened this issue Aug 4, 2016 · 9 comments
Closed

Duplicate keys in Yaml fail silently #19526

alexpott opened this issue Aug 4, 2016 · 9 comments

Comments

@alexpott
Copy link
Contributor

alexpott commented Aug 4, 2016

When the yaml parser reads

foo: bar
foo: bar2

It will return an array with the first key. This is unfortunate for a couple of reasons.

  1. It is not to spec. The Yaml spec is clear that duplicate keys should be treated as an error - see http://yaml.org/spec/1.2/spec.html#id2759572
  2. The current behaviour of choosing the first key is pretty unique. See:
Psy Shell v0.7.0 (PHP 7.0.7 — cli) by Justin Hileman
>>> $a = ['a' => '1', 'a' => '2']
=> [
     "a" => "2",
   ]

Also it is the opposite behaviour to the PECL yaml parser (which behaves like PHP) so allowing users to swap between implementations can have interesting affects.

@alexpott
Copy link
Contributor Author

alexpott commented Aug 4, 2016

So reading the links from the Symfony Yaml component where there are tests for this behaviour it links to http://yaml.org/spec/1.1/#id932806

It is an error for two equal keys to appear in the same mapping node. In such a case the YAML processor may continue, ignoring the second key: value pair and issuing an appropriate warning.

So what I think we are missing is and issuing an appropriate warning

@javiereguiluz
Copy link
Member

javiereguiluz commented Aug 4, 2016

This is the PR that introduced this behavior: #10902 and in other issues we've been discussing about this behavior too: #14512.

@xabbuh is the main Yaml component maintainer, so let's see what he thinks about introducing changes related to this. Thanks!

@sstok
Copy link
Contributor

sstok commented Aug 4, 2016

Would it be possible to use a silenced warning or a flag to throw/warn on a duplicate key?

@alexpott
Copy link
Contributor Author

alexpott commented Aug 4, 2016

@sstok I was just coding up a PR to introduce a new parse flag PARSE_EXCEPTION_ON_DUPLICATE

@xabbuh
Copy link
Member

xabbuh commented Aug 5, 2016

I guess that the current behaviour was implemented based on the assumption that duplicate keys are allowed (see the quoted part of the spec at #19526 (comment)). However, as you can this part is from the YAML 1.1 spec, but we do follow the YAML 1.2 which does forbid duplicate keys (see the issue description).

With this in mind I suggest to deprecate the current behaviour in Symfony 3.2 and throw a ParsException starting with Symfony 4.0.

@symfony/deciders What are your thoughts on this?

@javiereguiluz
Copy link
Member

@xabbuh I agree: deprecate in 3.2; exception in 4.0.

@dunglas
Copy link
Member

dunglas commented Aug 6, 2016

I agree too. Duplicate keys can be a pain to debug anyway.

@patrick-mcdougle
Copy link
Contributor

Does this effect the ability to include other config files? Like how the config_prod.yml file loads the config.yml file? Or is that a different concept that does that?

@xabbuh
Copy link
Member

xabbuh commented Aug 7, 2016

@patrick-mcdougle That's a different concept which is specific to how the DependencyInjection and Routing component handle the merging of configuration files that have been parsed by the parser from the Yaml component.

@fabpot fabpot closed this as completed Aug 9, 2016
fabpot added a commit that referenced this issue Aug 9, 2016
…ions on duplicates (Alex Pott)

This PR was squashed before being merged into the 3.2-dev branch (closes #19529).

Discussion
----------

Add Yaml::PARSE_EXCEPTION_ON_DUPLICATE to throw exceptions on duplicates

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #19526
| License       | MIT
| Doc PR        |

Commits
-------

cb362f2 Add Yaml::PARSE_EXCEPTION_ON_DUPLICATE to throw exceptions on duplicates
symfony-splitter pushed a commit to symfony/yaml that referenced this issue Aug 9, 2016
…ions on duplicates (Alex Pott)

This PR was squashed before being merged into the 3.2-dev branch (closes #19529).

Discussion
----------

Add Yaml::PARSE_EXCEPTION_ON_DUPLICATE to throw exceptions on duplicates

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony/symfony#19526
| License       | MIT
| Doc PR        |

Commits
-------

cb362f2 Add Yaml::PARSE_EXCEPTION_ON_DUPLICATE to throw exceptions on duplicates
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

7 participants