NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Yaml] Recommend using quotes instead of PARSE_KEYS_AS_STRINGS#22948

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

GuilhemN wants to merge3 commits intosymfony:3.4fromGuilhemN:fix

Closed

[Yaml] Recommend using quotes instead of PARSE_KEYS_AS_STRINGS#22948

GuilhemN wants to merge3 commits intosymfony:3.4fromGuilhemN:fix

Conversation

Copy link

Contributor

GuilhemN commentedMay 29, 2017•
edited
Loading

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 ofYaml::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 deprecatePARSE_KEYS_AS_STRINGS later.~~

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

ping@xabbuh

carsonbot added Status: Needs Review Yaml labels

May 29, 2017

GuilhemN changed the base branch frommaster to3.3

May 29, 2017 20:15

GuilhemN force-pushed thefix branch from8629ea8 toccf500cCompare

May 29, 2017 20:15

GuilhemN commented

May 29, 2017

View reviewed changes

src/Symfony/Component/Yaml/Inline.php

		}

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

Copy link

ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Added because quoted keys should not be evaluated

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

Copy link

ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I added theis_int check because unquoted integers work fine

GuilhemN force-pushed thefix branch fromccf500c to04fef9eCompare

May 29, 2017 20:21

Copy link

Member

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.

nicolas-grekas added this to the3.3 milestone

May 30, 2017

Copy link

ContributorAuthor

GuilhemN commentedMay 30, 2017

UsingPARSE_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 usefalse,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 to1,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 changed the base branch from3.3 to3.4

May 30, 2017 17:07

GuilhemN force-pushed thefix branch 2 times, most recently from307c083 toc5210adCompare

May 30, 2017 17:10

Copy link

ContributorAuthor

GuilhemN commentedMay 30, 2017

Updated to target 3.4

nicolas-grekas modified the milestones:3.4,3.3

Jun 2, 2017

GuilhemN force-pushed thefix branch fromfd711b8 to34c5fb2Compare

June 7, 2017 10:34

Copy link

ContributorAuthor

GuilhemN commentedJun 7, 2017•
edited
Loading

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

Copy link

Member

xabbuh commentedJun 13, 2017

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

GuilhemN force-pushed thefix branch from34c5fb2 tof89b090Compare

June 13, 2017 08:13

Copy link

ContributorAuthor

GuilhemN commentedJun 13, 2017

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

dunglas mentioned this pull request

Jun 16, 2017

Fix/2.0 testsapi-platform/core#1183

Merged

GuilhemN force-pushed thefix branch fromf89b090 to5d701f8Compare

June 25, 2017 06:20

fabpot approved these changes

Jul 6, 2017

View reviewed changes

Copy link

Member

fabpot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@xabbuh Is this one ok for you?

nicolas-grekas force-pushed thefix branch from92f13c2 toc52afd6Compare

July 12, 2017 12:25

nicolas-grekas requested a review fromxabbuh

July 12, 2017 12:26

stof requested changes

Jul 12, 2017

View reviewed changes

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

		$this->assertSame($expected,$this->parser->parse($yaml, Yaml::PARSE_CONSTANT));
		}

		publicfunctiontestPhpConstantTagMappingKeyWithKeysCastToStrings()

Copy link

Member

stofJul 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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)

Copy link

ContributorAuthor

GuilhemNJul 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

ContributorAuthor

GuilhemNJul 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

done

src/Symfony/Component/Yaml/Parser.php

		}

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

Copy link

Member

stofJul 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

are you sure about this change ?

Copy link

ContributorAuthor

GuilhemNJul 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

src/Symfony/Component/Yaml/Parser.php

		if (!is_string($key) && !is_int($key)) {
		$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);

Copy link

Member

stofJul 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

ContributorAuthor

GuilhemNJul 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

GuilhemNJul 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

see#23635

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


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

Copy link

Member

stofJul 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

isn't this a BC break ?

Copy link

ContributorAuthor

GuilhemNJul 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

Member

xabbuhJul 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

GuilhemNJul 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

xabbuhJul 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Theuse statement can be removed too?

Copy link

Member

xabbuh commentedJul 19, 2017

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

Copy link

ContributorAuthor

GuilhemN commentedJul 19, 2017

@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 and0x1 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.

GuilhemN force-pushed thefix branch 2 times, most recently from200c9b9 to21ada49Compare

July 23, 2017 12:47

xabbuh requested changes

Jul 24, 2017

