NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Yaml] Remove escaping regex#19782

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

GuilhemN wants to merge2 commits intosymfony:masterfromGuilhemN:ESCAPING

Closed

[Yaml] Remove escaping regex#19782

GuilhemN wants to merge2 commits intosymfony:masterfromGuilhemN:ESCAPING

Conversation

Copy link

Contributor

GuilhemN commentedAug 29, 2016•
edited
Loading

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

This PR replaces regexs inEscaper by call tostrcspn. It is much easier to read imo and performance aren't degraded.
I also updated the data provider of a test because it was in practice testing nothing (it is checking if values are quoted as expected but as they always began with\t it was always quoted anyway).

Edit: quotes are moved to indicators as they are supported inside plain scalars as said by@stefk:

It seems those characters are perfectly valid in unquoted strings:
http://yaml.org/spec/1.2/spec.html#id2788859
http://yaml-online-parser.appspot.com/?yaml=foo%3A+ba%22r%0Abaz%3A+q%27u%27x%27%0A&type=canonical_yaml

carsonbot added Status: Needs Review Yaml labels

Aug 29, 2016

GuilhemN force-pushed theESCAPING branch 2 times, most recently fromf470831 toe1ea62bCompare

August 29, 2016 22:25

GuilhemN reviewed

Aug 29, 2016

View reviewed changes

src/Symfony/Component/Yaml/Escaper.php Outdated

		publicstaticfunctionrequiresDoubleQuoting($value)
		{
		returnpreg_match('/'.self::REGEX_CHARACTER_TO_ESCAPE.'/u',$value);
		returnstrlen($value) !==strcspn($value,implode('',self::$escapees));

Copy link

ContributorAuthor

GuilhemNAug 29, 2016

Choose a reason for hiding this comment

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

This would force escaping values containing",/ and\ that weren't escaped before. Does it matter ?

GuilhemN force-pushed theESCAPING branch 3 times, most recently fromb534d61 to87bc631Compare

August 30, 2016 19:50

nicolas-grekas reviewed

Aug 31, 2016

View reviewed changes

benchmark.php Outdated

		@@ -0,0 +1,15 @@
		<?php

Copy link

Member

nicolas-grekasAug 31, 2016

Choose a reason for hiding this comment

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

not sure about this file :)

Copy link

ContributorAuthor

GuilhemNAug 31, 2016

Choose a reason for hiding this comment

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

Indeed, i should stop using-A ^^

GuilhemN force-pushed theESCAPING branch from87bc631 to0158efdCompare

August 31, 2016 08:50

GuilhemN reviewed

Aug 31, 2016

View reviewed changes

src/Symfony/Component/Yaml/Escaper.php

		'\\x18','\\x19','\\x1a','\\e','\\x1c','\\x1d','\\x1e','\\x1f',
		'\\N','\\_','\\L','\\P');
		privatestatic$escapees =array(
		'\\','\\\\','\\"','"','/',

Copy link

ContributorAuthor

GuilhemNAug 31, 2016

Choose a reason for hiding this comment

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

BTW this fixes a "bug",/ was missing.

Copy link

Member

xabbuhSep 2, 2016

Choose a reason for hiding this comment

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

Shouldn't this be done in 2.7 then?

Copy link

ContributorAuthor

GuilhemNSep 2, 2016•
edited
Loading

Choose a reason for hiding this comment

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

It works in 2.7, it should be escaped but that's not mandatory.

Copy link

Member

nicolas-grekasDec 7, 2016

Choose a reason for hiding this comment

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

so why doing it then?

Copy link

ContributorAuthor

GuilhemNDec 7, 2016•
edited
Loading

Choose a reason for hiding this comment

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

Actually that's not clear what we should do here.Non-printable characters must be escaped but/ is aprintable character (the spec says\/ is available for json compatibily), so I guess it's up to us to decide what to do.

For readability/ should indeed be better while\/ is more compliant with json. If you prefer/ I'll revert this change.

Copy link

Member

fabpot commentedAug 31, 2016

As it's not a bug fix, this should be done in master.

GuilhemN changed the base branch from2.7 tomaster

August 31, 2016 18:07

GuilhemN force-pushed theESCAPING branch 2 times, most recently fromd317288 tob377df8Compare

August 31, 2016 18:14

Copy link

ContributorAuthor

GuilhemN commentedAug 31, 2016

ok, retargeted and rebased.
So no refactoring won't be merged in older branches? That's probably a good thing :)

Copy link

Member

fabpot commentedAug 31, 2016

That's the main idea, refactorings do not fix anything, so they should be done in master (the only exception is when it gives a huge performance improvement and the changes are trivial).

GuilhemN force-pushed theESCAPING branch 2 times, most recently from5be4264 to820dc13Compare

August 31, 2016 19:17

GuilhemN mentioned this pull request

Aug 31, 2016

[Yaml] Excessive quoting when dumping string values#19800

Closed

[Yaml] Remove escaping regex

4386861

GuilhemN force-pushed theESCAPING branch from820dc13 to4386861Compare

September 1, 2016 10:01

xabbuh reviewed

Sep 2, 2016

View reviewed changes

src/Symfony/Component/Yaml/Inline.php

		privatestaticfunctionisBinaryString($value)
		{
		return !preg_match('//u',$value) \|\|preg_match('/[^\x09-\x0d\x20-\xff]/',$value);
		return !preg_match('//u',$value) \|\|preg_match('/[^\x00-\x1f\x20-\xff]/',$value);

Copy link

Member

xabbuhSep 2, 2016

Choose a reason for hiding this comment

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

What's the reason for this change?

Copy link

ContributorAuthor

GuilhemNSep 2, 2016

Choose a reason for hiding this comment

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

Because these caracters are escapable in double quoted strings

Copy link

Member

xabbuhSep 27, 2016

Choose a reason for hiding this comment

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

Ah okay, maybe we should add a comment with some reference to an external source or something like that to help understanding the code.

Copy link

ContributorAuthor

GuilhemNSep 27, 2016•
edited
Loading

Choose a reason for hiding this comment

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

in fact i'm not even sure we should use!!binary when dumping, all characters are supported in double-quoted scalars anyway.
I'll try to find what the spec says about that.

Copy link

Member

nicolas-grekas commentedDec 6, 2016•
edited
Loading

Well, since you changed some char ranges, this looks like a bug fix to me. About the const, removing it is a BC break.
Thus, I think I'd prefer keeping the regexp and focus on the other changes for 2.7.

Copy link

Member

stof commentedDec 6, 2016

About the const, removing it is a BC break.

@nicolas-grekas the whole class is marked as internal, so not covered by the BC promise

stof reviewed

Dec 6, 2016

View reviewed changes

src/Symfony/Component/Yaml/Escaper.php Outdated

		returnpreg_match('/[ \s\' " \: \{ \} \[ \] , & \* \# \?] \| \A[ \- ? \| < > = ! % @ ` ]/x',$value);
		// First character is an indicator
		// @see http://yaml.org/spec/1.2/spec.html#c-indicator
		if ($value &&1 ===strspn($value[0],'-?&*!\|<>\'"=%@`')) {

Copy link

Member

stofDec 6, 2016

Choose a reason for hiding this comment

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

this is broken for the empty string (btw, the simpler fix for that is probably to add the empty string in the array of values above)

Copy link

ContributorAuthor

GuilhemNDec 6, 2016

Choose a reason for hiding this comment

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

Indeed, good catch!

src/Symfony/Component/Yaml/Tests/DumperTest.php Outdated

		'slash' =>array('/','/'),
		'backslash' =>array('\\','\\'),
		'next-line' =>array("\xC2\x85",'"\\N"'),
		'non-breaking-space' =>array('�','�'),

Copy link

Member

stofDec 6, 2016

Choose a reason for hiding this comment

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

shouldn't this be"\_" in YAML ?

Copy link

ContributorAuthor

GuilhemNDec 6, 2016

Choose a reason for hiding this comment

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

Sure, the test is wrong.

Fixes

bb0bbb8

Copy link

Member

nicolas-grekas commentedDec 27, 2016

So, to help moving this one forward, I'm personnaly 👎 on this:

This PR replaces regexs in Escaper by call to strcspn. It is much easier to read imo and performance aren't degraded.

This is subjective, with no clear technical benefit, and will create merge conflicts that are thus not worth it.

Copy link

ContributorAuthor

GuilhemN commentedDec 27, 2016•
edited
Loading

@nicolas-grekas this PR mostly intents to improve readability and maintainability (do you think it's obvious whatpreg_match('/[ \s \' " \: \{ \} \[ \] , & \* \# \?] | \A[ \- ? | < > = ! % @ ]/x', $value); does ?).
About merge conflicts I understand your concern but this file isnot modified often (last modification of a line updated here was in january 2015) so I don't think it would be a problem.

It also improves a bit scalars dumping when quotes are in the middle of the scalar (e.g.foo'bar was dumped as'foo''bar' now it is dumped asfoo'bar as it is a valid plain scalar).

Anyway, we should at least updatethe tests which are currently useless.

Copy link

Member

fabpot commentedMar 5, 2017

Closing as I agree with@nicolas-grekas here.@GuilhemN Can you submit a PR for the tests though?

fabpot closed this

Mar 5, 2017

GuilhemN deleted the ESCAPING branch

March 6, 2017 20:30

GuilhemN mentioned this pull request

Mar 6, 2017

[Yaml] Fix the tests#21903

Merged

fabpot added a commit that referenced this pull request

Mar 6, 2017

minor#21903[Yaml] Fix the tests (GuilhemN)

f3217c3

This PR was merged into the 2.7 branch.Discussion----------[Yaml] Fix the tests| Q             | A| ------------- | ---| Branch?       | 2.7| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets || License       | MIT| Doc PR        |Fix tests that are currently useless (previously part of#19782).Commits-------5107b94 [Yaml] Fix the tests

Labels

Status: Needs Review Yaml

Movatterモバイル変換

Uh oh!

[Yaml] Remove escaping regex#19782

[Yaml] Remove escaping regex#19782

Uh oh!

Conversation

GuilhemN commentedAug 29, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuilhemNSep 2, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuilhemNDec 7, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commentedAug 31, 2016

Uh oh!

GuilhemN commentedAug 31, 2016

Uh oh!

fabpot commentedAug 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuilhemNSep 27, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commentedDec 6, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

stof commentedDec 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commentedDec 27, 2016

Uh oh!

GuilhemN commentedDec 27, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

fabpot commentedMar 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

GuilhemN commentedAug 29, 2016•
edited
Loading

GuilhemNSep 2, 2016•
edited
Loading

GuilhemNDec 7, 2016•
edited
Loading

GuilhemNSep 27, 2016•
edited
Loading

nicolas-grekas commentedDec 6, 2016•
edited
Loading

GuilhemN commentedDec 27, 2016•
edited
Loading