Add Yaml::PARSE_EXCEPTION_ON_DUPLICATE to throw exceptions on duplicates #19529
Conversation
alexpott
commented
Aug 4, 2016
•
| 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 |
|
I'd love this feature. In a few big applications with 25+ developers, mistakes like this happen every once in a while. I love to see them crash instead of silently continuing. |
|
As proposed by other people, i would prefer deprecate this and throw an exception in symfony 4.0. |
|
@alexpott OK to replace this flag by a deprecation? |
| @@ -475,6 +475,8 @@ private static function parseMapping($mapping, $flags, &$i = 0, $references = ar | |||
| // are processed sequentially. | |||
| if (!isset($output[$key])) { | |||
| $output[$key] = $value; | |||
| } else { | |||
| @trigger_error(sprintf('Duplicate key "%s" detected whilst parsing YAML. Silent handling of duplicates in YAML is deprecated since version 3.3 and will cause an exception in 4.0.', $key), E_USER_DEPRECATED); | |||
There was a problem hiding this comment.
Should be since 3.2, right ?
Also should be:
- and will cause an exception
+ and will throw an exceptionThere was a problem hiding this comment.
We should probably also indicates the thrown exception will be an instance of Symfony\Component\Yaml\Exception\ParseException
|
The failure in fabbot.io is on a line not changed by this patch - I guess src/Symfony/Component/Yaml/Tests/Fixtures/YtsSpecificationExamples.yml does not comply with the standard. |
|
👍 |
| @@ -771,7 +771,7 @@ public function testScalarInSequence() | |||
| * @see http://yaml.org/spec/1.2/spec.html#id2759572 | |||
| * @see http://yaml.org/spec/1.1/#id932806 | |||
| */ | |||
| public function testMappingDuplicateKeyBlock() | |||
| public function testLegacyMappingDuplicateKeyBlock() | |||
There was a problem hiding this comment.
missing @group legacy annotation (and no need to rename the test case then)
same below
|
Please also update the upgrade files for 3.2 and 4.0. |
| @@ -230,6 +230,10 @@ Yaml | |||
| * The `!!php/object` tag to indicate dumped PHP objects was removed in favor of | |||
| the `!php/object` tag. | |||
|
|
|||
| * Support for silently ignoring duplicate keys in YAML has been deprecated and | |||
There was a problem hiding this comment.
In 4.0 it will be longer deprecated.
|
Thank you @alexpott. |
…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
|
IIRC, the Yaml spec explicitly says that parsers should use the first key when finding duplicates (and we made a bugfix in the past to respect it instead of using the last one). So this deprecation is a violation of the spec |
|
@stof from what i see, the spec says that it should be considered as an error:
And:
|
|
Good day! |
|
@ivangretsky There is no flag controlling this. The parser just triggers a deprecation when detecting duplicated keys on 3.2 or higher and will throw an exception starting with Symfony 4.0. |