[Yaml] deprecate parsing mappings without keys #21643
Conversation
xabbuh
commented
Feb 17, 2017
•
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | no |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | yes |
| Tests pass? | yes |
| Fixed tickets | |
| License | MIT |
| Doc PR |
c24fcf6 to
47e720a
Compare
| public function testTheEmptyStringIsAValidMappingKey() | ||
| { | ||
| $this->assertSame(array('' => 'foo'), Inline::parse('{ "": foo }')); | ||
| } |
There was a problem hiding this comment.
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.
Not sure if we shouldn't just add these two tests in 2.7 now.
There was a problem hiding this comment.
👍 for adding them in an older version, to prevent regressions on them when doing bugfixes in older branches
|
what is the existing behavior for a mapping without key ? |
|
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 |
|
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 to
e0ea989
Compare
5022517 to
f2ab286
Compare
|
I updated here to deprecate the possibility to omit mapping keys and changed the target branch to Status: Needs Review |
|
the other PR is now merged (including in master) |
f2ab286 to
703e810
Compare
703e810 to
135a5cb
Compare
|
rebased |
|
👍 |
UPGRADE-3.3.md
Outdated
| @@ -119,6 +119,8 @@ Workflow | |||
| Yaml | |||
| ---- | |||
|
|
|||
| * Omitting the key of a mapping is deprecated and will lead to a `ParseException` in Symfony 4.0. | |||
135a5cb to
c02dca3
Compare
|
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 | see symfony/symfony#21643 (comment) | License | MIT | Doc PR | Commits ------- b8e0d705f6 [Yaml] add tests for specific mapping keys