Skip to content

[Yaml] Recommend using quotes instead of PARSE_KEYS_AS_STRINGS #22948

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 34 additions & 7 deletions UPGRADE-3.4.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,13 @@ FrameworkBundle
require symfony/stopwatch` in your `dev` environment.

* Using the `KERNEL_DIR` environment variable or the automatic guessing based
on the `phpunit.xml` / `phpunit.xml.dist` file location is deprecated since 3.4.
on the `phpunit.xml` / `phpunit.xml.dist` file location is deprecated since 3.4.
Set the `KERNEL_CLASS` environment variable to the fully-qualified class name
of your Kernel instead. Not setting the `KERNEL_CLASS` environment variable
will throw an exception on 4.0 unless you override the `KernelTestCase::createKernel()`
of your Kernel instead. Not setting the `KERNEL_CLASS` environment variable
will throw an exception on 4.0 unless you override the `KernelTestCase::createKernel()`
or `KernelTestCase::getKernelClass()` method.
* The `KernelTestCase::getPhpUnitXmlDir()` and `KernelTestCase::getPhpUnitCliConfigArgument()`

* The `KernelTestCase::getPhpUnitXmlDir()` and `KernelTestCase::getPhpUnitCliConfigArgument()`
methods are deprecated since 3.4 and will be removed in 4.0.

* The `--no-prefix` option of the `translation:update` command is deprecated and
Expand Down Expand Up @@ -83,7 +83,7 @@ TwigBridge
* deprecated the `Symfony\Bridge\Twig\Form\TwigRenderer` class, use the `FormRenderer`
class from the Form component instead

* deprecated `Symfony\Bridge\Twig\Command\DebugCommand::set/getTwigEnvironment` and the ability
* deprecated `Symfony\Bridge\Twig\Command\DebugCommand::set/getTwigEnvironment` and the ability
to pass a command name as first argument

* deprecated `Symfony\Bridge\Twig\Command\LintCommand::set/getTwigEnvironment` and the ability
Expand All @@ -95,7 +95,7 @@ TwigBundle
* deprecated the `Symfony\Bundle\TwigBundle\Command\DebugCommand` class, use the `DebugCommand`
class from the Twig bridge instead

* deprecated relying on the `ContainerAwareInterface` implementation for
* deprecated relying on the `ContainerAwareInterface` implementation for
`Symfony\Bundle\TwigBundle\Command\LintCommand`

Validator
Expand All @@ -111,3 +111,30 @@ Yaml

* Using the non-specific tag `!` is deprecated and will have a different
behavior in 4.0. Use a plain integer or `!!float` instead.

* Using the `Yaml::PARSE_KEYS_AS_STRINGS` flag is deprecated as it will be
removed in 4.0.

Before:

```php
$yaml = <<<YAML
null: null key
true: boolean true
2.0: float key
YAML;

Yaml::parse($yaml, Yaml::PARSE_KEYS_AS_STRINGS);
```

After:

```php
$yaml = <<<YAML
"null": null key
"true": boolean true
"2.0": float key
YAML;

Yaml::parse($yaml);
```
36 changes: 31 additions & 5 deletions UPGRADE-4.0.md
Original file line number Diff line number Diff line change
Expand Up @@ -338,9 +338,9 @@ FrameworkBundle
class instead.

* Using the `KERNEL_DIR` environment variable and the automatic guessing based
on the `phpunit.xml` file location have been removed from the `KernelTestCase::getKernelClass()`
on the `phpunit.xml` file location have been removed from the `KernelTestCase::getKernelClass()`
method implementation. Set the `KERNEL_CLASS` environment variable to the
fully-qualified class name of your Kernel or override the `KernelTestCase::createKernel()`
fully-qualified class name of your Kernel or override the `KernelTestCase::createKernel()`
or `KernelTestCase::getKernelClass()` method instead.

* The `Symfony\Bundle\FrameworkBundle\Validator\ConstraintValidatorFactory` class has been removed.
Expand All @@ -349,10 +349,10 @@ FrameworkBundle
* The `--no-prefix` option of the `translation:update` command has
been removed.

* The `Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\AddCacheClearerPass` class has been removed.
* The `Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\AddCacheClearerPass` class has been removed.
Use the `Symfony\Component\HttpKernel\DependencyInjection\AddCacheClearerPass` class instead.

* The `Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\AddCacheWarmerPass` class has been removed.
* The `Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\AddCacheWarmerPass` class has been removed.
Use the `Symfony\Component\HttpKernel\DependencyInjection\AddCacheWarmerPass` class instead.

* The `Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\TranslationDumperPass`
Expand Down Expand Up @@ -554,7 +554,7 @@ TwigBridge
* The `TwigRendererEngine::setEnvironment()` method has been removed.
Pass the Twig Environment as second argument of the constructor instead.

* Removed `Symfony\Bridge\Twig\Command\DebugCommand::set/getTwigEnvironment` and the ability
* Removed `Symfony\Bridge\Twig\Command\DebugCommand::set/getTwigEnvironment` and the ability
to pass a command name as first argument.

* Removed `Symfony\Bridge\Twig\Command\LintCommand::set/getTwigEnvironment` and the ability
Expand Down Expand Up @@ -670,6 +670,32 @@ Yaml
Yaml::parse($yaml);
```

* Removed the `Yaml::PARSE_KEYS_AS_STRINGS` flag.

Before:

```php
$yaml = <<<YAML
null: null key
true: boolean true
2.0: float key
YAML;

Yaml::parse($yaml, Yaml::PARSE_KEYS_AS_STRINGS);
```

After:

```php
$yaml = <<<YAML
"null": null key
"true": boolean true
"2.0": float key
YAML;

Yaml::parse($yaml);
```

* Omitting the key of a mapping is not supported anymore and throws a `ParseException`.

* Mappings with a colon (`:`) that is not followed by a whitespace are not
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -609,7 +609,7 @@ protected function loadFile($file)
}

try {
$configuration = $this->yamlParser->parse(file_get_contents($file), Yaml::PARSE_CONSTANT | Yaml::PARSE_CUSTOM_TAGS | Yaml::PARSE_KEYS_AS_STRINGS);
$configuration = $this->yamlParser->parse(file_get_contents($file), Yaml::PARSE_CONSTANT | Yaml::PARSE_CUSTOM_TAGS);
} catch (ParseException $e) {
throw new InvalidArgumentException(sprintf('The file "%s" does not contain valid YAML.', $file), 0, $e);
}
Expand Down
3 changes: 1 addition & 2 deletions src/Symfony/Component/Routing/Loader/YamlFileLoader.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
use Symfony\Component\Yaml\Exception\ParseException;
use Symfony\Component\Yaml\Parser as YamlParser;
use Symfony\Component\Config\Loader\FileLoader;
use Symfony\Component\Yaml\Yaml;

