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] deprecate parsing mappings without keys#21643
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
c24fcf6 to47e720aCompare| publicfunctiontestTheEmptyStringIsAValidMappingKey() | ||
| { | ||
| $this->assertSame(array('' =>'foo'), Inline::parse('{ "": foo }')); | ||
| } |
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 two tests above were already passing without this patch. I just added them to prevent future regressions.
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.
Not sure if we shouldn't just add these two tests in2.7 now.
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.
👍 for adding them in an older version, to prevent regressions on them when doing bugfixes in older branches
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.
see#21650
stof commentedFeb 17, 2017
what is the existing behavior for a mapping without key ? |
stof commentedFeb 17, 2017
if the parser is not broken currently for such case, and so accepts the input (even though it is not compliant), we should rather deprecate it instead, to be consistent with our previous changes where we give users time to update their YAML files |
xabbuh commentedFeb 17, 2017
The parser behaviour currently is inconsistent since some changes we made in 3.2. However, you are right that we should deprecate it first on Status: Needs work |
47e720a toe0ea989Compare5022517 tof2ab286Comparexabbuh commentedFeb 17, 2017
I updated here to deprecate the possibility to omit mapping keys and changed the target branch to Status: Needs Review |
stof commentedFeb 17, 2017
the other PR is now merged (including in master) |
f2ab286 to703e810Compare703e810 to135a5cbComparexabbuh commentedFeb 17, 2017
rebased |
stof commentedFeb 17, 2017
👍 |
UPGRADE-3.3.md Outdated
| Yaml | ||
| ---- | ||
| * Omitting the key of a mapping is deprecated and will lead to a`ParseException` in Symfony 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.
and throws a?
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.
updated
135a5cb toc02dca3Comparefabpot commentedFeb 17, 2017
Thank you@xabbuh. |
This PR was merged into the 3.3-dev branch.Discussion----------[Yaml] deprecate parsing mappings without keys| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |Commits-------c02dca3 [Yaml] deprecate parsing mappings without keys
This PR was merged into the 2.7 branch.Discussion----------[Yaml] add tests for specific mapping keys| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | see#21643 (comment)| License | MIT| Doc PR |Commits-------b8e0d70 [Yaml] add tests for specific mapping keys
This PR was merged into the 2.7 branch.Discussion----------[Yaml] add tests for specific mapping keys| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | seesymfony/symfony#21643 (comment)| License | MIT| Doc PR |Commits-------b8e0d705f6 [Yaml] add tests for specific mapping keys
Uh oh!
There was an error while loading.Please reload this page.