-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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] Fix Yaml Parser with quote end in a new line #48022
Conversation
Hey! I think @mamazu has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
67e40af
to
b3cc966
Compare
b3cc966
to
50250dc
Compare
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.
LGTM but I'd like to ping @xabbuh also :)
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.
I've tried many online parser, written in different languages, which all behave differently.
I would not change the current behavior if the spec is unclear on this, as this would be a BC break.
So, that's a 👎 from me for this change.
foo: | ||
bar: 'baz | ||
|
||
' |
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.
This is not valid YAML AFAIU. The quote must be indented.
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.
Then we should close this PR and the issue with "Won't fix", because this is exactly the case, what is requested to be fixed!
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.
If I am not mistaken, this is indeed valid YAML.
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.
I have taken some time to read up on this YAML topic. In the internet are two kinds of YAML parser for this topic. Either the content must be indented even if it is in quotes or everything in quotes can be indented free, because the part in quotes is simply considered as a text block.
What I want to say is that the PR should not be merged as it is and we should decide here how to proceed.
Solution A: everything should be how it is and we only support indented quotes, seems to be how it's described in the spec https://yaml.org/spec/1.2.2/#8111-block-indentation-indicator.
foo:
bar: 'baz
'
Solution B: be a bit more generous and allow what requested in the issue and how a lot of online parsers/validators see the quoted part as a string and allow stuff like that:
foo:
bar: 'baz1
baz2
baz3
'
Based on that I'd prefer solution A and keep it how it is as @fabpot mentioned. What do you think?
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.
I think we should merge this since this is YAML compliant, isn't it?
But there's still an issue since this fails:
--- a/src/Symfony/Component/Yaml/Tests/YamlTest.php
+++ b/src/Symfony/Component/Yaml/Tests/YamlTest.php
@@ -30,6 +30,7 @@ class YamlTest extends TestCase
$yaml = <<<YAML
foo:
bar: 'baz
+biz
'
baz: 'Lorem
@@ -38,7 +39,7 @@ foo:
foobar: 'foobar'
YAML;
- $this->assertSame(['foo' => ['bar' => "baz\n", 'baz' => "Lorem\nipsum", 'foobar' => 'foobar']], Yaml::parse($yaml));
+ $this->assertSame(['foo' => ['bar' => "baz biz\n", 'baz' => "Lorem\nipsum", 'foobar' => 'foobar']], Yaml::parse($yaml));
}
@nicolas-grekas you're right ... this is solution b, I described here ... this is not possible with the current implementation and this would be a bigger change of the yaml component. Because for that the component needs to understand the context (this is part of a quoted multiline string) ... for now it just reads the yaml line by line at this point of the yaml parser ... I'll have a look on that and see whats possible. |
c315183
to
1bc16e5
Compare
864ccf2
to
0c8ef52
Compare
Added support for a colon in the unquoted key |
YAML; | ||
|
||
$this->assertSame(['foo' => [ | ||
'bar' => "baz biz\n", |
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.
'bar' => "baz biz\n", | |
'bar' => 'baz biz ', |
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.
Is this the behavior you expect @xabbuh? Currently with this input we get this result without your suggestion.
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.
Yes, as far as I know that should be the outcome.
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.
In the YAML spec I didn't find sth. for this case so it might be a very special case. This is the ChatGPT answer for that:
An empty line is treated as a line break (
\n
)
This is also the way it works in different online interpreters which are supporting this (like this one: http://www.yaml-online-parser.appspot.com/.
This one also interpretes
baz: 'Lorem
ipsum'
As {"baz": "Lorem\nipsum"}
.
While other interpreters like https://codebeautify.org/yaml-parser-online interpret this as {"baz": "Lorem ipsum"}
.
So it seems to be not really clear how to deal with this and we should decide how to deal with it. My suggestion would be "keep it simple for this very special case" and keep it how it is atm in this PR with the \n
. What do you think?
|
||
$this->assertSame(['foo' => [ | ||
'bar' => "baz biz\n", | ||
'baz' => "Lorem\nipsum", |
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.
'baz' => "Lorem\nipsum", | |
'baz' => 'Lorem ipsum', |
'baz' => "Lorem\nipsum", | ||
'error' => "Une erreur s'est produite.", | ||
'trialMode' => "période d'essai", | ||
'double_line' => "Les utilisateurs sélectionnés n'ont pas d'email.\n", |
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.
'double_line' => "Les utilisateurs sélectionnés n'ont pas d'email.\n", | |
'double_line' => "Les utilisateurs sélectionnés n'ont pas d'email. ", |
} | ||
} | ||
|
||
if (!str_starts_with($value, "'")) { |
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.
if (!str_starts_with($value, "'")) { | |
if (0 !== strpos($value, "'")) { |
continue; | ||
} elseif ($isInMultiLineQuote) { | ||
$data[] = $this->currentLine; | ||
if (str_ends_with(rtrim($this->currentLine, ' '), "'")) { |
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.
if (str_ends_with(rtrim($this->currentLine, ' '), "'")) { | |
if ("'" === (rtrim($this->currentLine)[-1] ?? '')) { |
0c8ef52
to
dd0070c
Compare
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.
Merging it as a new feature
Thank you @maxbeckers. |
I suggest to revert this for now as the changes to the parser break existing tests in the SecurityBundle (see #53747). |
…te end in a newline (maxbeckers)" (xabbuh) This PR was merged into the 7.1 branch. Discussion ---------- [Yaml] Revert "feature #48022 Fix Yaml Parser with quote end in a newline (maxbeckers)" | Q | A | ------------- | --- | Branch? | 7.1 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | | License | MIT Unfortunately, the features as implemented breaks tests in other bundles (see https://github.com/symfony/symfony/actions/runs/7768721075/job/21186968515?pr=53745#step:8:3092). Commits ------- d709af0 Revert "feature #48022 [Yaml] Fix Yaml Parser with quote end in a new line (maxbeckers)"
This is a fix for issue #33082.
The bug described in the ticket breaks on a ending quote in a new line:
Before the fix:
Symfony\Component\Yaml\Exception\ParseException: Malformed inline YAML string: 'baz at line 4.
There was already a PR #33119, which was closed because of problems.