Skip to content

[Yaml] deprecate starting plain scalars with % characters #17809

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
Merged
merged 1 commit into from
Feb 18, 2016
Merged

[Yaml] deprecate starting plain scalars with % characters #17809

merged 1 commit into from
Feb 18, 2016

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Feb 15, 2016

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets symfony/symfony-docs#6269, symfony/symfony-standard#426
License MIT
Doc PR

@xabbuh xabbuh added the Yaml label Feb 15, 2016
fabpot
fabpot reviewed Feb 15, 2016
View reviewed changes
src/Symfony/Component/Yaml/Inline.php
@@ -287,6 +287,10 @@ public static function parseScalar($scalar, $delimiters = null, $stringDelimiter
throw new ParseException(sprintf('The reserved indicator "%s" cannot start a plain scalar; you need to quote the scalar.', $output[0]));
}

if ($output && '%' === $output[0]) {
@trigger_error(sprintf('Not quoting a scalar starting with the "%s" indicator character is deprecated since Symfony 3.1 and will throw a ParseException in 4.0.', $output[0]), E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need a sprintf call here. Why not just using "%"?

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. I first thought to deprecate more indicator characters. But it turned out that all other were already handled properly.

@xabbuh xabbuh force-pushed the deprecate-unquoted-indicator-chars branch from 8031dc2 to 1e27787 Compare February 15, 2016 17:38
@xabbuh
Copy link
Member Author

xabbuh commented Feb 15, 2016

Looks like we first need to fix the dumper on lower versions.

stof
stof reviewed Feb 15, 2016
View reviewed changes
src/Symfony/Component/Yaml/Tests/InlineTest.php
$this->assertCount(1, $deprecations);
$this->assertContains('Not quoting a scalar starting with the "%" indicator character is deprecated since Symfony 3.1 and will throw a ParseException in 4.0.', $deprecations[0]);

restore_error_handler();
Copy link
Member

Choose a reason for hiding this comment

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

This won't restore it in case of an assertion failure. This should be placed in a try/finally block (or be done before assertions)

This was referenced Feb 15, 2016
Merged
Merged
@xabbuh xabbuh mentioned this pull request Feb 15, 2016
Merged
@xabbuh xabbuh force-pushed the deprecate-unquoted-indicator-chars branch from 1e27787 to ba07470 Compare February 15, 2016 18:21
@xabbuh xabbuh mentioned this pull request Feb 15, 2016
Merged
fabpot added a commit that referenced this pull request Feb 16, 2016
@fabpot
bug #17814 [DependencyInjection] fix dumped YAML snytax (xabbuh)
3f749d6 
This PR was merged into the 2.3 branch.

Discussion
----------

[DependencyInjection] fix dumped YAML snytax

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

see the failing tests in #17809

Commits
-------

30388f1 [DependencyInjection] fix dumped YAML snytax
fabpot added a commit that referenced this pull request Feb 16, 2016
@fabpot
minor #17812 [Yaml] always restore the error handler in tests (xabbuh)
9bd9978 
This PR was merged into the 2.8 branch.

Discussion
----------

[Yaml] always restore the error handler in tests

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #17809 (comment)
| License       | MIT
| Doc PR        |

The error handler must be restored even when assertions failed.

Commits
-------

7631202 [Yaml] always restore the error handler in tests
fabpot added a commit that referenced this pull request Feb 16, 2016
@fabpot
minor #17811 [FrameworkBundle] fix YAML syntax (xabbuh)
a6e934c 
This PR was merged into the 2.7 branch.

Discussion
----------

[FrameworkBundle] fix YAML syntax

| Q             | A
| ------------- | ---
| Fixed tickets |
| License       | MIT

see failing tests in #17809

Commits
-------

2da4038 [FrameworkBundle] fix YAML syntax
fabpot added a commit that referenced this pull request Feb 16, 2016
@fabpot
minor #17810 fix YAML syntax in functional tests config (xabbuh)
9b093c9 
This PR was merged into the 2.3 branch.

Discussion
----------

fix YAML syntax in functional tests config

| Q             | A
| ------------- | ---
| Fixed tickets |
| License       | MIT

see failing tests in #17809

Commits
-------

d9af4bc fix YAML syntax in functional tests config
@fabpot
Copy link
Member

fabpot commented Feb 16, 2016

@xabbuh The other PRs have been merged now. Can you merge 2.8 into master and rebase this one?

@xabbuh
Copy link
Member Author

xabbuh commented Feb 16, 2016

@fabpot Unfortunately, my fix in #17814 broke our test suite when being executed with newer versions of the Yaml component. I think we should fix the test suite for all branches first (see #17816 for the start).

@xabbuh xabbuh force-pushed the deprecate-unquoted-indicator-chars branch from ba07470 to 7e71786 Compare February 17, 2016 07:36
@xabbuh xabbuh force-pushed the deprecate-unquoted-indicator-chars branch from 7e71786 to e883a4c Compare February 18, 2016 09:35
@xabbuh
Copy link
Member Author

xabbuh commented Feb 18, 2016

Tests finally pass. This is ready to be reviewed.

ping @symfony/deciders

@fabpot
Copy link
Member

fabpot commented Feb 18, 2016

Thank you @xabbuh.

@fabpot fabpot merged commit e883a4c into symfony:master Feb 18, 2016
fabpot added a commit that referenced this pull request Feb 18, 2016
@fabpot
feature #17809 [Yaml] deprecate starting plain scalars with % charact…
3e9c268 
…ers (xabbuh)

This PR was merged into the 3.1-dev branch.

Discussion
----------

[Yaml] deprecate starting plain scalars with % characters

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | symfony/symfony-docs#6269, symfony/symfony-standard#426
| License       | MIT
| Doc PR        |

Commits
-------

e883a4c deprecate starting plain scalars with % characters
@xabbuh xabbuh deleted the deprecate-unquoted-indicator-chars branch February 18, 2016 10:27
@xabbuh xabbuh mentioned this pull request Feb 24, 2016
Closed
@fabpot fabpot mentioned this pull request May 13, 2016
Merged
revoltek-daniel added a commit to revoltek-daniel/Evolution7BugsnagBundle that referenced this pull request Aug 8, 2016
@revoltek-daniel
Fix Symfony 3.1 yaml deprecation warning
68f193e 
Since Symfony 3.1 plain scalars starting with % are deprecated.

@see symfony/symfony#17809
@revoltek-daniel revoltek-daniel mentioned this pull request Aug 8, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Status: Needs Review Yaml
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@xabbuh @fabpot @stof @carsonbot