Skip to content

[Yaml] deprecate unquoted indicator characters #16433

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
Nov 5, 2015
Merged

[Yaml] deprecate unquoted indicator characters #16433

merged 1 commit into from
Nov 5, 2015

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Nov 3, 2015

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets FriendsOfPHP/security-advisories#111
License MIT
Doc PR

stof
stof reviewed Nov 3, 2015
View reviewed changes
src/Symfony/Component/Yaml/Inline.php
@@ -237,7 +237,7 @@ public static function parseScalar($scalar, $delimiters = null, $stringDelimiter
}

// a non-quoted string cannot start with @ or ` (reserved)
Copy link
Member

Choose a reason for hiding this comment

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

this comment should be updated too

@stof
Copy link
Member

stof commented Nov 3, 2015

We should also handle the case of unquoted values containing a :, which are currently accepted in some cases by the Symfony parser (see FriendsOfPHP/security-advisories#113)

@xabbuh xabbuh force-pushed the deprecate-indicator-chars branch 2 times, most recently from 5f54121 to dd6418a Compare November 4, 2015 19:41
@xabbuh
Copy link
Member Author

xabbuh commented Nov 4, 2015

I updated the handling of the | and > characters and now also trigger a deprecation when a colon (followed by a space) is used in a mapping value.

fabpot
fabpot reviewed Nov 4, 2015
View reviewed changes
src/Symfony/Component/Yaml/Parser.php
@@ -472,6 +473,13 @@ private function parseValue($value, $exceptionOnInvalidType, $objectSupport, $ob
return $this->parseBlockScalar($matches['separator'], preg_replace('#\d+#', '', $modifiers), (int) abs($modifiers));
}

if ('mapping' === $context && '"' !== $value[0] && "'" !== $value[0] && '[' !== $value[0] && '{' !== $value[0] && '!' !== $value[0] && false !== strpos($value, ': ')) {
@trigger_error('Using a colon in an unquoted mapping value is deprecated since Symfony 2.8 and will throw a ParseException in 3.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.

Any way to provide some context like the line of the YAML file?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@xabbuh xabbuh force-pushed the deprecate-indicator-chars branch 2 times, most recently from e76395f to a9d7d07 Compare November 4, 2015 21:09
fabpot
fabpot reviewed Nov 5, 2015
View reviewed changes
src/Symfony/Component/Yaml/Tests/ParserTest.php
EOF;

$this->deprecations = array();
set_error_handler(array($this, 'handleError'));
Copy link
Member

Choose a reason for hiding this comment

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

Can you inline the error handler as an anonymous function? It's done like that in Twig because of PHP 5.2 but we don't need that here. I should have referenced the Twig docs instead where it's done the "modern way" (sorry about that): http://twig.sensiolabs.org/doc/recipes.html#displaying-deprecation-notices

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I should have thought about this myself.

@xabbuh xabbuh force-pushed the deprecate-indicator-chars branch from a9d7d07 to 8416f7b Compare November 5, 2015 09:04
@fabpot
Copy link
Member

fabpot commented Nov 5, 2015

Thank you @xabbuh.

@fabpot fabpot merged commit 8416f7b into symfony:2.8 Nov 5, 2015
fabpot added a commit that referenced this pull request Nov 5, 2015
@fabpot
feature #16433 [Yaml] deprecate unquoted indicator characters (xabbuh)
3443230 
This PR was merged into the 2.8 branch.

Discussion
----------

[Yaml] deprecate unquoted indicator characters

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | FriendsOfPHP/security-advisories#111
| License       | MIT
| Doc PR        |

Commits
-------

8416f7b [Yaml] deprecate unquoted indicator characters
@xabbuh xabbuh deleted the deprecate-indicator-chars branch November 5, 2015 14:15
@fabpot fabpot mentioned this pull request Nov 16, 2015
Merged
@soullivaneuh
Copy link
Contributor

Any planned upgrade notes about that @xabbuh?

@xabbuh xabbuh mentioned this pull request Nov 24, 2015
Merged
@xabbuh
Copy link
Member Author

xabbuh commented Nov 24, 2015

@soullivaneuh see #16654

fabpot added a commit that referenced this pull request Nov 25, 2015
@fabpot
minor #16654 [Yaml] sync changelog and upgrade files (xabbuh)
8b58291 
This PR was merged into the 2.8 branch.

Discussion
----------

[Yaml] sync changelog and upgrade files

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

Commits
-------

062d707 [Yaml] sync changelog and upgrade files
@soullivaneuh
Copy link
Contributor

Thank you @xabbuh.

@xabbuh xabbuh mentioned this pull request Nov 25, 2015
Merged
fabpot added a commit that referenced this pull request Nov 28, 2015
@fabpot
minor #16662 [Yaml] more fixes to changelog and upgrade files (Christ…
0627979 
…ian Flothmann)

This PR was merged into the 2.8 branch.

Discussion
----------

[Yaml] more fixes to changelog and upgrade files

| Q             | A
| ------------- | ---
| Fixed tickets | #16433, #16654
| License       | MIT

Commits
-------

273ed25 [Yaml] more fixes to changelog and upgrade files
@xabbuh xabbuh mentioned this pull request May 24, 2016
Closed
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
Projects
None yet
Milestone
No milestone
Development

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