Skip to content
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

[Yaml] Added support for parsing PHP constants #18626

Merged
merged 2 commits into from
Jun 23, 2016
Merged
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
1 change: 1 addition & 0 deletions src/Symfony/Component/DependencyInjection/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ CHANGELOG
-----

* allowed to prioritize compiler passes by introducing a third argument to `PassConfig::addPass()`, to `Compiler::addPass` and to `ContainerBuilder::addCompilerPass()`
* added support for PHP constants in YAML configuration files

3.0.0
-----
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
use Symfony\Component\Config\Resource\FileResource;
use Symfony\Component\Yaml\Exception\ParseException;
use Symfony\Component\Yaml\Parser as YamlParser;
use Symfony\Component\Yaml\Yaml;
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 please move it one line up in the yaml group?

Copy link
Contributor Author

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? The "good way" would be to move up the Expression line but as a CS fix it shouldn't be done in master, right?

Copy link
Member

Choose a reason for hiding this comment

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

let's group yaml lines now that we can (but we don't move existing lines to reduce merge conflicts)

use Symfony\Component\ExpressionLanguage\Expression;

/**
Expand Down Expand Up @@ -366,7 +367,7 @@ protected function loadFile($file)
}

try {
$configuration = $this->yamlParser->parse(file_get_contents($file));
$configuration = $this->yamlParser->parse(file_get_contents($file), Yaml::PARSE_CONSTANT);
Copy link
Member

@nicolas-grekas nicolas-grekas Jun 8, 2016

Choose a reason for hiding this comment

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

this means that the DI component now can't work with Yaml < 3.2, isn't it? So the composer.json file should get a conflict rule. Or: would it make sense to enable const parsing by default (and thus not require any change here?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

} catch (ParseException $e) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Symfony\Component\Yaml\Exception\RuntimeException might not be caught here, @xabbuh any ideas on how we should handle it?

Copy link
Member

Choose a reason for hiding this comment

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

throw new InvalidArgumentException(sprintf('The file "%s" does not contain valid YAML.', $file), 0, $e);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ parameters:
- false
- 0
- 1000.3
- !php/const:PHP_INT_MAX
bar: foo
escape: '@@escapeme'
foo_bar: '@foo_bar'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public function testLoadParameters()
$container = new ContainerBuilder();
$loader = new YamlFileLoader($container, new FileLocator(self::$fixturesPath.'/yaml'));
$loader->load('services2.yml');
$this->assertEquals(array('foo' => 'bar', 'mixedcase' => array('MixedCaseKey' => 'value'), 'values' => array(true, false, 0, 1000.3), 'bar' => 'foo', 'escape' => '@escapeme', 'foo_bar' => new Reference('foo_bar')), $container->getParameterBag()->all(), '->load() converts YAML keys to lowercase');
$this->assertEquals(array('foo' => 'bar', 'mixedcase' => array('MixedCaseKey' => 'value'), 'values' => array(true, false, 0, 1000.3, PHP_INT_MAX), 'bar' => 'foo', 'escape' => '@escapeme', 'foo_bar' => new Reference('foo_bar')), $container->getParameterBag()->all(), '->load() converts YAML keys to lowercase');
}

public function testLoadImports()
Expand All @@ -113,7 +113,7 @@ public function testLoadImports()
$loader->load('services4.yml');

$actual = $container->getParameterBag()->all();
$expected = array('foo' => 'bar', 'values' => array(true, false), 'bar' => '%foo%', 'escape' => '@escapeme', 'foo_bar' => new Reference('foo_bar'), 'mixedcase' => array('MixedCaseKey' => 'value'), 'imported_from_ini' => true, 'imported_from_xml' => true);
$expected = array('foo' => 'bar', 'values' => array(true, false, PHP_INT_MAX), 'bar' => '%foo%', 'escape' => '@escapeme', 'foo_bar' => new Reference('foo_bar'), 'mixedcase' => array('MixedCaseKey' => 'value'), 'imported_from_ini' => true, 'imported_from_xml' => true);
$this->assertEquals(array_keys($expected), array_keys($actual), '->load() imports and merges imported files');

// Bad import throws no exception due to ignore_errors value.
Expand Down
5 changes: 4 additions & 1 deletion src/Symfony/Component/DependencyInjection/composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
"php": ">=5.5.9"
},
"require-dev": {
"symfony/yaml": "~2.8|~3.0",
"symfony/yaml": "~3.2",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicolas-grekas

this means that the DI component not can't work with Yaml < 3.2, isn't it?

Isn't this line enough?

Copy link
Member

Choose a reason for hiding this comment

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

this line does apply only when testing the component, it has no effect when used as a dep of something else

"symfony/config": "~2.8|~3.0",
"symfony/expression-language": "~2.8|~3.0"
},
Expand All @@ -29,6 +29,9 @@
"symfony/expression-language": "For using expressions in service container configuration",
"symfony/proxy-manager-bridge": "Generate service proxies to lazy load them"
},
"conflict": {
"symfony/yaml": "<3.2"
},
"autoload": {
"psr-4": { "Symfony\\Component\\DependencyInjection\\": "" },
"exclude-from-classmap": [
Expand Down
9 changes: 9 additions & 0 deletions src/Symfony/Component/Yaml/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,15 @@
CHANGELOG
=========

3.2.0
-----

* Added support for parsing PHP constants:

```php
Yaml::parse('!php/const:PHP_INT_MAX', Yaml::PARSE_CONSTANT);
```

3.1.0
-----

Expand Down
15 changes: 15 additions & 0 deletions src/Symfony/Component/Yaml/Inline.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ class Inline
private static $exceptionOnInvalidType = false;
private static $objectSupport = false;
private static $objectForMap = false;
private static $constantSupport = false;
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure we need a flag here? Couldn't we unconditionally enable this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xabbuh what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Depends on whether we want to throw exceptions when a constant is not defined. Imo the flag makes sense if we then throw an exception when a constant does not exist (which would make debugging a lot easier than when we just returned null). Though if we enabled this feature unconditionally, we would not be able to throw exceptions when the constant was not defined.

Copy link
Member

Choose a reason for hiding this comment

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

Throwing the exception already depends on the exceptionOnInvalidType flag currently

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok then, they are many options.

  1. Parse constants by defaults:

    case 0 === strpos($scalar, '!php/const:'):
    if (defined($const = substr($scalar, 11))) {
        return constant($const);
    }
    if (self::$exceptionOnInvalidType) {
        throw new ParseException(sprintf('The PHP constant "%s" is not defined.', $const));
    }
    
    return;
  2. Or using a flag as is:

    // use Symfony\Component\Yaml\Exception\RuntimeException;
    
    case 0 === strpos($scalar, '!php/const:'):
    if (self::$constantSupport) {
        $const = substr($scalar, 11);
        if (defined($const)) {
            return constant($const);
        }
    
        throw new RuntimeException(sprintf('The constant "%s" is not defined.', $const));
    }
    if (self::$exceptionOnInvalidType) {
        throw new ParseException(sprintf('The string "%s" could not be parsed as constant. Have you forgotten to use "Yaml::PARSE_CONSTANT" flag?', $scalar));
    }
    
    return;
  3. Suggestions?

For now I vote for option 2, since php objects are not parsed by default.

Copy link
Member

Choose a reason for hiding this comment

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

My vote is for a flag too because this component should be as close to the Yaml spec as possible. !php/const: is a Symfony thing, not a Yaml thing.

Copy link
Member

@xabbuh xabbuh Jun 15, 2016

Choose a reason for hiding this comment

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

I agree with that. It will consistent with how we already treat PHP object support.

Copy link
Member

Choose a reason for hiding this comment

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

Fine by me, then my comment above applies.


/**
* Converts a YAML string to a PHP array.
Expand Down Expand Up @@ -77,6 +78,7 @@ public static function parse($value, $flags = 0, $references = array())
self::$exceptionOnInvalidType = (bool) (Yaml::PARSE_EXCEPTION_ON_INVALID_TYPE & $flags);
self::$objectSupport = (bool) (Yaml::PARSE_OBJECT & $flags);
self::$objectForMap = (bool) (Yaml::PARSE_OBJECT_FOR_MAP & $flags);
self::$constantSupport = (bool) (Yaml::PARSE_CONSTANT & $flags);

$value = trim($value);

Expand Down Expand Up @@ -580,6 +582,19 @@ private static function evaluateScalar($scalar, $flags, $references = array())
throw new ParseException('Object support when parsing a YAML file has been disabled.');
}

return;
case 0 === strpos($scalar, '!php/const:'):
Copy link
Member

Choose a reason for hiding this comment

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

What about:

                    case 0 === strpos($scalar, '!php/const:'):
                        if (self::$constantSupport && defined($const = substr($scalar, 11))) {
                            return constant($const);
                        }
                        if (self::$exceptionOnInvalidType) {
                            throw new ParseException(sprintf('The "%s" YAML string could not be parsed because the %s.', $scalar, self::$constantSupport ? ' "Yaml::PARSE_CONSTANT" flag was not set' : 'corresponding PHP constant was not defined'));
                        }

                        return; 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicolas-grekas Working on it I think we should throw an exception when the constant is not defined even if self::$exceptionOnInvalidType is false, what do you think @symfony/deciders?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case maybe I should only change the YAML RuntimeException to a ParseException so it's caught in the DI loader.

Copy link
Member

Choose a reason for hiding this comment

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

I think throwing an exception when a constant is not defined is a good idea.

if (self::$constantSupport) {
if (defined($const = substr($scalar, 11))) {
return constant($const);
}

throw new ParseException(sprintf('The constant "%s" is not defined.', $const));
}
if (self::$exceptionOnInvalidType) {
throw new ParseException(sprintf('The string "%s" could not be parsed as a constant. Have you forgotten to pass the "Yaml::PARSE_CONSTANT" flag to the parser?', $scalar));
}
Copy link
Member

Choose a reason for hiding this comment

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

This must throw an exception when constant support is disabled and $exceptionOnInvalidType is set (like we do for object support).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright!


return;
case 0 === strpos($scalar, '!!float '):
return (float) substr($scalar, 8);
Expand Down
38 changes: 38 additions & 0 deletions src/Symfony/Component/Yaml/Tests/InlineTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,44 @@ public function testParseWithMapObjects($yaml, $value)
$this->assertSame(serialize($value), serialize($actual));
}

/**
* @dataProvider getTestsForParsePhpConstants
*/
public function testParsePhpConstants($yaml, $value)
{
$actual = Inline::parse($yaml, Yaml::PARSE_CONSTANT);

$this->assertSame($value, $actual);
}

public function getTestsForParsePhpConstants()
{
return array(
array('!php/const:Symfony\Component\Yaml\Yaml::PARSE_CONSTANT', Yaml::PARSE_CONSTANT),
array('!php/const:PHP_INT_MAX', PHP_INT_MAX),
array('[!php/const:PHP_INT_MAX]', array(PHP_INT_MAX)),
array('{ foo: !php/const:PHP_INT_MAX }', array('foo' => PHP_INT_MAX)),
);
}

/**
* @expectedException \Symfony\Component\Yaml\Exception\ParseException
* @expectedExceptionMessage The constant "WRONG_CONSTANT" is not defined
*/
public function testParsePhpConstantThrowsExceptionWhenUndefined()
{
Inline::parse('!php/const:WRONG_CONSTANT', Yaml::PARSE_CONSTANT);
}

/**
* @expectedException \Symfony\Component\Yaml\Exception\ParseException
* @expectedExceptionMessageRegExp #The string "!php/const:PHP_INT_MAX" could not be parsed as a constant.*#
*/
public function testParsePhpConstantThrowsExceptionOnInvalidType()
{
Inline::parse('!php/const:PHP_INT_MAX', Yaml::PARSE_EXCEPTION_ON_INVALID_TYPE);
}

/**
* @group legacy
* @dataProvider getTestsForParseWithMapObjects
Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Component/Yaml/Yaml.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ class Yaml
const PARSE_DATETIME = 32;
const DUMP_OBJECT_AS_MAP = 64;
const DUMP_MULTI_LINE_LITERAL_BLOCK = 128;
const PARSE_CONSTANT = 256;

/**
* Parses YAML into a PHP value.
Expand Down