[Yaml] Recommend using quotes instead of PARSE_KEYS_AS_STRINGS #22948

GuilhemN · 2017-05-29T20:15:00Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#...
License	MIT
Doc PR

Sorry for opening this so lately... I just realized that we could get rid of Yaml::PARSE_KEYS_AS_STRINGS just by recommending using quotes...

~~This way we don't allow a behavior not respecting the spec and we won't need to deprecate PARSE_KEYS_AS_STRINGS later.~~

~~Is it too late for this to be merged in 3.3?~~

ping @xabbuh

GuilhemN · 2017-05-29T20:18:26Z

src/Symfony/Component/Yaml/Inline.php

@@ -490,11 +490,10 @@ private static function parseMapping($mapping, $flags, &$i = 0, $references = ar
                @trigger_error('Omitting the key of a mapping is deprecated and will throw a ParseException in 4.0.', E_USER_DEPRECATED);
            }

-            if (!(Yaml::PARSE_KEYS_AS_STRINGS & $flags)) {
+            if (!$isKeyQuoted) {


Added because quoted keys should not be evaluated

GuilhemN · 2017-05-29T20:19:08Z

src/Symfony/Component/Yaml/Inline.php

-
-                if ('' !== $key && $evaluatedKey !== $key && !is_string($evaluatedKey)) {
-                    @trigger_error('Implicit casting of incompatible mapping keys to strings is deprecated since version 3.3 and will throw \Symfony\Component\Yaml\Exception\ParseException in 4.0. Pass the PARSE_KEYS_AS_STRING flag to explicitly enable the type casts.', E_USER_DEPRECATED);
+                if ('' !== $key && $evaluatedKey !== $key && !is_string($evaluatedKey) && !is_int($evaluatedKey)) {


I added the is_int check because unquoted integers work fine

xabbuh · 2017-05-30T06:36:36Z

I do not think this really is an option in general. The reason is that you may not be able to influence how the YAML file is actually dumped (e.g. an external data source) and not having quotes around mapping keys is totally valid.

GuilhemN · 2017-05-30T17:06:36Z

Using PARSE_KEYS_AS_STRINGS in our loaders mean that they don't evaluate keys and so do not comply with the spec.
At the same time, I don't think we should be able to parse a yaml file using features not supported by the language because it loses its meaning (if you use false as a key it means something, if you use "false" it means something else).

The reason is that you may not be able to influence how the YAML file is actually dumped

Let's be pragmatic, I don't think someone will ever use false, true, or whatever as a mapping key so imo we should be protected against these cases, because php doesn't support them so you can't reflect these values in the result you get from the parse.
The php extension always evaluate mapping keys but objects are serialized, false is converted to "", true to 1, null to "". I think this is wrong because most do not know this and you can't know what was the exact original value.

not having quotes around mapping keys is totally valid

It shouldn't be for values that are not supported by php imo because you end up with a weird behavior for basically no gain (who would voluntarily ever use these values as keys?).

GuilhemN · 2017-05-30T17:28:46Z

Updated to target 3.4

GuilhemN · 2017-06-07T10:52:54Z

Phpunit fails when using the 4.0 version of the Yaml component because of #22770, this exception must not be thrown when an integer is parsed (it is supported by php so there's no problem).

xabbuh · 2017-06-13T07:52:17Z

@GuilhemN That's fixed by #23108, right?

GuilhemN · 2017-06-13T08:21:52Z

@xabbuh indeed, pr rebased. The build still fails but this is not caused by this PR.

fabpot

@xabbuh Is this one ok for you?

stof · 2017-07-12T12:30:43Z

src/Symfony/Component/Yaml/Tests/ParserTest.php

@@ -1843,27 +1818,13 @@ public function testPhpConstantTagMappingKey()
        $this->assertSame($expected, $this->parser->parse($yaml, Yaml::PARSE_CONSTANT));
    }

-    public function testPhpConstantTagMappingKeyWithKeysCastToStrings()


IMO, this test should not disappear. We need to keep it to ensure this case keeps working in 3.4 (it should become a legacy test though)

I don't remember why i removed it so I guess we can revert this. However I won't be able to do it for the next 2 weeks, feel free to push on my repository if you have the time, otherwise I'll do the change ASAP.

stof · 2017-07-12T12:32:23Z

src/Symfony/Component/Yaml/Parser.php

@@ -236,9 +240,9 @@ private function doParse($value, $flags)
                    throw $e;
                }

-                if (!(Yaml::PARSE_KEYS_AS_STRINGS & $flags) && !is_string($key) && !is_int($key)) {
+                if (!is_string($key) && !is_int($key)) {


are you sure about this change ?

Yes, evaluable keys are no longer allowed unless they're quoted, even when using Yaml::PARSE_KEYS_AS_STRINGS.

stof · 2017-07-12T12:33:27Z

src/Symfony/Component/Yaml/Parser.php

                    $keyType = is_numeric($key) ? 'numeric key' : 'non-string key';
-                    @trigger_error(sprintf('Implicit casting of %s to string is deprecated since version 3.3 and will throw \Symfony\Component\Yaml\Exception\ParseException in 4.0. Pass the PARSE_KEYS_AS_STRINGS flag to explicitly enable the type casts.', $keyType), E_USER_DEPRECATED);
+                    @trigger_error(sprintf('Implicit casting of %s to string is deprecated since version 3.3 and will throw \Symfony\Component\Yaml\Exception\ParseException in 4.0. Quote your evaluable mapping keys instead.', $keyType), E_USER_DEPRECATED);


I suggest changing this message in 3.3 already. People who have not yet migrated to 3.3 should get the advice of quoting directly, to avoid a double migration for them. Quoting should already be working in 3.3.

I agree, I'll submit a pr if this is merged.

stof · 2017-07-12T12:34:44Z

src/Symfony/Component/Routing/Loader/YamlFileLoader.php

@@ -59,7 +59,7 @@ public function load($file, $type = null)
        }

        try {
-            $parsedConfig = $this->yamlParser->parse(file_get_contents($path), Yaml::PARSE_KEYS_AS_STRINGS);
+            $parsedConfig = $this->yamlParser->parse(file_get_contents($path));


isn't this a BC break ?

Not more than when we added it in 3.3 (for instance !str foo in routing files was parsed as "foo" in 3.2 and is as "!str foo" since 3.3). Reverting this change is in fact more intuitive imo as it does make the parser always spec compliant.

Shouldn't we do this change in #23635 then?

I prefer doing this in a minor but if you insist I can of course do it in 3.3.

The use statement can be removed too?

xabbuh · 2017-07-19T09:50:35Z

Recommending to quote keys indeed looks like a good idea. However, I am not convinced we should deprecate the PARSE_KEYS_AS_STRINGS. It can still be useful when you try to parse some YAML files that you do not control.

GuilhemN · 2017-07-19T10:04:13Z

@xabbuh imo it causes too many issues for what it's worth.
For instance !str true: foo should work fine but with the flag, you'll end up with a key you don't expect. We have the same problem with other syntax such as ! true and !str "foo".
Because of this you also won't be able to detect colliding keys (1 and 0x1 for example).

If the yaml file you try to parse uses features not supported by the sf parser nor by PHP, it looks normal and expectable to me that you can't parse this file.

Lastly, I don't think this fits something else than very edge cases that could probably also be fixed by altering the yaml file parsed.

xabbuh · 2017-07-24T06:37:54Z

src/Symfony/Component/Validator/Mapping/Loader/YamlFileLoader.php

@@ -14,7 +14,6 @@
 use Symfony\Component\Validator\Mapping\ClassMetadata;
 use Symfony\Component\Yaml\Exception\ParseException;
 use Symfony\Component\Yaml\Parser as YamlParser;
-use Symfony\Component\Yaml\Yaml;


this must be reverted

xabbuh · 2017-07-24T06:40:24Z

UPGRADE-3.4.md

+   After:
+
+   ```php
+


the blank line can be removed

xabbuh · 2017-07-24T06:41:00Z

UPGRADE-4.0.md

+   After:
+
+   ```php
+


can be removed

xabbuh · 2017-07-24T06:43:34Z

src/Symfony/Component/Routing/Loader/YamlFileLoader.php

@@ -59,7 +59,7 @@ public function load($file, $type = null)
        }

        try {
-            $parsedConfig = $this->yamlParser->parse(file_get_contents($path), Yaml::PARSE_KEYS_AS_STRINGS);
+            $parsedConfig = $this->yamlParser->parse(file_get_contents($path));


Shouldn't we do this change in #23635 then?

xabbuh · 2017-07-24T06:46:59Z

src/Symfony/Component/Yaml/Tests/InlineTest.php

-    /**
-     * @dataProvider getNotPhpCompatibleMappingKeyData
-     */
-    public function testExplicitStringCastingOfMappingKeys($yaml, $expected)


IMO the test should be kept but being flagged as legacy

xabbuh · 2017-07-24T06:47:07Z

src/Symfony/Component/Yaml/Tests/ParserTest.php

@@ -71,19 +71,6 @@ public function getDataFormSpecifications()
    }

    /**
-     * @dataProvider getNonStringMappingKeysData
-     */
-    public function testNonStringMappingKeys($expected, $yaml, $comment)


xabbuh · 2017-07-24T06:49:29Z

@GuilhemN I would still expect floats to be used more frequently as keys than tags, but if noone else objects, I am fine with the proposed changes. I just left some minor comments.

GuilhemN · 2017-07-24T08:04:01Z

@xabbuh maybe but people just have to quote the key if they want to use a float while we have no solution for tags. And still the same argument, the yaml file is invalid and could be rejected by another parser (at least the other parser won't produce the same result) :)

xabbuh · 2017-07-25T10:37:27Z

src/Symfony/Component/Serializer/Mapping/Loader/YamlFileLoader.php

@@ -114,7 +114,7 @@ private function getClassesFromYaml()
            $this->yamlParser = new Parser();
        }

-        $classes = $this->yamlParser->parse(file_get_contents($this->file), Yaml::PARSE_KEYS_AS_STRINGS);
+        $classes = $this->yamlParser->parse(file_get_contents($this->file));


Can the use statement be removed too?

xabbuh · 2017-07-26T07:21:44Z

@GuilhemN Can you rebase here? #23635 has been merged up.

GuilhemN · 2017-07-26T07:35:22Z

@xabbuh done

nicolas-grekas

shouldn't the @deprecated annotation be added to PARSE_KEYS_AS_STRINGS in Yaml.php?

GuilhemN · 2017-08-01T20:00:58Z

@nicolas-grekas indeed.

nicolas-grekas · 2017-08-01T20:27:35Z

src/Symfony/Component/Yaml/Yaml.php

@@ -31,6 +31,11 @@ class Yaml
    const PARSE_CONSTANT = 256;
    const PARSE_CUSTOM_TAGS = 512;
    const DUMP_EMPTY_ARRAY_AS_SEQUENCE = 1024;
+
+    /**
+     * @deprecated since version 3.4, to be removed in 4.0. Quote your evaluable


Should be on one line

xabbuh · 2017-08-03T09:28:39Z

Thank you @GuilhemN.

@xabbuh

…TRINGS (GuilhemN) This PR was squashed before being merged into the 3.4 branch (closes #22948). Discussion ---------- [Yaml] Recommend using quotes instead of PARSE_KEYS_AS_STRINGS | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no  | BC breaks? | no | Deprecations? | yes  | Tests pass? | yes | Fixed tickets | #...  | License | MIT | Doc PR | Sorry for opening this so lately... I just realized that we could get rid of `Yaml::PARSE_KEYS_AS_STRINGS` just by recommending using quotes... ~This way we don't allow a behavior not respecting the spec and we won't need to deprecate `PARSE_KEYS_AS_STRINGS` later.~ ~Is it too late for this to be merged in 3.3?~ ping @xabbuh Commits ------- b63c55c [Yaml] Recommend using quotes instead of PARSE_KEYS_AS_STRINGS

carsonbot added Status: Needs Review Yaml labels May 29, 2017

GuilhemN changed the base branch from master to 3.3 May 29, 2017 20:15

GuilhemN force-pushed the fix branch from 8629ea8 to ccf500c Compare May 29, 2017 20:15

GuilhemN commented May 29, 2017

View reviewed changes

GuilhemN force-pushed the fix branch from ccf500c to 04fef9e Compare May 29, 2017 20:21

nicolas-grekas added this to the 3.3 milestone May 30, 2017

GuilhemN changed the base branch from 3.3 to 3.4 May 30, 2017 17:07

GuilhemN force-pushed the fix branch 2 times, most recently from 307c083 to c5210ad Compare May 30, 2017 17:10

nicolas-grekas modified the milestones: 3.4, 3.3 Jun 2, 2017

GuilhemN force-pushed the fix branch from fd711b8 to 34c5fb2 Compare June 7, 2017 10:34

GuilhemN force-pushed the fix branch from 34c5fb2 to f89b090 Compare June 13, 2017 08:13

dunglas mentioned this pull request Jun 16, 2017

Fix/2.0 tests api-platform/core#1183

Merged

GuilhemN force-pushed the fix branch from f89b090 to 5d701f8 Compare June 25, 2017 06:20

fabpot approved these changes Jul 6, 2017

View reviewed changes

nicolas-grekas force-pushed the fix branch from 92f13c2 to c52afd6 Compare July 12, 2017 12:25

nicolas-grekas requested a review from xabbuh July 12, 2017 12:26

stof requested changes Jul 12, 2017

View reviewed changes

GuilhemN force-pushed the fix branch 2 times, most recently from 200c9b9 to 21ada49 Compare July 23, 2017 12:47

xabbuh requested changes Jul 24, 2017

View reviewed changes

xabbuh reviewed Jul 25, 2017

View reviewed changes

xabbuh mentioned this pull request Jul 26, 2017

[Yaml] ~ is not interpreted as null when used as array key #23664

Closed

[Yaml] Recommend using quotes instead of PARSE_KEYS_AS_STRINGS

4d9b2f9

GuilhemN force-pushed the fix branch from 441454a to 4d9b2f9 Compare July 26, 2017 07:34

xabbuh approved these changes Aug 1, 2017

View reviewed changes

nicolas-grekas reviewed Aug 1, 2017

View reviewed changes

fix

8189bba

nicolas-grekas reviewed Aug 1, 2017

View reviewed changes

fix

6e88335

xabbuh closed this Aug 3, 2017

GuilhemN deleted the fix branch November 1, 2020 14:58

[Yaml] Recommend using quotes instead of PARSE_KEYS_AS_STRINGS #22948

[Yaml] Recommend using quotes instead of PARSE_KEYS_AS_STRINGS #22948

Conversation

GuilhemN commented May 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh commented May 30, 2017

GuilhemN commented May 30, 2017

GuilhemN commented May 30, 2017

GuilhemN commented Jun 7, 2017 • edited Loading

xabbuh commented Jun 13, 2017

GuilhemN commented Jun 13, 2017

fabpot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh commented Jul 19, 2017

GuilhemN commented Jul 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh commented Jul 24, 2017

GuilhemN commented Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh commented Jul 26, 2017

GuilhemN commented Jul 26, 2017

nicolas-grekas left a comment

Choose a reason for hiding this comment

GuilhemN commented Aug 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh commented Aug 3, 2017

GuilhemN commented May 29, 2017 •

edited

Loading

GuilhemN commented Jun 7, 2017 •

edited

Loading

GuilhemN commented Jul 24, 2017 •

edited

Loading