Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Yaml] parse mappings with a colon in a key#20841
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
stof commentedDec 9, 2016
Does it change the parsed value for previously accepted values ? If yes, we should not put this in 2.7 IMO |
| array('{\'foo\':\'bar\', "bar":\'foo: bar\'}',array('foo' =>'bar','bar' =>'foo: bar')), | ||
| array('{\'foo\'\'\':\'bar\', "bar\"":\'foo: bar\'}',array('foo\'' =>'bar','bar"' =>'foo: bar')), | ||
| array('{\'foo:\':\'bar\', "bar: ":\'foo: bar\'}',array('foo:' =>'bar','bar:' =>'foo: bar')), | ||
| array('{foo:bar: "baz"}',array('foo:bar' =>'baz')), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@stof This was parsed differently before (it would have been parsed as['foo' => 'bar: baz']).
Actually, I am really mixed about what to do here. But I could live with merging it intomaster to be completely safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
as it was accepted before, we should indeed do it in master.
changing the behavior in a 2.7 patch release is too likely to break things silently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actually, in master, it should still parse as before and generate a deprecation notice, and the behavior changed in 4.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Then let's postpone this PR for 4.0. The deprecation should already be covered by#19504 if I didn't overlook any edge cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The current deprecation is in part wrong, values such as{foo:[bar]} or{foo:} are valid, but the deprecation is triggered ({foo:} is not supported by the parser currently so for this one, I guess it's ok).
And what about replacing its message byThe behaviour of colon not followed by an indicator (e.g " ", ",", "{", "}", "[", "]") is deprecated since 3.2. It will be considered as part of the value in 4.0.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In fact, this is not valid YAML at all. However, we have to improve the deprecation handling (see#22829).
…abbuh)This PR was merged into the 3.3-dev branch.Discussion----------[Yaml] parse omitted inlined mapping values as null| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#20841 (comment)| License | MIT| Doc PR |As@GuilhemN mentioned in#20841 (comment) when using the inline YAML notation, it is currently not possible to completely omit the mapping value to have it parsed as `null` (you have to pass `~` or `null` explicitly).Commits-------c473504 parse omitted inlined mapping values as null
xabbuh commentedMar 20, 2017
Closing here for now. To be reopened when we start the development of 4.0. |
Uh oh!
There was an error while loading.Please reload this page.