Skip to content
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 lexing nested sequences/mappings #33763

Merged
merged 1 commit into from Nov 19, 2020
Merged

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Sep 30, 2019

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets #34805, #37788, #37876, #39011, #39013, #39064
License MIT
Doc PR

src/Symfony/Component/Yaml/Parser.php Outdated Show resolved Hide resolved
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
@fabpot
Copy link
Member

@fabpot fabpot commented Aug 11, 2020

@xabbuh Is it something you're still willing to work on?

Copy link
Contributor

@simonberger simonberger left a comment

Nicely done.

I just missed a test with strings in mappings/sequences containing brackets which has been fixed in this PR as well.
As a possible test the following part of inlineNotationSpanningMultipleLinesProvider passed successfully.

            'nested collections containing strings with bracket chars' => [
                [['ba[r'], ['[ba]r'], ['bar]'], ['foo' => 'bar{'], ['foo' => 'b{ar}'], ['foo' => 'bar}']],
                <<<YAML
[
    [
        "ba[r"
    ],
    [
        '[ba]r'
    ],
    [
        "bar]"
    ],
    {
        foo: "bar{"
    },
    {
        foo: "b{ar}"
    },
    {
        foo: 'bar}'
    }
]

src/Symfony/Component/Yaml/Parser.php Outdated Show resolved Hide resolved
@xabbuh
Copy link
Member Author

@xabbuh xabbuh commented Nov 15, 2020

Many thanks to @RevZer0, @simonberger and @Matth--. I have added the tests you added in #37876, #39013, and #39064 here.

@xabbuh xabbuh force-pushed the pr-33658 branch 3 times, most recently from 728cba9 to f21bd46 Compare Nov 15, 2020
@xabbuh
Copy link
Member Author

@xabbuh xabbuh commented Nov 15, 2020

This is ready to be reviewed.

Copy link
Contributor

@simonberger simonberger left a comment

Escaped quotes are not yet skipped in lexInlineQuotedString and are leading to problems for example in this yaml test:

[
    ["te\"st"],["test"]
]

Another simple error case is:

[
    ["te\"st]"]
]

@simonberger
Copy link
Contributor

@simonberger simonberger commented Nov 17, 2020

There is (now) another small issue with escaped backslashes in quotes treated as the escape for the quote.

'escaped backslash in mapping' => [
                [['"']],
                <<<YAML
[
    ["\\"]
]
YAML
            ]

I got a segmentation error writing the yam like this: [["\\"]]. l didn't look further in. Should be related to the problem but maybe a boundary check isn't robust enough.

@xabbuh
Copy link
Member Author

@xabbuh xabbuh commented Nov 17, 2020

Did you mean [["\\\\"]]? [["\\"]] isn't valid YAML and runs into a parsing error for me locally.

@simonberger
Copy link
Contributor

@simonberger simonberger commented Nov 17, 2020

Depends how it is declared. In HEREDOC [["\\"]]in quotes '[["\\\\"]]'.
There is also for me a parser error when using new lines, but there shouldn't.

Malformed inline YAML string: ""\"] ]]]" at line 3 (near "]")

@xabbuh
Copy link
Member Author

@xabbuh xabbuh commented Nov 17, 2020

@simonberger I don't see a difference here (see also https://3v4l.org/iOVjl). Could you please send a PR with a test case that you have in mind to my branch if you still think that something isn't working as expected?

@simonberger
Copy link
Contributor

@simonberger simonberger commented Nov 17, 2020

@xabbuh I am sorry. This was indeed my wrong test.

Copy link
Contributor

@simonberger simonberger left a comment

My pleasure. LGTM now.

@fabpot
Copy link
Member

@fabpot fabpot commented Nov 19, 2020

Thank you @xabbuh.

@fabpot fabpot merged commit 308231a into symfony:4.4 Nov 19, 2020
4 of 5 checks passed
@xabbuh xabbuh deleted the pr-33658 branch Nov 19, 2020
@derrabus
Copy link
Member

@derrabus derrabus commented Nov 21, 2020

@xabbuh Can I ask you to merge 4.4 into 5.1? This PR gives me a headache.

@xabbuh
Copy link
Member Author

@xabbuh xabbuh commented Nov 21, 2020

@derrabus done 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment