Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from Mar 6, 2017
Merged

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

merged 1 commit into from Mar 6, 2017

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Feb 27, 2017 •
edited

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

GuilhemN
GuilhemN reviewed Feb 27, 2017
View reviewed changes
src/Symfony/Component/Yaml/Inline.php Outdated
@@ -485,6 +485,14 @@ 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_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
Member Author

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
@@ -30,6 +30,7 @@ class Yaml
const DUMP_MULTI_LINE_LITERAL_BLOCK = 128;
const PARSE_CONSTANT = 256;
const DUMP_EMPTY_ARRAY_AS_SEQUENCE = 1024;
const PARSE_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
Member Author

@xabbuh xabbuh Feb 27, 2017 •
edited

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
Member Author

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
Member Author

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 with PHP datastructures. YAML allows any 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

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 use false: foo while it's not possible in php ?

@xabbuh xabbuh mentioned this pull request Feb 27, 2017
Merged
@xabbuh xabbuh force-pushed the deprecate-not-usable-key-parsing branch from ce0b647 to c82b566 Compare February 27, 2017 08:41
GuilhemN
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

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 xabbuh force-pushed the deprecate-not-usable-key-parsing branch from c82b566 to fa23697 Compare February 27, 2017 11:22
@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Feb 27, 2017
@GuilhemN
Copy link
Contributor

GuilhemN commented Feb 27, 2017 •
edited

What about block mappings?

@xabbuh xabbuh force-pushed the deprecate-not-usable-key-parsing branch 3 times, most recently from 81c4c79 to 0206282 Compare March 6, 2017 13:37
@xabbuh
Copy link
Member Author

xabbuh commented Mar 6, 2017

tests for block mappings added

xabbuh
xabbuh commented Mar 6, 2017
View reviewed changes
src/Symfony/Component/Yaml/Tests/ParserTest.php

$expected = array(
1 => 'foo',
0 => 'bar',
Copy link
Member Author

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 strings true and false inside an inlined mapping).

@fabpot
Copy link
Member

fabpot commented Mar 6, 2017

👍 (with a note added in the CHANGELOG)

@xabbuh xabbuh force-pushed the deprecate-not-usable-key-parsing branch from 0206282 to 3c5d915 Compare March 6, 2017 19:18
@xabbuh
Copy link
Member Author

xabbuh commented Mar 6, 2017

Changelog and upgrade files updated, I also renamed PARSE_KEYS_AS_STRING to PARSE_KEYS_AS_STRINGS for consistency.

@fabpot
Copy link
Member

fabpot commented Mar 6, 2017

Thank you @xabbuh.

@fabpot fabpot merged commit 3c5d915 into symfony:master Mar 6, 2017
fabpot added a commit that referenced this pull request Mar 6, 2017
@fabpot
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 xabbuh deleted the deprecate-not-usable-key-parsing branch March 6, 2017 19:40
@fabpot fabpot mentioned this pull request May 1, 2017
Merged
@meyerbaptiste meyerbaptiste mentioned this pull request May 4, 2017
Merged
@GuilhemN GuilhemN mentioned this pull request May 25, 2017
Merged
lcobucci added a commit to lcobucci/doctrine2 that referenced this pull request May 30, 2017
@lcobucci
Force YAML parser to convert keys to string
a366c2c 
Since Symfony 3.3 implicit conversion is not enabled by default so we
need 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
@lcobucci
Force YAML parser to convert keys to string
f9062d9 
Since Symfony 3.3 implicit conversion is not enabled by default so we
need to pass that flag manually.

Related to: symfony/symfony#21774
@plachance
Copy link

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.

@xabbuh
Copy link
Member Author

xabbuh commented Jun 13, 2017

@plachance Can you please open a new issue with an example YAML snippet that is causing this deprecation?

@plachance plachance mentioned this pull request Jun 13, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@GuilhemN GuilhemN GuilhemN left review comments

Assignees
No one assigned
Labels
Deprecation Status: Needs Review Yaml
Projects
None yet
Milestone
3.3
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@xabbuh @GuilhemN @fabpot @plachance @stof @nicolas-grekas @carsonbot