[Yaml] add tests for specific mapping keys #21650

Open
wants to merge 1 commit into
from

Projects

None yet

5 participants

@xabbuh
Member
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}'));
@stof
stof Feb 17, 2017 Member

I would test both true and false

@xabbuh
xabbuh Feb 17, 2017 Member

True, did that.

@GuilhemN
GuilhemN Feb 17, 2017 Contributor

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.

@stof
stof Feb 17, 2017 Member

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.

@stof
stof Feb 17, 2017 Member

btw, if we start evaluating, we would also have to forbid floats to avoid breaking BC.

@xabbuh
xabbuh Feb 17, 2017 Member

@GuilhemN Are you sure? I did not find anything in the spec explaining that the key should indeed become the boolean true/false.

@stof
stof Feb 17, 2017 Member

see http://www.yaml.org/spec/1.2/spec.html#id2802432:

YAML places no restrictions on the type of keys; in particular, they are not restricted to being scalars.

Keys can be absolutely anything in YAML, including another mapping.

@GuilhemN
GuilhemN Feb 17, 2017 Contributor

@stof sure but not in PHP: only scalars are supported as keys.

@xabbuh i can't search now, i'll tell you later.

@xabbuh
xabbuh Feb 17, 2017 Member

I missed that part. In this case, I would merge these tests into the 2.7 branch and deprecate the behaviour in master.

@stof
stof Feb 17, 2017 Member

@GuilhemN our current behavior is to avoid evaluating keys (as PHP does not support arbitrary keys). Changing this to throw exceptions for any non integer or string keys is a big BC break, and I'm not sure this one would actually benefit users that much.

@GuilhemN
GuilhemN Feb 17, 2017 Contributor

I missed that part. In this case, I would merge these tests into the 2.7 branch and deprecate the behaviour in master.

👍

Changing this to throw exceptions for any non integer or string keys is a big BC break, and I'm not sure this one would actually benefit users that much.

It is more for interop with other languages, in symfony false: will give a string but in other languages, it could be a real bool, especially when using the yaml encoder of the Serializer component as it can be a file you know nothing about.

@stof
stof Feb 17, 2017 Member

if you encode in PHP, you cannot have booleans as key.

This just means that our YAML dumper must take care of quoting special keys during dumping, to ensure that other languages also parse the key as a string.

@GuilhemN
GuilhemN Feb 17, 2017 Contributor

Well that works too :)

@stof
stof Feb 17, 2017 Member

Well, that's a requirement anyway, to actually dump what we wanted (assuming we support parsing quoted string keys, but I think we do)

@xabbuh
xabbuh Feb 17, 2017 Member

But we don't know if the user wants false to be dumped as a string or as the boolean value.

@GuilhemN
GuilhemN Feb 17, 2017 Contributor

As a bool key is supported neither in php nor in symfony, i think we can safelly guess the user wants a string.

@xabbuh
xabbuh Feb 17, 2017 Member

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.

@GuilhemN
GuilhemN Feb 17, 2017 Contributor

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

@xabbuh xabbuh [Yaml] add tests for specific mapping keys
b8e0d70
@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Feb 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment