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] do not eagerly filter comment lines #25241

Merged
merged 1 commit into from Dec 4, 2017

Conversation

Projects
None yet
3 participants
@xabbuh
Member

xabbuh commented Dec 1, 2017

Q A
Branch? 3.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

Trying to be clever by filtering commented lines inside
getNextEmbedBlock() does not work as expected. The # may as well be
part of a multi-line quoted string where it must not be treated as the
beginning of a comment. Thus, we only must ensure that a comment-like
line does not skip the process of getting the next line of the embed
block.

@xabbuh xabbuh added this to the 3.3 milestone Dec 1, 2017

do not eagerly filter comment lines
Trying to be clever by filtering commented lines inside
`getNextEmbedBlock()` does not work as expected. The `#` may as well be
part of a multi-line quoted string where it must not be treated as the
beginning of a comment. Thus, we only must ensure that a comment-like
line does not skip the process of getting the next line of the embed
block.
@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Dec 4, 2017

Member

Thank you @xabbuh.

Member

nicolas-grekas commented Dec 4, 2017

Thank you @xabbuh.

@nicolas-grekas nicolas-grekas merged commit d594038 into symfony:3.3 Dec 4, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Dec 4, 2017

bug #25241 [Yaml] do not eagerly filter comment lines (xabbuh)
This PR was merged into the 3.3 branch.

Discussion
----------

[Yaml] do not eagerly filter comment lines

| Q             | A
| ------------- | ---
| Branch?       | 3.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

Trying to be clever by filtering commented lines inside
`getNextEmbedBlock()` does not work as expected. The `#` may as well be
part of a multi-line quoted string where it must not be treated as the
beginning of a comment. Thus, we only must ensure that a comment-like
line does not skip the process of getting the next line of the embed
block.

Commits
-------

d594038 do not eagerly filter comment lines

@xabbuh xabbuh deleted the xabbuh:quoted-comment-multi-line-string branch Dec 4, 2017

This was referenced Dec 4, 2017

nicolas-grekas added a commit that referenced this pull request Aug 14, 2018

minor #28187 [Yaml] clean up unused code (xabbuh)
This PR was merged into the 3.4 branch.

Discussion
----------

[Yaml] clean up unused code

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

The removed lines were added in #17159 to be able to detect comment lines without running into parse errors. Part of that was reverted in #25241 when the logic was overhauled leading to now useless initialisation of variables that are never evaluated.

Commits
-------

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