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.

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

[Yaml] parse omitted inlined mapping values as null #21118

Merged
merged 1 commit into from Mar 1, 2017

Conversation

Projects
None yet
5 participants
@xabbuh
Copy link
Member

commented Jan 1, 2017

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

@GuilhemN

This comment has been minimized.

Copy link
Contributor

commented Jan 1, 2017

Happy new year ! Best wishes :)

About your change, why not replacing the + here by a * ?

@xabbuh xabbuh force-pushed the xabbuh:inline-null-parsing branch from f172cce to 47afa60 Jan 1, 2017

@xabbuh

This comment has been minimized.

Copy link
Member Author

commented Jan 1, 2017

Happy new year for you too! :)

About your change, why not replacing the + here by a * ?

That's indeed much better. I updated the code accordingly.

@GuilhemN

This comment has been minimized.

Copy link
Contributor

commented Jan 1, 2017

Having a null key should be forbidden at the same time as it is not supported by PHP. Not sure how to do it as {: foo} and {'': foo} will be considered the same way while only the second one should be valid.
Same applies for other constants btw {false: foo} should not be valid in a PHP parser but it is equivalent to {'false': foo} currently (keys are not evaluated).

@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Jan 2, 2017

@xabbuh xabbuh force-pushed the xabbuh:inline-null-parsing branch from 47afa60 to 0cd3efb Jan 2, 2017

@xabbuh xabbuh force-pushed the xabbuh:inline-null-parsing branch from 0cd3efb to 9692f38 Jan 30, 2017

@fabpot

This comment has been minimized.

Copy link
Member

commented Feb 16, 2017

@xabbuh any comments regarding the last comment by @GuilhemN?

@xabbuh

This comment has been minimized.

Copy link
Member Author

commented Feb 16, 2017

@GuilhemN @fabpot As far as I understand the specs the key cannot be omitted in a mapping and it will never be treated as a boolean value (correct me if I am wrong). The YAML parser at http://yaml-online-parser.appspot.com/ seem to confirm this.

@xabbuh xabbuh force-pushed the xabbuh:inline-null-parsing branch from 9692f38 to 8103249 Feb 16, 2017

@fabpot

This comment has been minimized.

Copy link
Member

commented Feb 16, 2017

@xabbuh I think that's exactly the problem mentioned by @GuilhemN

You can add the following test, which is valid YAML (and works great):

            'empty key is allowed' => array('{"": "foo"}', array("" => "foo")),

But this one should fail (but it does not):

            'null key is not allowed' => array('{: "foo"}', array(null => "foo")),

@xabbuh xabbuh force-pushed the xabbuh:inline-null-parsing branch from 8103249 to 10e1523 Feb 17, 2017

@xabbuh

This comment has been minimized.

Copy link
Member Author

commented Feb 17, 2017

That's indeed an issue. I opened #21643 to fix this in older maintained versions too (this one should be merged into master as it IMO is a new feature).

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 1, 2017

@xabbuh Can you rebase and add a note in the CHANGELOG?

@xabbuh xabbuh force-pushed the xabbuh:inline-null-parsing branch from 10e1523 to c473504 Mar 1, 2017

@xabbuh

This comment has been minimized.

Copy link
Member Author

commented Mar 1, 2017

done

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 1, 2017

Thank you @xabbuh.

@fabpot fabpot merged commit c473504 into symfony:master Mar 1, 2017

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 1, 2017

feature #21118 [Yaml] parse omitted inlined mapping values as null (x…
…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 xabbuh deleted the xabbuh:inline-null-parsing branch Mar 1, 2017

nicolas-grekas added a commit that referenced this pull request Mar 2, 2017

bug #21831 [Yaml] Fix legacy support for omitting mapping key (ogizan…
…agi)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[Yaml] Fix legacy support for omitting mapping key

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | N/A
| License       | MIT
| Doc PR        | N/A

Quick and dirty fix for the failing legacy `InlineTest::testOmittedMappingKeyIsParsedAsColon()` test (failing since #21118).

Ping @xabbuh : I'm not used to the yaml component codebase and it's probably not the most pleasant one to read. 😄 So if you think there is a cleaner way to go, please just close this one in favor of yours.

Commits
-------

d246f2f [Yaml] Fix legacy support for omitting mapping key

@fabpot fabpot referenced this pull request May 1, 2017

Merged

Release v3.3.0-BETA1 #22603

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.