Skip to content

Yaml parser regression with comments and non-strings #25787

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 10 commits into from
Closed

Yaml parser regression with comments and non-strings #25787

wants to merge 10 commits into from

Conversation

alexpott
Copy link
Contributor

@alexpott alexpott commented Jan 13, 2018 •
edited
Loading

Q A
Branch? 3.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? no
Fixed tickets #25786
License MIT
Doc PR symfony/symfony-docs#...

alexpott
alexpott commented Jan 13, 2018
View reviewed changes
src/Symfony/Component/Yaml/Parser.php Outdated
// followed by a comment causes us to try to parse
// the value as a multi-line string. In this
// instance, return the empty array
if ([] === $parsedLine) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not work. If I change the {} to 1.1 ie. float then we get the parse exception.

@alexpott alexpott changed the title Yaml parser regression with comments and empty arrays Yaml parser regression with comments and non-strings Jan 14, 2018
@alexpott alexpott mentioned this pull request Jan 14, 2018
Closed
@nicolas-grekas nicolas-grekas requested a review from xabbuh January 15, 2018 13:01
@alexpott alexpott force-pushed the yaml-parser-regression branch from f71571d to 4f0c963 Compare January 15, 2018 13:59
@alexpott alexpott changed the base branch from 3.4 to 3.3 January 15, 2018 13:59
@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Jan 16, 2018
@alexpott
Copy link
Contributor Author

alexpott commented Jan 20, 2018 •
edited by javiereguiluz
Loading

I've just added a new test \Symfony\Component\Yaml\Tests\ParserTest::testMultiLineStringLastResortParsing() which shows that the Inline::parse() in the loop is a bad idea anyway.

The following yaml fails in 3.3 / 3.4 at the moment:

test:
  You can have things that don't look like strings here
  true
  yes you can

At the moment this produces an exception because the Inline::parse() parses the true to a boolean.

@xabbuh xabbuh modified the milestones: 3.3, 3.4 Jan 29, 2018
@xabbuh
Copy link
Member

xabbuh commented Jan 29, 2018

moving to the 3.4 milestone as the last bugfix release for 3.3 was published today

@stof stof changed the base branch from 3.3 to 3.4 January 29, 2018 16:29
@xabbuh
Copy link
Member

xabbuh commented Feb 16, 2018

Thank you @alexpott.

xabbuh added a commit that referenced this pull request Feb 16, 2018
@xabbuh
bug #25787 Yaml parser regression with comments and non-strings (alex…
7bcccef 
…pott)

This PR was squashed before being merged into the 3.4 branch (closes #25787).

Discussion
----------

Yaml parser regression with comments and non-strings

| Q             | A
| ------------- | ---
| Branch?       | 3.3 <!-- see below -->
| Bug fix?      | yes
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->
| Tests pass?   | no
| Fixed tickets | #25786
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features-->

<!--
- Bug fixes must be submitted against the lowest branch where they apply
  (lowest branches are regularly merged to upper ones so they get the fixes too).
- Features and deprecations must be submitted against the master branch.
- Replace this comment by a description of what your PR is solving.
-->

Commits
-------

a7e2a49 Yaml parser regression with comments and non-strings
@xabbuh xabbuh closed this Feb 16, 2018
This was referenced Mar 1, 2018
Merged
Merged
@meyerbaptiste meyerbaptiste mentioned this pull request Mar 7, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xabbuh xabbuh xabbuh approved these changes

Assignees
No one assigned
Labels
Bug Status: Reviewed
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
4 participants
@alexpott @xabbuh @nicolas-grekas @carsonbot