8000 [RFC][Yaml] Consider colon not followed by spaces as a value · Issue #19874 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content
8000

[RFC][Yaml] Consider colon not followed by spaces as a value #19874

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
Closed
GuilhemN opened this issue Sep 6, 2016 · 14 comments
Closed

[RFC][Yaml] Consider colon not followed by spaces as a value #19874

GuilhemN opened this issue Sep 6, 2016 · 14 comments
Labels
Yaml
Milestone
4.0

Comments

@GuilhemN
Copy link
Contributor
GuilhemN commented Sep 6, 2016 •
edited
Loading

According to the spec, colon not followed by flow indicators ([]{},) should be considered as part of the key and not throw an exception which is gonna be the 4.0 behavior according to the deprecation.

For example, foo:{foo:bar} should be valid but it won't be in the current state.

BTW we can also consider fixing other inconsistensies:

  • [] is allowed in sf mapping values but not in the spec {fo[o: bar}
  • {} is allowed in sf sequence values but not in the spec [fo{o}: quz]
  • mapping values not followed by : (i.e. {value}) is allowed and should be converted to {value: null} (i fixed this one in [Yaml] Base the parser on a StringReader #19750, in 51b87f7)

WDYT?

cc @xabbuh
@xabbuh xabbuh added the Yaml label Sep 7, 2016
@xabbuh
Copy link
Member
xabbuh commented Sep 7, 2016

The first example you gave (foo:{foo:bar}) will correctly be parsed as a string.

For the other examples you are right and we should look into how we can improve the parser to detect these edge cases.

@GuilhemN
Copy link
Contributor Author
GuilhemN commented Sep 7, 2016

What about {foo:{bar}}? The inner mapping should be parsed correctly but according to the deprecation, it will throw an exception.

@mvrhov
Copy link
mvrhov commented Sep 7, 2016

BTW the pomm bundle uses this

pomm:
    configuration:
        my_db1:
            dsn: 'pgsql://%db_user1%:%db_password1%@%db_host1%:%db_port1%/%db_name1%'
            pomm:default: true
        my_db2:
            dsn: 'pgsql://%db_user2%:%db_password2%@%db_host2%:%db_port2%/%db_name2%'
            class:session_builder: '\PommProject\ModelManager\SessionBuilder'

so this should stay valid.

@GuilhemN
Copy link
Contributor Author
GuilhemN commented Sep 7, 2016

@mvrhov i don't think this behaves as you expect. If i'm not wrong it is equivalent to "pomm": "default: true" for the sf parser. According to the spec it should be equivalent to "pomm:default": true.

@mvrhov
Copy link
mvrhov commented Sep 7, 2016

ATM This comes out as expected "pomm:default": true

@GuilhemN
Copy link
Contributor Author
GuilhemN commented Sep 7, 2016

@mvrhov actually mapping blocks are not managed the same way than the one inlined so that's possible it works as expected (and make the inline behavior even more weird).

@githubjeka githubjeka mentioned this issue Nov 11, 2016
Closed
@xabbuh
Copy link
Member
xabbuh commented Dec 3, 2016

Is there still anything to do here? I mean can one of you give an example of a YAML string that would not be parsed as expected?

@GuilhemN
Copy link
Contributor Author
GuilhemN commented Dec 3, 2016 •
edited
Loading

{pomm:default: true} will parse as { "pomm": "default:true" } while it should be parsed as { "pomm:default": true }. I think we should update the deprecation to something compliant with the spec such as Omitting the space after the colon is deprecated and won't have the same behavior in 4.0. A colon followed by something else than "{", "}", "[", "]", " " will be considered as part of the value.

@xabbuh
Copy link
Member
xabbuh commented Dec 6, 2016

@GuilhemN This example is not valid according to the spec. You would have to quote the key then.

@GuilhemN
Copy link
Contributor Author
GuilhemN commented Dec 6, 2016

@xabbuh from what I see it is as long as : is followed by a safe character.

@xabbuh
Copy link
Member
xabbuh commented Dec 6, 2016

@GuilhemN only if they are the first character in a plain string if I don't misunderstand to which part of the spec you refer to.

@GuilhemN
Copy link
Contributor Author
GuilhemN commented Dec 6, 2016 •
edited
Loading

I don't think so, i'll detail my research:

@xabbuh xabbuh mentioned this issue Dec 9, 2016
Closed
@xabbuh
Copy link
Member
xabbuh commented Dec 9, 2016

@GuilhemN I think you are right. See my attempt in #20841 to fix this.

@xabbuh xabbuh added this to the 4.0 milestone Dec 9, 2016
@xabbuh xabbuh mentioned this issue Dec 10, 2016
Merged
fabpot added a commit that referenced this issue Dec 13, 2016
@fabpot
bug #20855 [Yaml] do not trigger deprecations for valid YAML (xabbuh)
a1a058b 
This PR was merged into the 3.2 branch.

Discussion
----------

[Yaml] do not trigger deprecations for valid YAML

| Q             | A
| ------------- | ---
| Branch?       | 3.2
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #19874
| License       | MIT
| Doc PR        |

Commits
-------

1436349 do not trigger deprecations for valid YAML
@GuilhemN
Copy link
Contributor Author

Fixed in #20855. Thanks @xabbuh !

6B4D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Yaml
Projects
None yet
Milestone
4.0
Development

No branches or pull requests
3 participants
@mvrhov @xabbuh @GuilhemN
0