[Yaml] add tests for specific mapping keys #21650
Conversation
xabbuh
commented
Feb 17, 2017
| Q | A |
|---|---|
| Branch? | 2.7 |
| Bug fix? | no |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | see #21643 (comment) |
| License | MIT |
| Doc PR |
|
|
||
| public function testBooleanMappingKeysAreConvertedToStrings() | ||
| { | ||
| $this->assertSame(array('false' => 'foo'), Inline::parse('{false: foo}')); |
There was a problem hiding this comment.
That should not be allowed actually, keys should be treated like any other value and evaluated, so boolean should not be allowed as a key.
There was a problem hiding this comment.
Well, if we want to forbid using boolean as keys because they are not supported in PHP, this must be done in master with a deprecation.
Having these tests in our maintenance branches are good to avoid introducing BC breaks by mistake.
There was a problem hiding this comment.
btw, if we start evaluating, we would also have to forbid floats to avoid breaking BC.
There was a problem hiding this comment.
But we don't know if the user wants false to be dumped as a string or as the boolean value.
There was a problem hiding this comment.
As a bool key is supported neither in php nor in symfony, i think we can safelly guess the user wants a string.
There was a problem hiding this comment.
The point is this is broken if they use the same YAML file with another language that supports boolean keys as parsing and then dumping the parsed structure changes the result for other languages. Deprecating the current behaviour doesn't change anything about the inconsistency between the different languages, but it makes it obvious and prevents hard to discover bugs related to these inconsistencies.
There was a problem hiding this comment.
That's a good point too... Sounds complicated to fix after all, should we consider it as won't fix or would you prefer deprecating evaluated keys as I proposed at the beginning (note that this second solution may cause a small overhead)?
There was a problem hiding this comment.
I suggest that we deprecate parsing keys as strings implicitly and make this an opt-in feature instead (see #21774).
5a6f4e3 to
b8e0d70
Compare
|
Thank you @xabbuh. |
This PR was merged into the 2.7 branch. Discussion ---------- [Yaml] add tests for specific mapping keys | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | see #21643 (comment) | License | MIT | Doc PR | Commits ------- b8e0d70 [Yaml] add tests for specific mapping keys
…ys (xabbuh) This PR was merged into the 3.3-dev branch. Discussion ---------- [Yaml] deprecate implicit string casting of mapping keys | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #21650 (comment) | License | MIT | Doc PR | Commits ------- 3c5d915 deprecate implicit string casting of mapping keys