[Dumper] Trim leading newlines when checking if value begins with a space #50066
Conversation
bradtreloar
commented
Apr 20, 2023
•
bradtreloar
commented
| Q | A |
|---|---|
| Branch? | 5.4 |
| Bug fix? | yes |
| New feature? | no |
| Deprecations? | no |
| Tickets | Fix #50065 |
| License | MIT |
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
bradtreloar
changed the title
| @@ -69,9 +69,9 @@ public function dump($input, int $inline = 0, int $indent = 0, int $flags = 0): | |||
| } | |||
|
|
|||
| if (Yaml::DUMP_MULTI_LINE_LITERAL_BLOCK & $flags && \is_string($value) && false !== strpos($value, "\n") && false === strpos($value, "\r")) { | |||
| // If the first line starts with a space character, the spec requires a blockIndicationIndicator | |||
| // If the first non-empty line starts with a space character, the spec requires a blockIndicationIndicator | |||
There was a problem hiding this comment.
Is that true? Or what happens when we change the fixture from your test to something like this?
$data = [
'data' => [
'multi_line' => "\n \n the first line has leading newline and spaces\nThe second line does not.",
],
];There was a problem hiding this comment.
The parser treats any line containing only spaces as an empty line, even if the number of space exceeds the indentation.
So it isn't enough to trim off the first newline before looking for a leading space, the dumper needs to find the first line that is neither empty nor contains only spaces.
I've added an additional test to check that the indicator is not added if the value begins with a line containing only spaces.
The indentation indicator check is now more than a single-line operation, so I've moved it into a private function.
|
The fix itself looks good to me now. Can you take a look at the things fabbot reported though please? |
carsonbot
changed the title
|
👍 just one last minor comment |
|
Thank you @bradtreloar. |