/**
* YamlFileLoader loads Yaml routing files.
Expand Down Expand Up @@ -59,7 +58,7 @@ public function load($file, $type = null)
}

try {
$parsedConfig = $this->yamlParser->parse(file_get_contents($path), Yaml::PARSE_KEYS_AS_STRINGS);
$parsedConfig = $this->yamlParser->parse(file_get_contents($path));
Copy link
Member

Choose a reason for hiding this comment

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

isn't this a BC break ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not more than when we added it in 3.3 (for instance !str foo in routing files was parsed as "foo" in 3.2 and is as "!str foo" since 3.3). Reverting this change is in fact more intuitive imo as it does make the parser always spec compliant.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we do this change in #23635 then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer doing this in a minor but if you insist I can of course do it in 3.3.

Copy link
Member

Choose a reason for hiding this comment

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

The use statement can be removed too?

} catch (ParseException $e) {
throw new \InvalidArgumentException(sprintf('The file "%s" does not contain valid YAML.', $path), 0, $e);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
use Symfony\Component\Serializer\Mapping\AttributeMetadata;
use Symfony\Component\Serializer\Mapping\ClassMetadataInterface;
use Symfony\Component\Yaml\Parser;
use Symfony\Component\Yaml\Yaml;

/**
* YAML File Loader.
Expand Down Expand Up @@ -114,7 +113,7 @@ private function getClassesFromYaml()
$this->yamlParser = new Parser();
}

$classes = $this->yamlParser->parse(file_get_contents($this->file), Yaml::PARSE_KEYS_AS_STRINGS);
$classes = $this->yamlParser->parse(file_get_contents($this->file));
Copy link
Member

Choose a reason for hiding this comment

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

Can the use statement be removed too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed


if (empty($classes)) {
return array();
Expand Down
3 changes: 1 addition & 2 deletions src/Symfony/Component/Translation/Loader/YamlFileLoader.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
use Symfony\Component\Translation\Exception\LogicException;
use Symfony\Component\Yaml\Parser as YamlParser;
use Symfony\Component\Yaml\Exception\ParseException;
use Symfony\Component\Yaml\Yaml;

/**
* YamlFileLoader loads translations from Yaml files.
Expand All @@ -40,7 +39,7 @@ protected function loadResource($resource)
}

try {
$messages = $this->yamlParser->parse(file_get_contents($resource), Yaml::PARSE_KEYS_AS_STRINGS);
$messages = $this->yamlParser->parse(file_get_contents($resource));
} catch (ParseException $e) {
throw new InvalidResourceException(sprintf('Error parsing YAML, invalid file "%s"', $resource), 0, $e);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ protected function parseNodes(array $nodes)
private function parseFile($path)
{
try {
$classes = $this->yamlParser->parse(file_get_contents($path), Yaml::PARSE_KEYS_AS_STRINGS | Yaml::PARSE_CONSTANT);
$classes = $this->yamlParser->parse(file_get_contents($path), Yaml::PARSE_CONSTANT);
} catch (ParseException $e) {
throw new \InvalidArgumentException(sprintf('The file "%s" does not contain valid YAML.', $path), 0, $e);
}
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Yaml/Inline.php
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,7 @@ private static function parseMapping($mapping, $flags, &$i = 0, $references = ar
@trigger_error('Omitting the key of a mapping is deprecated and will throw a ParseException in 4.0.', E_USER_DEPRECATED);
}

if (!(Yaml::PARSE_KEYS_AS_STRINGS & $flags)) {
if (!$isKeyQuoted) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added because quoted keys should not be evaluated

$evaluatedKey = self::evaluateScalar($key, $flags, $references);

if ('' !== $key && $evaluatedKey !== $key && !is_string($evaluatedKey) && !is_int($evaluatedKey)) {
Expand Down
6 changes: 5 additions & 1 deletion src/Symfony/Component/Yaml/Parser.php
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,10 @@ public function parse($value, $flags = 0)
}
}

if (Yaml::PARSE_KEYS_AS_STRINGS & $flags) {
@trigger_error('Using the Yaml::PARSE_KEYS_AS_STRINGS flag is deprecated since version 3.4 as it will be removed in 4.0. Quote your keys when they are evaluable instead.', E_USER_DEPRECATED);
}

if (false === preg_match('//u', $value)) {
throw new ParseException('The YAML value does not appear to be valid UTF-8.');
}
Expand Down Expand Up @@ -236,7 +240,7 @@ private function doParse($value, $flags)
throw $e;
}

if (!(Yaml::PARSE_KEYS_AS_STRINGS & $flags) && !is_string($key) && !is_int($key)) {
if (!is_string($key) && !is_int($key)) {
Copy link
Member

Choose a reason for hiding this comment

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

are you sure about this change ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, evaluable keys are no longer allowed unless they're quoted, even when using Yaml::PARSE_KEYS_AS_STRINGS.

$keyType = is_numeric($key) ? 'numeric key' : 'non-string key';
@trigger_error(sprintf('Implicit casting of %s to string is deprecated since version 3.3 and will throw \Symfony\Component\Yaml\Exception\ParseException in 4.0. Quote your evaluable mapping keys instead.', $keyType), E_USER_DEPRECATED);
}
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Yaml/Tests/DumperTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ public function testSpecifications()
// TODO
} else {
eval('$expected = '.trim($test['php']).';');
$this->assertSame($expected, $this->parser->parse($this->dumper->dump($expected, 10), Yaml::PARSE_KEYS_AS_STRINGS), $test['test']);
$this->assertSame($expected, $this->parser->parse($this->dumper->dump($expected, 10)), $test['test']);
}
}
}
Expand Down
17 changes: 10 additions & 7 deletions src/Symfony/Component/Yaml/Tests/InlineTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -385,8 +385,8 @@ public function getTestsForParse()
array('[\'foo,bar\', \'foo bar\']', array('foo,bar', 'foo bar')),

// mappings
array('{foo: bar,bar: foo,false: false,null: null,integer: 12}', array('foo' => 'bar', 'bar' => 'foo', 'false' => false, 'null' => null, 'integer' => 12), Yaml::PARSE_KEYS_AS_STRINGS),
array('{ foo : bar, bar : foo, false : false, null : null, integer : 12 }', array('foo' => 'bar', 'bar' => 'foo', 'false' => false, 'null' => null, 'integer' => 12), Yaml::PARSE_KEYS_AS_STRINGS),
array('{foo: bar,bar: foo,"false": false, "null": null,integer: 12}', array('foo' => 'bar', 'bar' => 'foo', 'false' => false, 'null' => null, 'integer' => 12)),
array('{ foo : bar, bar : foo, "false" : false, "null" : null, integer : 12 }', array('foo' => 'bar', 'bar' => 'foo', 'false' => false, 'null' => null, 'integer' => 12)),
array('{foo: \'bar\', bar: \'foo: bar\'}', array('foo' => 'bar', 'bar' => 'foo: bar')),
array('{\'foo\': \'bar\', "bar": \'foo: bar\'}', array('foo' => 'bar', 'bar' => 'foo: bar')),
array('{\'foo\'\'\': \'bar\', "bar\"": \'foo: bar\'}', array('foo\'' => 'bar', 'bar"' => 'foo: bar')),
Expand Down Expand Up @@ -456,8 +456,8 @@ public function getTestsForParseWithMapObjects()
array('[\'foo,bar\', \'foo bar\']', array('foo,bar', 'foo bar')),

// mappings
array('{foo: bar,bar: foo,false: false,null: null,integer: 12}', (object) array('foo' => 'bar', 'bar' => 'foo', 'false' => false, 'null' => null, 'integer' => 12), Yaml::PARSE_OBJECT_FOR_MAP | Yaml::PARSE_KEYS_AS_STRINGS),
array('{ foo : bar, bar : foo, false : false, null : null, integer : 12 }', (object) array('foo' => 'bar', 'bar' => 'foo', 'false' => false, 'null' => null, 'integer' => 12), Yaml::PARSE_OBJECT_FOR_MAP | Yaml::PARSE_KEYS_AS_STRINGS),
array('{foo: bar,bar: foo,"false": false,"null": null,integer: 12}', (object) array('foo' => 'bar', 'bar' => 'foo', 'false' => false, 'null' => null, 'integer' => 12), Yaml::PARSE_OBJECT_FOR_MAP),
array('{ foo : bar, bar : foo, "false" : false, "null" : null, integer : 12 }', (object) array('foo' => 'bar', 'bar' => 'foo', 'false' => false, 'null' => null, 'integer' => 12), Yaml::PARSE_OBJECT_FOR_MAP),
array('{foo: \'bar\', bar: \'foo: bar\'}', (object) array('foo' => 'bar', 'bar' => 'foo: bar')),
array('{\'foo\': \'bar\', "bar": \'foo: bar\'}', (object) array('foo' => 'bar', 'bar' => 'foo: bar')),
array('{\'foo\'\'\': \'bar\', "bar\"": \'foo: bar\'}', (object) array('foo\'' => 'bar', 'bar"' => 'foo: bar')),
Expand Down Expand Up @@ -538,7 +538,7 @@ public function getTestsForDump()
array('[\'foo,bar\', \'foo bar\']', array('foo,bar', 'foo bar')),

// mappings
array('{ foo: bar, bar: foo, \'false\': false, \'null\': null, integer: 12 }', array('foo' => 'bar', 'bar' => 'foo', 'false' => false, 'null' => null, 'integer' => 12), Yaml::PARSE_KEYS_AS_STRINGS),
array('{ foo: bar, bar: foo, \'false\': false, \'null\': null, integer: 12 }', array('foo' => 'bar', 'bar' => 'foo', 'false' => false, 'null' => null, 'integer' => 12)),
array('{ foo: bar, bar: \'foo: bar\' }', array('foo' => 'bar', 'bar' => 'foo: bar')),

// nested sequences and mappings
Expand All @@ -554,7 +554,7 @@ public function getTestsForDump()

array('[foo, \'@foo.baz\', { \'%foo%\': \'foo is %foo%\', bar: \'%foo%\' }, true, \'@service_container\']', array('foo', '@foo.baz', array('%foo%' => 'foo is %foo%', 'bar' => '%foo%'), true, '@service_container')),

array('{ foo: { bar: { 1: 2, baz: 3 } } }', array('foo' => array('bar' => array(1 => 2, 'baz' => 3))), Yaml::PARSE_KEYS_AS_STRINGS),
array('{ foo: { bar: { 1: 2, baz: 3 } } }', array('foo' => array('bar' => array(1 => 2, 'baz' => 3)))),
);
}

Expand Down Expand Up @@ -739,11 +739,14 @@ public function testImplicitStringCastingOfMappingKeysIsDeprecated($yaml, $expec
}

/**
* @group legacy
* @expectedDeprecation Using the Yaml::PARSE_KEYS_AS_STRINGS flag is deprecated since version 3.4 as it will be removed in 4.0. Quote your keys when they are evaluable instead.
* @expectedDeprecation Implicit casting of incompatible mapping keys to strings is deprecated since version 3.3 and will throw \Symfony\Component\Yaml\Exception\ParseException in 4.0. Quote your evaluable mapping keys instead.
* @dataProvider getNotPhpCompatibleMappingKeyData
*/
public function testExplicitStringCastingOfMappingKeys($yaml, $expected)
{
$this->assertSame($expected, Inline::parse($yaml, Yaml::PARSE_KEYS_AS_STRINGS));
$this->assertSame($expected, Yaml::parse($yaml, Yaml::PARSE_KEYS_AS_STRINGS));
}

public function getNotPhpCompatibleMappingKeyData()
Expand Down
Loading