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

[Yaml] deprecate implicit string casting of mapping keys#21774

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

Merged

fabpot merged 1 commit intosymfony:masterfromxabbuh:deprecate-not-usable-key-parsing

Mar 6, 2017

Merged

[Yaml] deprecate implicit string casting of mapping keys#21774

fabpot merged 1 commit intosymfony:masterfromxabbuh:deprecate-not-usable-key-parsing

Mar 6, 2017

Conversation

Copy link

Member

xabbuh commentedFeb 27, 2017•
edited
Loading

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

carsonbot added Status: Needs Review Yaml Deprecation labels

Feb 27, 2017

GuilhemN reviewed

Feb 27, 2017

View reviewed changes

src/Symfony/Component/Yaml/Inline.php

		}

		if (!(Yaml::PARSE_KEYS_AS_STRING &$flags)) {
		$evaluatedKey =self::evaluateScalar($key,$flags,$references);

Copy link

Contributor

Choose a reason for hiding this comment

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

Shouldn't we also check that$evaluatedKey !== $key?

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

good idea

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

		constDUMP_MULTI_LINE_LITERAL_BLOCK =128;
		constPARSE_CONSTANT =256;
		constDUMP_EMPTY_ARRAY_AS_SEQUENCE =1024;
		constPARSE_KEYS_AS_STRING =2048;

Copy link

Contributor

Choose a reason for hiding this comment

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

It seems weird to me to allow something not allowed by the spec

Copy link

MemberAuthor

xabbuhFeb 27, 2017•
edited
Loading

Choose a reason for hiding this comment

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

If you just parse the YAML file and do not want to dump it afterwards (or consistency is not important), this may be enough.

Copy link

Contributor

Choose a reason for hiding this comment

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

Sure but it would work fine without it too so I don't think we should bother.

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

But sometimes you are not able to control how the actual YAML structure looks like. I don't think we should make it harder to parse such files.

Copy link

Contributor

Choose a reason for hiding this comment

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

But then the file is invalid, so shouldn't we consider it must be updated?
If someone has the case why not but it seems a bit too early imo to add this without being sure it will be used.

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Why would it be invalid?

Copy link

Contributor

Choose a reason for hiding this comment

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

Because it does not respect the spec

Copy link

Member

Choose a reason for hiding this comment

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

@GuilhemN the YAML file is not invalid. It is incompatible withPHP datastructures. YAML allowsany value as key of a mapping (including floats, boolean, sequences and mapping).
PHP only supports strings and integers (and it accepts floats but casts them as integer, which is loosing data).

Copy link

Contributor

GuilhemNFeb 27, 2017

Choose a reason for hiding this comment

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

Sure but then do you think we should allow someone to usefalse: foo while it's not possible in php ?

xabbuh mentioned this pull request

Feb 27, 2017

[Yaml] add tests for specific mapping keys#21650

Merged

xabbuh force-pushed thedeprecate-not-usable-key-parsing branch fromce0b647 toc82b566Compare

February 27, 2017 08:41

GuilhemN reviewed

Feb 27, 2017

View reviewed changes

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

		if (!(Yaml::PARSE_KEYS_AS_STRING &$flags)) {
		$evaluatedKey =self::evaluateScalar($key,$flags,$references);

		if ($evaluatedKey !==$key && !is_string($evaluatedKey) && !is_int($evaluatedKey)) {

Copy link

Contributor

GuilhemNFeb 27, 2017

Choose a reason for hiding this comment

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

!is_int($evaluated) should be removed imo, it is not supported currently so it should trigger a deprecation too.

xabbuh force-pushed thedeprecate-not-usable-key-parsing branch fromc82b566 tofa23697Compare

February 27, 2017 11:22

nicolas-grekas added this to the3.3 milestone

Feb 27, 2017

Copy link

Contributor

GuilhemN commentedFeb 27, 2017•
edited
Loading

What about block mappings?

xabbuh force-pushed thedeprecate-not-usable-key-parsing branch 3 times, most recently from81c4c79 to0206282Compare

March 6, 2017 13:37

Copy link

MemberAuthor

xabbuh commentedMar 6, 2017

tests for block mappings added

xabbuh commented

Mar 6, 2017

View reviewed changes

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

		EOF;

		$expected =array(
		1 =>'foo',

Copy link

MemberAuthor

xabbuhMar 6, 2017

Choose a reason for hiding this comment

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

That's actually an inconsistency in the current behaviour of the parser for inlined and block mapping (the keys would be parsed as the stringstrue andfalse inside an inlined mapping).

Copy link

Member

fabpot commentedMar 6, 2017

👍 (with a note added in the CHANGELOG)

deprecate implicit string casting of mapping keys

3c5d915

xabbuh force-pushed thedeprecate-not-usable-key-parsing branch from0206282 to3c5d915Compare

March 6, 2017 19:18

Copy link

MemberAuthor

xabbuh commentedMar 6, 2017

Changelog and upgrade files updated, I also renamedPARSE_KEYS_AS_STRING toPARSE_KEYS_AS_STRINGS for consistency.

Copy link

Member

fabpot commentedMar 6, 2017

Thank you@xabbuh.

fabpot merged commit3c5d915 intosymfony:master

Mar 6, 2017

fabpot added a commit that referenced this pull request

Mar 6, 2017

feature#21774[Yaml] deprecate implicit string casting of mapping ke…

323529c

…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

xabbuh deleted the deprecate-not-usable-key-parsing branch

March 6, 2017 19:40

fabpot mentioned this pull request

May 1, 2017

Release v3.3.0-BETA1#22603

Merged

meyerbaptiste mentioned this pull request

May 4, 2017

Add filter locator and deprecate filter collectionapi-platform/core#1099

Merged

GuilhemN mentioned this pull request

May 25, 2017

[Yaml] parse PHP constants in mapping keys#22878

Merged

lcobucci added a commit to lcobucci/doctrine2 that referenced this pull request

May 30, 2017

Force YAML parser to convert keys to string

a366c2c

Since Symfony 3.3 implicit conversion is not enabled by default so weneed to pass that flag manually.Related to:symfony/symfony#21774

lcobucci added a commit to lcobucci/doctrine2 that referenced this pull request

May 30, 2017

Force YAML parser to convert keys to string

f9062d9

Since Symfony 3.3 implicit conversion is not enabled by default so weneed to pass that flag manually.Related to:symfony/symfony#21774

Copy link

plachance commentedJun 8, 2017

The YAML linter throws the error "Implicit casting of incompatible key type to string on line X is deprecated since version 3.3" when we use !php/const in keys even if the value of the constant is a string. Is it the intended behavior? I don't fully understand the YAML specification on that regard.