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] Fix Yaml Parser with quote end in a new line#48022
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
carsonbot commentedOct 29, 2022
Hey! I think@mamazu has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
67e40af tob3cc966Compareb3cc966 to50250dcCompare
nicolas-grekas left a comment
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 :)
Uh oh!
There was an error while loading.Please reload this page.
fabpot left a comment
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.
maxbeckersFeb 10, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 spechttps://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: 'baz1baz2 baz3'Based on that I'd prefer solution A and keep it how it is as@fabpot mentioned. What do you think?
nicolas-grekas left a comment
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)); }
maxbeckers commentedJul 7, 2023
@nicolas-grekas you're right ... this is solution b, I describedhere ... 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 to1bc16e5Compare864ccf2 to0c8ef52Comparemaxbeckers commentedJul 11, 2023
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 likehttps://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] ??'')) { |
fabpot left a comment
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
fabpot commentedFeb 3, 2024
Thank you@maxbeckers. |
xabbuh commentedFeb 3, 2024
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 | MITUnfortunately, the features as implemented breaks tests in other bundles (seehttps://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)"
Uh oh!
There was an error while loading.Please reload this page.
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.