[Yaml] Improve YAML boolean escaping #13262
Conversation
- Moves dumping single-quoting logic into Yaml\Escaper - Ensures that PHP values which would be interpreted as booleans in older versions of the YAML spec are escaped with single quotes when dumped by the Dumper.
893db8d to
3760e67
Compare
|
This will move towards interoperability with the pecl yaml extension (which uses libyaml, which is Yaml 1.1 spec). Which is something Drupal wants to make possible for performance reasons - see https://www.drupal.org/node/1920902 - in our current patch we are using a callback in our parsing for booleans to treat other than true/false as strings. One other issue we've seen between pecl/yaml and symfony/yaml is that |
| */ | ||
| private static function isValueRequiresSingleQuoting($value) | ||
| { | ||
| return in_array(strtolower($value), array('null', '~', 'true', 'false', 'y', 'n', 'yes', 'no', 'on', 'off')); |
There was a problem hiding this comment.
Note to myself and others: even if we do not support 'y', 'n', ... as valid Booleans, it does not hurt to escape them here for interoperability.
There was a problem hiding this comment.
I suggest adding it as a comment to avoid removing it by mistake in the future
Can you open an issue for that ? |
Remove duplicate 'require' for symfony/symfony PR
Let me confirm it first, I suspect it might have been hand-edited, but if not - will open |
Add comment as requested - see comment for stof
| * @param string $value A PHP value | ||
| * @return bool True if the value would require single quotes. | ||
| */ | ||
| private static function isValueRequiresSingleQuoting($value) |
There was a problem hiding this comment.
Actually, I'm wondering if we really need those 2 private methods. What about inlining them with the right comments instead? Especially as they can be called quite a lot.
|
👍 after my small comment is addressed. |
29dc388 to
8fa056b
Compare
|
@fabpot They're gone. :) |
|
Thank you @petert82. |
This PR was merged into the 2.3 branch. Discussion ---------- [Yaml] Improve YAML boolean escaping | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #13209 | License | MIT | Doc PR | None This PR ensures that PHP [values which would be interpreted as booleans][1] in older versions of the YAML spec are escaped with single quotes when dumped by the Dumper. For example, dumping this: ```php array( 'country_code' => 'no', 'speaks_norwegian' => 'y', 'heating' => 'on', ) ``` Will produce this YAML: ```yaml country_code: 'no' speaks_norwegian: 'y' heating: 'on' ``` [1]: http://yaml.org/type/bool.html Commits ------- 8fa056b Inline private 'is quoting required' methods in Escaper afe827a Merge pull request #2 from larowlan/patch-2 a0ec0fe Add comment as requested 1e0633e Merge pull request #1 from larowlan/patch-1 81a8090 Remove duplicate 'require' 3760e67 [Yaml] Improve YAML boolean escaping
|
|
||
| // Determines if a PHP value is entirely composed of a value that would | ||
| // require single quoting in YAML. | ||
| return in_array(strtolower($value), array('null', '~', 'true', 'false', 'y', 'n', 'yes', 'no', 'on', 'off')); |
There was a problem hiding this comment.
I would do the in_array check before the regex check. It is likely to be faster
… (xabbuh) This PR was merged into the 2.3 branch. Discussion ---------- [Yaml] execute cheaper checks before more expensive ones | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Minor improvements to the checks as suggested by @stof in #13262. Commits ------- cd4349d execute cheaper checks before more expensive ones
This PR ensures that PHP values which would be interpreted as booleans in older versions of the YAML spec are escaped with single quotes when dumped by the Dumper.
For example, dumping this:
Will produce this YAML: