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

Conversation

Projects
None yet
@HeahDude
Member

HeahDude commented Apr 24, 2016

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

This comment has been minimized.

Show comment
Hide comment
@HeahDude

HeahDude Apr 24, 2016

Member

I was reading http://symfony.com/doc/current/components/dependency_injection/parameters.html#constants-as-parameters and felt sad about the note for yaml support.

Is this PR acceptable? Does this need a flag? Or to be reversible (which does not seem doable)?

There is no sense to handle such case with the dumper imho (unless it is passed as a string?? but what would be the use case?).

Member

HeahDude commented Apr 24, 2016

I was reading http://symfony.com/doc/current/components/dependency_injection/parameters.html#constants-as-parameters and felt sad about the note for yaml support.

Is this PR acceptable? Does this need a flag? Or to be reversible (which does not seem doable)?

There is no sense to handle such case with the dumper imho (unless it is passed as a string?? but what would be the use case?).

@hhamon

This comment has been minimized.

Show comment
Hide comment
@hhamon

hhamon Apr 24, 2016

Contributor

I'm -1 for such horrible hacks. YAML is not a suitable format to handle metada like describing a thing is a constant.

Contributor

hhamon commented Apr 24, 2016

I'm -1 for such horrible hacks. YAML is not a suitable format to handle metada like describing a thing is a constant.

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Apr 24, 2016

Member

Usually I would agree with @hhamon and I had the same feeling when reading the headline of this PR. But on the other hand @fabpot always wanted to have the Yaml component to support the things we need for Symfony (i.e. being able to configure the framework) instead of supporting the full YAML specs. Now, this is something that is supported by other configuration formats (i.e. PHP and XML), but which isn't supported for the YAML config format. Thus, I am 👍 for closing the gap.

Member

xabbuh commented Apr 24, 2016

Usually I would agree with @hhamon and I had the same feeling when reading the headline of this PR. But on the other hand @fabpot always wanted to have the Yaml component to support the things we need for Symfony (i.e. being able to configure the framework) instead of supporting the full YAML specs. Now, this is something that is supported by other configuration formats (i.e. PHP and XML), but which isn't supported for the YAML config format. Thus, I am 👍 for closing the gap.

@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Apr 24, 2016

Contributor

I would be really happy to see this implemented. I prefer yaml over xml by a lot for non-public packages, but not having constants is a big limitation to yaml :(

Contributor

iltar commented Apr 24, 2016

I would be really happy to see this implemented. I prefer yaml over xml by a lot for non-public packages, but not having constants is a big limitation to yaml :(

@HeahDude

This comment has been minimized.

Show comment
Hide comment
@HeahDude

HeahDude Apr 25, 2016

Member

Ok, comments addressed, tests are green :)

I guess the failure on low depth will have to wait for master to point on 3.2.

Member

HeahDude commented Apr 25, 2016

Ok, comments addressed, tests are green :)

I guess the failure on low depth will have to wait for master to point on 3.2.

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Apr 29, 2016

Member

Supporting PHP constants would be nice but the proposed syntax !php/const:PHP_INT_MAX is not intuitive and looks ugly. Can we have another nicer syntax?

Member

dunglas commented Apr 29, 2016

Supporting PHP constants would be nice but the proposed syntax !php/const:PHP_INT_MAX is not intuitive and looks ugly. Can we have another nicer syntax?

@HeahDude

This comment has been minimized.

Show comment
Hide comment
@HeahDude

HeahDude Apr 29, 2016

Member

@dunglas suggestions are welcome! It actually matches the object syntax !php/object, so I thought it would be both consistent and easy to learn. Have you something else in mind?

Member

HeahDude commented Apr 29, 2016

@dunglas suggestions are welcome! It actually matches the object syntax !php/object, so I thought it would be both consistent and easy to learn. Have you something else in mind?

@HeahDude

This comment has been minimized.

Show comment
Hide comment
@HeahDude

HeahDude Apr 29, 2016

Member

Could be like float or binary !! constant PHP_INT_MAX.

Member

HeahDude commented Apr 29, 2016

Could be like float or binary !! constant PHP_INT_MAX.

Show outdated Hide outdated src/Symfony/Component/Yaml/Inline.php Outdated
@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Apr 29, 2016

Member

Could be like float or binary !! constant PHP_INT_MAX.

no it cannot. !!float is part of the Yaml standard. custom tags must star with a single !, not 2

Member

stof commented Apr 29, 2016

Could be like float or binary !! constant PHP_INT_MAX.

no it cannot. !!float is part of the Yaml standard. custom tags must star with a single !, not 2

@HeahDude

This comment has been minimized.

Show comment
Hide comment
@HeahDude

HeahDude Apr 29, 2016

Member

Ok, so why not !constant PHP_INT_MAX? Although I'm not against the current syntax.

Member

HeahDude commented Apr 29, 2016

Ok, so why not !constant PHP_INT_MAX? Although I'm not against the current syntax.

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Apr 29, 2016

Member

I would not change the tag name. The php: part acts like a Namespace here which reduces the risk to conflict with custom tags from other libraries.

Member

xabbuh commented Apr 29, 2016

I would not change the tag name. The php: part acts like a Namespace here which reduces the risk to conflict with custom tags from other libraries.

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Apr 29, 2016

Member

@xabbuh it makes perfectly sense but !constant PHP_INT_MAX is really nicer.

Member

dunglas commented Apr 29, 2016

@xabbuh it makes perfectly sense but !constant PHP_INT_MAX is really nicer.

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Apr 29, 2016

Member

I'm against adding something that does not follow the YAML specification.

Member

fabpot commented Apr 29, 2016

I'm against adding something that does not follow the YAML specification.

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Apr 29, 2016

Member

If I understand correctly the spec, both proposals are valid: http://www.yaml.org/spec/1.2/spec.html#id2782728

Member

dunglas commented Apr 29, 2016

If I understand correctly the spec, both proposals are valid: http://www.yaml.org/spec/1.2/spec.html#id2782728

@@ -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);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 8, 2016

Member

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

@nicolas-grekas

nicolas-grekas Jun 8, 2016

Member

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

This comment has been minimized.

@HeahDude

HeahDude Jun 15, 2016

Member

Fixed

@HeahDude

HeahDude Jun 15, 2016

Member

Fixed

This comment has been minimized.

@HeahDude

HeahDude Jun 15, 2016

Member
@@ -28,6 +28,7 @@ class Inline
private static $exceptionOnInvalidType = false;
private static $objectSupport = false;
private static $objectForMap = false;
private static $constantSupport = false;

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 8, 2016

Member

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

@nicolas-grekas

nicolas-grekas Jun 8, 2016

Member

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

This comment has been minimized.

@HeahDude

HeahDude Jun 10, 2016

Member

@xabbuh what do you think?

@HeahDude

HeahDude Jun 10, 2016

Member

@xabbuh what do you think?

This comment has been minimized.

@xabbuh

xabbuh Jun 10, 2016

Member

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.

@xabbuh

xabbuh Jun 10, 2016

Member

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.

This comment has been minimized.

@stof

stof Jun 10, 2016

Member

Throwing the exception already depends on the exceptionOnInvalidType flag currently

@stof

stof Jun 10, 2016

Member

Throwing the exception already depends on the exceptionOnInvalidType flag currently

This comment has been minimized.

@HeahDude

HeahDude Jun 13, 2016

Member

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.

@HeahDude

HeahDude Jun 13, 2016

Member

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.

This comment has been minimized.

@javiereguiluz

javiereguiluz Jun 13, 2016

Member

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.

@javiereguiluz

javiereguiluz Jun 13, 2016

Member

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.

This comment has been minimized.

@xabbuh

xabbuh Jun 15, 2016

Member

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

@xabbuh

xabbuh Jun 15, 2016

Member

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 15, 2016

Member

Fine by me, then my comment above applies.

@nicolas-grekas

nicolas-grekas Jun 15, 2016

Member

Fine by me, then my comment above applies.

@@ -19,7 +19,7 @@
"php": ">=5.5.9"
},
"require-dev": {
"symfony/yaml": "~2.8|~3.0",
"symfony/yaml": "~3.2",

This comment has been minimized.

@HeahDude

HeahDude Jun 10, 2016

Member

@nicolas-grekas

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

Isn't this line enough?

@HeahDude

HeahDude Jun 10, 2016

Member

@nicolas-grekas

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

Isn't this line enough?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 10, 2016

Member

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

@nicolas-grekas

nicolas-grekas Jun 10, 2016

Member

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

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Jun 15, 2016

Member

@HeahDude Can you finish this PR?

Member

fabpot commented Jun 15, 2016

@HeahDude Can you finish this PR?

@HeahDude

This comment has been minimized.

Show comment
Hide comment
@HeahDude

HeahDude Jun 15, 2016

Member

Yes! With option 2 from my comment above?

Member

HeahDude commented Jun 15, 2016

Yes! With option 2 from my comment above?

@HeahDude

This comment has been minimized.

Show comment
Hide comment
@HeahDude

HeahDude Jun 15, 2016

Member

Comments addressed. Waiting for tests to be green, needs review, thanks!

Member

HeahDude commented Jun 15, 2016

Comments addressed. Waiting for tests to be green, needs review, thanks!

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Jun 15, 2016

Member

👍

Member

fabpot commented Jun 15, 2016

👍

@@ -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);
} catch (ParseException $e) {

This comment has been minimized.

@HeahDude

HeahDude Jun 15, 2016

Member

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

@HeahDude

HeahDude Jun 15, 2016

Member

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 15, 2016

Member
@@ -581,6 +584,19 @@ private static function evaluateScalar($scalar, $flags, $references = array())
}
return;
case 0 === strpos($scalar, '!php/const:'):

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 15, 2016