View reviewed changes

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

		useSymfony\Component\Validator\Mapping\ClassMetadata;
		useSymfony\Component\Yaml\Exception\ParseException;
		useSymfony\Component\Yaml\ParserasYamlParser;
		useSymfony\Component\Yaml\Yaml;

Copy link

Member

xabbuhJul 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

this must be reverted

UPGRADE-3.4.md Outdated

		After:

		```php

Copy link

Member

xabbuhJul 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

the blank line can be removed

UPGRADE-4.0.md


		After:

		```php

Copy link

Member

xabbuhJul 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

can be removed

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


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

Copy link

Member

xabbuhJul 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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

		/**
		* @dataProvider getNotPhpCompatibleMappingKeyData
		*/
		publicfunctiontestExplicitStringCastingOfMappingKeys($yaml,$expected)

Copy link

Member

xabbuhJul 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

IMO the test should be kept but being flagged as legacy

Copy link

ContributorAuthor

GuilhemNJul 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

done

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


		/**
		* @dataProvider getNonStringMappingKeysData
		*/

Copy link

Member

xabbuhJul 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

same here

Copy link

Member

xabbuh commentedJul 24, 2017

@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.

Copy link

ContributorAuthor

GuilhemN commentedJul 24, 2017•
edited
Loading

@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 reviewed

Jul 25, 2017

View reviewed changes

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

		}

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

Copy link

Member

xabbuhJul 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Can theuse statement be removed too?

Copy link

ContributorAuthor

GuilhemNJul 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

indeed

xabbuh mentioned this pull request

Jul 26, 2017

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

Closed

Copy link

Member

xabbuh commentedJul 26, 2017

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

[Yaml] Recommend using quotes instead of PARSE_KEYS_AS_STRINGS

4d9b2f9

GuilhemN force-pushed thefix branch from441454a to4d9b2f9Compare

July 26, 2017 07:34

Copy link

ContributorAuthor

GuilhemN commentedJul 26, 2017

@xabbuh done

xabbuh approved these changes

Aug 1, 2017

View reviewed changes

nicolas-grekas reviewed

Aug 1, 2017

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

fix

8189bba

Copy link

ContributorAuthor

GuilhemN commentedAug 1, 2017

@nicolas-grekas indeed.

nicolas-grekas reviewed

Aug 1, 2017

View reviewed changes

src/Symfony/Component/Yaml/Yaml.php Outdated

		constDUMP_EMPTY_ARRAY_AS_SEQUENCE =1024;

		/**
		* @deprecated since version 3.4, to be removed in 4.0. Quote your evaluable

Copy link

Member

nicolas-grekasAug 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Should be on one line

Copy link

ContributorAuthor

GuilhemNAug 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Indeed :)

fix

6e88335

Copy link

Member

xabbuh commentedAug 3, 2017

Thank you@GuilhemN.

xabbuh closed this

Aug 3, 2017

xabbuh added a commit that referenced this pull request

Aug 3, 2017

minor#22948[Yaml] Recommend using quotes instead of PARSE_KEYS_AS_S…

d9bf253

…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 <!-- don't forget updating src/**/CHANGELOG.md files -->| BC breaks?    | no| Deprecations? | yes <!-- don't forget updating UPGRADE-*.md files -->| Tests pass?   | yes| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| 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@xabbuhCommits-------b63c55c [Yaml] Recommend using quotes instead of PARSE_KEYS_AS_STRINGS

GuilhemN deleted the fix branch

November 1, 2020 14:58

Labels

Status: Needs Review Yaml

Movatterモバイル変換

Uh oh!

[Yaml] Recommend using quotes instead of PARSE_KEYS_AS_STRINGS#22948

[Yaml] Recommend using quotes instead of PARSE_KEYS_AS_STRINGS#22948

Uh oh!

Conversation

GuilhemN commentedMay 29, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuh commentedMay 30, 2017

Uh oh!

GuilhemN commentedMay 30, 2017

Uh oh!

GuilhemN commentedMay 30, 2017

Uh oh!

GuilhemN commentedJun 7, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

xabbuh commentedJun 13, 2017

Uh oh!

GuilhemN commentedJun 13, 2017

Uh oh!

fabpot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuh commentedJul 19, 2017

Uh oh!

GuilhemN commentedJul 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuh commentedJul 24, 2017

Uh oh!

GuilhemN commentedJul 24, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuilhemN commentedMay 29, 2017•
edited
Loading

GuilhemN commentedJun 7, 2017•
edited
Loading

GuilhemN commentedJul 24, 2017•
edited
Loading