[Yaml] Fix regression when trying to parse multiline #26387
Conversation
|
Seems it might be fixing #22967 and avoiding regression again as it was working in the previous releases |
3065764 to
0a695a7
Compare
| @@ -421,7 +421,7 @@ private function doParse($value, $flags) | |||
| } | |||
|
|
|||
| // try to parse the value as a multi-line string as a last resort | |||
| if (0 === $this->currentLineNb) { | |||
| if (0 === $this->currentLineNb && 1 < $this->totalNumberOfLines && !$this->isNextLineIndented()) { | |||
There was a problem hiding this comment.
foo:
bar
bazWill such a YAML string still work?
There was a problem hiding this comment.
the result should be ['foo' => 'bar baz']
There was a problem hiding this comment.
indeed it doesn't. I'll add more tests and try to find another fix
|
Status: Needs Work |
f47ced3 to
36474f8
Compare
|
Status: Needs Review @xabbuh I've tried another approach and only account for indentation inconsistency at offset 0. I added more test based on your comments and my findings |
001de1b to
4b90d8e
Compare
|
@xabbuh is this good enough or is there missing test or a better/shorter solution ? |
| @@ -427,6 +427,10 @@ private function doParse($value, $flags) | |||
| $value = ''; | |||
|
|
|||
| foreach ($this->lines as $line) { | |||
| // If the indentation is not consistent at offset 0, it is to be considered as a ParseError | |||
| if (0 === $this->offset && !$deprecatedUsage && rtrim($line) !== trim($line)) { | |||
There was a problem hiding this comment.
What about using 0 === $this->offset && !$deprecatedUsage && isset($line[0]) && ' ' === $line[0] here instead to avoid calling (r)trim()?
There was a problem hiding this comment.
And is the check for $deprecatedUsage really needed? Which test would fail without it?
There was a problem hiding this comment.
@xabbuh thank you very much for your review and your feedback. Your version would work of course, I'll update.
The $deprecated check is only needed in 3.4 because of this test....
https://github.com/symfony/symfony/blob/3.4/src/Symfony/Component/Yaml/Tests/ParserTest.php#L1787-L1800 that checks that the deprecation message is thrown.
In 4.0, the exception is expected (https://github.com/symfony/symfony/blob/4.0/src/Symfony/Component/Yaml/Tests/ParserTest.php#L1689-L1702)
I'm not familiar with how you idealy deals with this...
4b90d8e to
081f866
Compare
|
updated and rebased @xabbuh |
|
failing test are not related |
|
Does this PR fix #22967? If yes, could you please update the PR description accordingly? |
|
It does @nicolas-grekas, PR description edited as requested |
081f866 to
fb0ccf9
Compare
|
Thank you @antograssiot. |
…grassiot) This PR was squashed before being merged into the 3.4 branch (closes #26387). Discussion ---------- [Yaml] Fix regression when trying to parse multiline | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #22967 | License | MIT | Doc PR | I think this is the lower impacted branch, it fixes a regression added in the last releases campain some days ago. As discused on Slack @xabbuh Commits ------- e787ecf [Yaml] Fix regression when trying to parse multiline
I think this is the lower impacted branch, it fixes a regression added in the last releases campain some days ago.
As discused on Slack @xabbuh