[Yaml] Remove escaping regex #19782

GuilhemN · 2016-08-29T22:18:26Z

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 in Escaper by call to strcspn. 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

GuilhemN · 2016-08-29T22:38:48Z

src/Symfony/Component/Yaml/Escaper.php

@@ -48,7 +52,7 @@ class Escaper
     */
    public static function requiresDoubleQuoting($value)
    {
-        return preg_match('/'.self::REGEX_CHARACTER_TO_ESCAPE.'/u', $value);
+        return strlen($value) !== strcspn($value, implode('', self::$escapees));


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

nicolas-grekas · 2016-08-31T07:23:07Z

benchmark.php

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


not sure about this file :)

Indeed, i should stop using -A ^^

GuilhemN · 2016-08-31T08:54:16Z

src/Symfony/Component/Yaml/Escaper.php

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


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

Shouldn't this be done in 2.7 then?

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

so why doing it then?

Actually that's not clear what we should do here. Non-printable characters must be escaped but / is a printable 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.

fabpot · 2016-08-31T18:06:14Z

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

GuilhemN · 2016-08-31T18:15:41Z

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

fabpot · 2016-08-31T18:17:07Z

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).

xabbuh · 2016-09-02T07:05:01Z

src/Symfony/Component/Yaml/Inline.php

@@ -684,7 +684,7 @@ public static function evaluateBinaryScalar($scalar)

    private static function isBinaryString($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);


What's the reason for this change?

Because these caracters are escapable in double quoted strings

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

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.

nicolas-grekas · 2016-12-06T13:05:22Z

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.

stof · 2016-12-06T13:24:08Z

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 · 2016-12-06T13:26:54Z

src/Symfony/Component/Yaml/Escaper.php

-        return preg_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], '-?&*!|<>\'"=%@`')) {


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)

Indeed, good catch!

stof · 2016-12-06T13:30:08Z

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

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


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

Sure, the test is wrong.

nicolas-grekas · 2016-12-27T14:25:37Z

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.

GuilhemN · 2016-12-27T21:46:42Z

@nicolas-grekas this PR mostly intents to improve readability and maintainability (do you think it's obvious what preg_match('/[ \s \' " \: \{ \} \[ \] , & \* \# \?] | \A[ \- ? | < > = ! % @ ]/x', $value); does ?).
About merge conflicts I understand your concern but this file is not 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 as foo'bar as it is a valid plain scalar).

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

fabpot · 2017-03-05T20:51:51Z

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

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

carsonbot added Status: Needs Review Yaml labels Aug 29, 2016

GuilhemN force-pushed the ESCAPING branch 2 times, most recently from f470831 to e1ea62b Compare August 29, 2016 22:25

GuilhemN reviewed Aug 29, 2016
View reviewed changes

GuilhemN force-pushed the ESCAPING branch 3 times, most recently from b534d61 to 87bc631 Compare August 30, 2016 19:50

nicolas-grekas reviewed Aug 31, 2016
View reviewed changes

GuilhemN force-pushed the ESCAPING branch from 87bc631 to 0158efd Compare August 31, 2016 08:50

GuilhemN reviewed Aug 31, 2016
View reviewed changes

GuilhemN changed the base branch from 2.7 to master August 31, 2016 18:07

GuilhemN force-pushed the ESCAPING branch 2 times, most recently from d317288 to b377df8 Compare August 31, 2016 18:14

GuilhemN force-pushed the ESCAPING branch 2 times, most recently from 5be4264 to 820dc13 Compare 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 the ESCAPING branch from 820dc13 to 4386861 Compare September 1, 2016 10:01

xabbuh reviewed Sep 2, 2016
View reviewed changes

stof reviewed Dec 6, 2016

View reviewed changes

Fixes

bb0bbb8

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

Uh oh!

[Yaml] Remove escaping regex #19782

[Yaml] Remove escaping regex #19782

Uh oh!

Conversation

GuilhemN commented Aug 29, 2016 • edited Loading 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!

GuilhemN Sep 2, 2016 • edited Loading 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!

GuilhemN Dec 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Aug 31, 2016

Uh oh!

GuilhemN commented Aug 31, 2016

Uh oh!

fabpot commented Aug 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!

GuilhemN Sep 27, 2016 • edited Loading 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 commented Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stof commented Dec 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 commented Dec 27, 2016

Uh oh!

GuilhemN commented Dec 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fabpot commented Mar 5, 2017

Uh oh!

Uh oh!

GuilhemN commented Aug 29, 2016 •

edited

Loading

GuilhemN Sep 2, 2016 •

edited

Loading

GuilhemN Dec 7, 2016 •

edited

Loading

GuilhemN Sep 27, 2016 •

edited

Loading

nicolas-grekas commented Dec 6, 2016 •

edited

Loading

GuilhemN commented Dec 27, 2016 •

edited

Loading