Member

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; 
@nicolas-grekas

nicolas-grekas Jun 15, 2016

Member

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; 

This comment has been minimized.

@HeahDude
@HeahDude

HeahDude Jun 15, 2016

Member

This comment has been minimized.

@HeahDude

HeahDude Jun 16, 2016

Member

@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?

@HeahDude

HeahDude Jun 16, 2016

Member

@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?

This comment has been minimized.

@HeahDude

HeahDude Jun 16, 2016

Member

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

@HeahDude

HeahDude Jun 16, 2016

Member

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

This comment has been minimized.

@fabpot

fabpot Jun 17, 2016

Member

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

@fabpot

fabpot Jun 17, 2016

Member

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

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jun 16, 2016

Member

Status: needs work

Member

nicolas-grekas commented Jun 16, 2016

Status: needs work

@HeahDude

This comment has been minimized.

Show comment
Hide comment
@HeahDude

HeahDude Jun 17, 2016

Member

Updated.

Member

HeahDude commented Jun 17, 2016

Updated.

@HeahDude

This comment has been minimized.

Show comment
Hide comment
@HeahDude

HeahDude Jun 17, 2016

Member

Status: Needs Review

Member

HeahDude commented Jun 17, 2016

Status: Needs Review

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Jun 21, 2016

Member

👍 (failures not related)

Member

fabpot commented Jun 21, 2016

👍 (failures not related)

array('[!php/const:PHP_INT_MAX]', array(PHP_INT_MAX)),
array('{ foo: !php/const:PHP_INT_MAX }', array('foo' => PHP_INT_MAX)),
);
}

This comment has been minimized.

@xabbuh

xabbuh Jun 21, 2016

Member

I would move the data provider just below the testParsePhpConstants test.

@xabbuh

xabbuh Jun 21, 2016

Member

I would move the data provider just below the testParsePhpConstants test.

This comment has been minimized.

@HeahDude

HeahDude Jun 23, 2016

Member

Done

@HeahDude

HeahDude Jun 23, 2016

Member

Done

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Jun 22, 2016

Member

@HeahDude Can you take @xabbuh comments into account?

Member

fabpot commented Jun 22, 2016

@HeahDude Can you take @xabbuh comments into account?

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Jun 23, 2016

Member

Thank you @HeahDude.

Member

fabpot commented Jun 23, 2016

Thank you @HeahDude.

@fabpot fabpot merged commit 17ec26e into symfony:master Jun 23, 2016

2 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Jun 23, 2016

feature #18626 [Yaml] Added support for parsing PHP constants (HeahDude)
This PR was merged into the 3.2-dev branch.

Discussion
----------

[Yaml] Added support for parsing PHP constants

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

Commits
-------

17ec26e [DI] added support for PHP constants in yaml configuration files
02d1dea [Yaml] added support for parsing PHP constants

@HeahDude HeahDude deleted the HeahDude:feature/yaml-php-const branch Jun 23, 2016

@fabpot fabpot referenced this pull request Oct 27, 2016

Merged

Release v3.2.0-BETA1 #20317

@teohhanhui

This comment has been minimized.

Show comment
Hide comment
@teohhanhui

teohhanhui Jun 2, 2017

Contributor

It's unfortunate that we did not consider it important to adhere to the YAML spec: http://www.yaml.org/spec/1.2/spec.html#tag/local/

EDIT: See #22913 😄

Contributor

teohhanhui commented Jun 2, 2017

It's unfortunate that we did not consider it important to adhere to the YAML spec: http://www.yaml.org/spec/1.2/spec.html#tag/local/

EDIT: See #22913 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment