[Yaml] Added support for parsing PHP constants #18626

HeahDude · 2016-04-24T06:29:45Z

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 · 2016-04-24T06:30:05Z

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 · 2016-04-24T09:30:04Z

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

xabbuh · 2016-04-24T09:35:19Z

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.

xabbuh · 2016-04-24T09:42:27Z

src/Symfony/Component/Yaml/Inline.php

@@ -562,6 +562,8 @@ private static function evaluateScalar($scalar, $flags, $references = array())
                        }

                        return;
+                    case 0 === strpos($scalar, '!php/const:'):
+                        return @constant(substr($scalar, 11));


I think we should to throw a ParseException here if the constant does not exist (or at least do that when the PARSE_EXCEPTION_ON_INVALID_TYPE flag is set).

I agree, I first did it because this flag looks indeed appropriate. I don't know why I've reverted it.

So I'll push it quickly :)

Well, now that I think of this again, I do not think we should evaluate the flag for this. Such an error imo should never be silenced.

It's not an error, it's a warning, preventing to get null, remove the @ and the last test fails because it gets 0.0 instead of null. That's why I silenced it (and removed the flag in the process without giving it a second thought).

What I mean is that this usually indicates a mistake made by the developer when writing the YAML file.

Ok I now see you answered before me, I maintain my proposal though, but I'm not against yours.

Still I would rather not add a new flag for this, the goal here is to ease configuration ; currently you need to import an xml or a php file while yaml is very common (I think and always used it only).

So needing to add a flag does not seem to "keep it easy" while using PARSE_EXCEPTION_ON_INVALID_TYPE looks more like "keep doing as usual, just pass a constant if you want".

@HeahDude Well, for the configuration of services the users would not have to do that. It's the responsibility of us to set up the parser properly in the YamlFileLoader. So we are more talking about someone who uses the parser in their own code.

Understood! So I need to:

add a flag like PARSE_PHP_CONSTANTS in Yaml class

either throw a ParseException or return null when the constant does not exist
maybe using PARSE_EXCEPTION_ON_INVALID_TYPE

add the flag PARSE_PHP_CONSTANTS to YamlFileLoader config.

Is that correct? What about point 2?

I agree. And I think as you would then have to deliberately turn on the feature, throwing an exception when a constant does not exist is the right way.

I'm almost there :)

Since the constant are not autoloaded (IIUC), one may expect to get null if a constant is not defined in an environment, so I guess we can take advantage of the triggered E_WARNING (ref http://php.net/manual/en/function.constant.php) as it will be shown as a silenced error in the profiler thanks to #16760.

Hence, I suggest we only trigger an exception when PARSE_EXCEPTION_ON_INVALID_TYPE flag is passed.

EDIT then it could be up to the user to test null !== $constant when no exception is thrown(?)

I already did such implementation but I've got an issue when the constant is not "inlined":

foo: - '!php/const:PHP_INT_MAX' # does not work bar: ['!php/const:PHP_INT_MAX'] # works baz: { foo: '!php/const:PHP_INT_MAX' } # works

I think it comes from the Parser as I only changed the Inline class for now, any lead ?

Thanks.

linaori · 2016-04-24T14:58:05Z

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 :(

xabbuh · 2016-04-25T09:43:56Z

src/Symfony/Component/DependencyInjection/CHANGELOG.md

+3.2.0
+-----
+
+ * added support for PHP constants in yaml configuration files


HeahDude · 2016-04-25T13:11:44Z

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 · 2016-04-29T08:23:04Z

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 · 2016-04-29T08:31:44Z

@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 · 2016-04-29T08:35:54Z

Could be like float or binary !! constant PHP_INT_MAX.

stof · 2016-04-29T08:45:11Z

src/Symfony/Component/Yaml/Inline.php

@@ -562,6 +564,15 @@ private static function evaluateScalar($scalar, $flags, $references = array())
                        }

                        return;
+                    case 0 === strpos($scalar, '!php/const:'):
+                        if (self::$constantSupport) {
+                            return @constant(substr($scalar, 11));


receiving null when a constant does not exist makes debugging very hard when you make a typo

@stof there is a hidden conversation on a diff above between me and @xabbuh where we argue about this. It returns null but there is a warning logged as silenced error thanks to #16760.
Isn't that enough? Maybe we could also silence it in master for the XmlLoader?

well, IMO, we should not silence things here

It would be consistent with the current XmlLoader indeed. But null will be returned anyway with an E_WARNING.

We shouldn't we trigger an exception when the const is not defined? Would look better to me ATM

Is there a way to distinguish null when assigned to a constant from when it's not defined?

yes, using defined()

OK great! Then we should do it indeed. What do others think? @xabbuh @stof

stof · 2016-04-29T08:46:33Z

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 · 2016-04-29T14:44:13Z

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

xabbuh · 2016-04-29T15:02:05Z

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 · 2016-04-29T16:11:53Z

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

fabpot · 2016-04-29T16:14:17Z

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

dunglas · 2016-04-29T16:22:53Z

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

nicolas-grekas · 2016-06-08T12:38:57Z

src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php

@@ -22,6 +22,7 @@
 use Symfony\Component\Yaml\Exception\ParseException;
 use Symfony\Component\Yaml\Parser as YamlParser;
 use Symfony\Component\ExpressionLanguage\Expression;
+use Symfony\Component\Yaml\Yaml;


can you please move it one line up in the yaml group?

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?

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

HeahDude · 2016-06-15T07:44:56Z

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

fabpot · 2016-06-15T07:47:30Z

👍

HeahDude · 2016-06-15T08:07:46Z

src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php

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


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

@Haehnchen see https://github.com/symfony/symfony/pull/18626/files#r67163458

nicolas-grekas · 2016-06-16T05:05:54Z

Status: needs work

HeahDude · 2016-06-17T09:55:30Z

Updated.

HeahDude · 2016-06-17T09:56:07Z

Status: Needs Review

fabpot · 2016-06-21T05:57:48Z

👍 (failures not related)

xabbuh · 2016-06-21T06:41:51Z

src/Symfony/Component/Yaml/Inline.php

+                            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 constant. Have you forgotten to pass "Yaml::PARSE_CONSTANT" flag to the parser?', $scalar));


[...] pass the [...]

[...] be parsed as a constant.

fabpot · 2016-06-22T05:56:11Z

@HeahDude Can you take @xabbuh comments into account?

fabpot · 2016-06-23T14:01:47Z

Thank you @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

teohhanhui · 2017-06-02T11:15:33Z

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 😄

carsonbot added the Status: Needs Review label Apr 24, 2016

javiereguiluz added Feature Yaml labels Apr 24, 2016

xabbuh reviewed Apr 24, 2016
View reviewed changes

HeahDude force-pushed the feature/yaml-php-const branch from 4f0835c to a58d960 Compare April 25, 2016 09:36

xabbuh reviewed Apr 25, 2016
View reviewed changes

HeahDude force-pushed the feature/yaml-php-const branch 2 times, most recently from 09134dc to 06fcb9f Compare April 25, 2016 13:03

stof reviewed Apr 29, 2016
View reviewed changes

nicolas-grekas reviewed Jun 8, 2016
View reviewed changes

HeahDude reviewed Jun 15, 2016
View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jun 16, 2016

HeahDude force-pushed the feature/yaml-php-const branch from 5fda13c to 6152fb5 Compare June 17, 2016 09:54

carsonbot added Status: Needs Review and removed Status: Needs Work labels Jun 17, 2016

xabbuh reviewed Jun 21, 2016
View reviewed changes

HeahDude force-pushed the feature/yaml-php-const branch 2 times, most recently from 4f861c0 to 17ec26e Compare June 23, 2016 13:44

HeahDude added 2 commits June 23, 2016 15:44

[Yaml] added support for parsing PHP constants

02d1dea

[DI] added support for PHP constants in yaml configuration files

17ec26e

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

HeahDude deleted the feature/yaml-php-const branch June 23, 2016 14:07

fabpot mentioned this pull request Oct 27, 2016

Release v3.2.0-BETA1 #20317

Merged

javiereguiluz mentioned this pull request Nov 25, 2016

Add docs for YAML constants symfony/symfony-docs#7178

Closed

[Yaml] Added support for parsing PHP constants #18626

[Yaml] Added support for parsing PHP constants #18626

Conversation

HeahDude commented Apr 24, 2016 • edited Loading

HeahDude commented Apr 24, 2016

hhamon commented Apr 24, 2016

xabbuh commented Apr 24, 2016

Choose a reason for hiding this comment

HeahDude Apr 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeahDude Apr 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeahDude Apr 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeahDude Apr 25, 2016 • edited Loading

Choose a reason for hiding this comment

linaori commented Apr 24, 2016

Choose a reason for hiding this comment

HeahDude commented Apr 25, 2016

dunglas commented Apr 29, 2016

HeahDude commented Apr 29, 2016

HeahDude commented Apr 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeahDude Apr 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Apr 29, 2016

HeahDude commented Apr 29, 2016

xabbuh commented Apr 29, 2016

dunglas commented Apr 29, 2016

fabpot commented Apr 29, 2016

dunglas commented Apr 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeahDude commented Jun 15, 2016

fabpot commented Jun 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Jun 16, 2016

HeahDude commented Jun 17, 2016 • edited Loading

HeahDude commented Jun 17, 2016

fabpot commented Jun 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Jun 22, 2016

fabpot commented Jun 23, 2016

teohhanhui commented Jun 2, 2017 • edited Loading

HeahDude commented Apr 24, 2016 •

edited

Loading

HeahDude Apr 24, 2016 •

edited

Loading

HeahDude Apr 24, 2016 •

edited

Loading

HeahDude Apr 24, 2016 •

edited

Loading

HeahDude Apr 25, 2016 •

edited

Loading

HeahDude Apr 29, 2016 •

edited

Loading

dunglas commented Apr 29, 2016 •

edited

Loading

HeahDude commented Jun 17, 2016 •

edited

Loading

teohhanhui commented Jun 2, 2017 •

edited

Loading