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 comment in multi line value #32767

Merged
merged 1 commit into from Jul 30, 2019

Conversation

soufianZantar
Copy link
Contributor

@soufianZantar soufianZantar commented Jul 26, 2019

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

Comments after blank lines are read as value in yaml.

`$yaml = <<<YAML
parameters:
    abc

# Comment 
YAML;

var_dump(Symfony\Component\Yaml\Yaml::parse($yaml));`

Result before fix:

array(1) {
  ["parameters"]=>
  string(13) "abc
# Comment"
}

Result after fix:

array(1) {
  ["parameters"]=>
  string(3) "abc"
}

Copy link
Member

@xabbuh xabbuh left a comment

Choose a reason for hiding this comment

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

This should be merged into 3.4 right?

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

This should be merged into 3.4 right?

Also 4.3 is affected, i think this should merged into 4.3 and 3.4.

Multi-line strings support will be removed in 4.4 and 5.0 versions, right?

@nicolas-grekas nicolas-grekas added this to the 4.3 milestone Jul 27, 2019
@xabbuh xabbuh changed the base branch from 4.3 to 3.4 July 29, 2019 15:39
@xabbuh xabbuh modified the milestones: 4.3, 3.4 Jul 29, 2019
Copy link
Member

@xabbuh xabbuh left a comment

Choose a reason for hiding this comment

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

@soufianZantar I have rebased your PR on the 3.4 branch. There's only one minor thing left to do. After that the PR is ready to be merged. Thanks for your work on this.

src/Symfony/Component/Yaml/Parser.php Outdated Show resolved Hide resolved
@nicolas-grekas nicolas-grekas changed the title [YAML]: fix comment in milti line value [Yaml] fix comment in multi line value Jul 30, 2019
@nicolas-grekas
Copy link
Member

Thank you @soufianZantar.

@nicolas-grekas nicolas-grekas merged commit dd945e3 into symfony:3.4 Jul 30, 2019
nicolas-grekas added a commit that referenced this pull request Jul 30, 2019
This PR was merged into the 3.4 branch.

Discussion
----------

[Yaml] fix comment in multi line value

| Q             | A
| ------------- | ---
| Branch?       |  4.3 and lower <!-- see below -->
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #32667    <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

<!--
Replace this notice by a short README for your feature/bugfix. This will help people
understand your PR and can be used as a start for the documentation.

Additionally (see https://symfony.com/roadmap):
 - Bug fixes must be submitted against the lowest maintained 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 branch 4.4.
 - Legacy code removals go to the master branch.
-->
Comments after blank lines are read as value in yaml.

```
`$yaml = <<<YAML
parameters:
    abc

# Comment
YAML;

var_dump(Symfony\Component\Yaml\Yaml::parse($yaml));`
```

**Result before fix:**

```
array(1) {
  ["parameters"]=>
  string(13) "abc
# Comment"
}
```
**Result after fix:**

```
array(1) {
  ["parameters"]=>
  string(3) "abc"
}
```

Commits
-------

dd945e3 fix(yml): fix comment in milti line value
This was referenced Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants