NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Yaml] Added support for parsing PHP constants#18626

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 2 commits intosymfony:masterfromHeahDude:feature/yaml-php-const

Jun 23, 2016

Merged

[Yaml] Added support for parsing PHP constants#18626

fabpot merged 2 commits intosymfony:masterfromHeahDude:feature/yaml-php-const

Jun 23, 2016

Conversation

Copy link

Contributor

HeahDude commentedApr 24, 2016•
edited
Loading

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

carsonbot added the Status: Needs Review label

Apr 24, 2016

Copy link

ContributorAuthor

HeahDude commentedApr 24, 2016

I was readinghttp://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?).

javiereguiluz added Feature Yaml labels

Apr 24, 2016

Copy link

Contributor

hhamon commentedApr 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.

Copy link

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.

xabbuh reviewed

Apr 24, 2016

View reviewed changes

src/Symfony/Component/Yaml/Inline.php Outdated


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

Copy link

Member

xabbuhApr 24, 2016

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

HeahDudeApr 24, 2016•
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

Member

xabbuhApr 24, 2016

Choose a reason for hiding this comment

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

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.

Copy link

ContributorAuthor

HeahDudeApr 24, 2016•
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

Member

xabbuhApr 24, 2016

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

HeahDudeApr 24, 2016

Choose a reason for hiding this comment

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

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 anxml or aphp file whileyaml is very common (I think and always used it only).

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

Copy link

Member

xabbuhApr 24, 2016

Choose a reason for hiding this comment

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

@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 theYamlFileLoader. So we are more talking about someone who uses the parser in their own code.

Copy link

ContributorAuthor

HeahDudeApr 24, 2016•
edited
Loading

Choose a reason for hiding this comment

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

Understood! So I need to:

add a flag likePARSE_PHP_CONSTANTS inYaml class
either throw aParseException or returnnull when the constant does not exist
maybe usingPARSE_EXCEPTION_ON_INVALID_TYPE
add the flagPARSE_PHP_CONSTANTS toYamlFileLoader config.

Is that correct? What about point 2?

Copy link

Member

xabbuhApr 25, 2016

Choose a reason for hiding this comment

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

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.

Copy link

ContributorAuthor

HeahDudeApr 25, 2016•
edited
Loading

Choose a reason for hiding this comment

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

I'm almost there :)

Since the constant are not autoloaded (IIUC), one may expect to getnull if a constant is not defined in an environment, so I guess we can take advantage of the triggeredE_WARNING (refhttp://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 whenPARSE_EXCEPTION_ON_INVALID_TYPE flag is passed.

EDIT then it could be up to the user to testnull !== $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 workbar:['!php/const:PHP_INT_MAX']# worksbaz:{ foo: '!php/const:PHP_INT_MAX' }# works

I think it comes from theParser as I only changed theInline class for now, any lead ?

Thanks.

Copy link

Contributor

linaori commentedApr 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 force-pushed thefeature/yaml-php-const branch from4f0835c toa58d960Compare

April 25, 2016 09:36

xabbuh reviewed

Apr 25, 2016

View reviewed changes

src/Symfony/Component/DependencyInjection/CHANGELOG.md Outdated

		3.2.0
		-----

		* added support for PHP constants in yaml configuration files

Copy link

Member

xabbuhApr 25, 2016

Choose a reason for hiding this comment

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

YAML

HeahDude force-pushed thefeature/yaml-php-const branch 2 times, most recently from09134dc to06fcb9fCompare

April 25, 2016 13:03

Copy link

ContributorAuthor

HeahDude commentedApr 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.

Copy link

Member

dunglas commentedApr 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?

Copy link

ContributorAuthor

HeahDude commentedApr 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?

Copy link

ContributorAuthor

HeahDude commentedApr 29, 2016

Could be like float or binary!! constant PHP_INT_MAX.

stof reviewed

Apr 29, 2016

View reviewed changes

src/Symfony/Component/Yaml/Inline.php


		return;
		case0 ===strpos($scalar,'!php/const:'):
		if (self::$constantSupport) {

Copy link

Member

stofApr 29, 2016

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

HeahDudeApr 29, 2016

Choose a reason for hiding this comment

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

@stof there is a hidden conversation on a diff above between me and@xabbuh where we argue about this. It returnsnull 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 theXmlLoader?

Copy link

Member

stofApr 29, 2016

Choose a reason for hiding this comment

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

well, IMO, we should not silence things here

Copy link

ContributorAuthor

HeahDudeApr 29, 2016•
edited
Loading

Choose a reason for hiding this comment

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

It would be consistent with the currentXmlLoader indeed. Butnull will be returned anyway with anE_WARNING.

Copy link

Member

nicolas-grekasJun 8, 2016

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

HeahDudeJun 10, 2016

Choose a reason for hiding this comment

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

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

Copy link

Member

nicolas-grekasJun 10, 2016

Choose a reason for hiding this comment

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

yes, usingdefined()

Copy link

ContributorAuthor

HeahDudeJun 10, 2016

Choose a reason for hiding this comment

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

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

Copy link

Member

stof commentedApr 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

Copy link

ContributorAuthor

HeahDude commentedApr 29, 2016

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

Copy link

Member

xabbuh commentedApr 29, 2016

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

Copy link

Member

dunglas commentedApr 29, 2016

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

Copy link

Member

fabpot commentedApr 29, 2016

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

Copy link

Member

dunglas commentedApr 29, 2016•
edited
Loading

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

nicolas-grekas reviewed

Jun 8, 2016

View reviewed changes

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

		useSymfony\Component\Yaml\Exception\ParseException;
		useSymfony\Component\Yaml\ParserasYamlParser;
		useSymfony\Component\ExpressionLanguage\Expression;
		useSymfony\Component\Yaml\Yaml;

Copy link

Member

nicolas-grekasJun 8, 2016

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

ContributorAuthor

HeahDudeJun 10, 2016

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 theExpression line but as a CS fix it shouldn't be done in master, right?

Copy link

Member

nicolas-grekasJun 10, 2016

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)

Copy link

ContributorAuthor

HeahDude commentedJun 15, 2016

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

Copy link

Member

fabpot commentedJun 15, 2016

👍

HeahDude reviewed

Jun 15, 2016

View reviewed changes

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

		try {
		$configuration =$this->yamlParser->parse(file_get_contents($file));
		$configuration =$this->yamlParser->parse(file_get_contents($file), Yaml::PARSE_CONSTANT);
		}catch (ParseException$e) {

Copy link

ContributorAuthor

HeahDudeJun 15, 2016

Choose a reason for hiding this comment

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

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

Copy link

Member

nicolas-grekasJun 15, 2016

Choose a reason for hiding this comment

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

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

Copy link

Member

nicolas-grekas commentedJun 16, 2016

Status: needs work

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Jun 16, 2016

HeahDude force-pushed thefeature/yaml-php-const branch from5fda13c to6152fb5Compare

June 17, 2016 09:54

Copy link

ContributorAuthor

HeahDude commentedJun 17, 2016•
edited
Loading

Updated.

Copy link

ContributorAuthor

HeahDude commentedJun 17, 2016

Status: Needs Review

carsonbot added Status: Needs Review and removed Status: Needs Work labels

Jun 17, 2016

Copy link

Member

fabpot commentedJun 21, 2016

👍 (failures not related)

xabbuh reviewed

Jun 21, 2016

View reviewed changes

src/Symfony/Component/Yaml/Inline.php


		thrownewParseException(sprintf('The constant "%s" is not defined.',$const));
		}
		if (self::$exceptionOnInvalidType) {

Copy link

Member

xabbuhJun 21, 2016

Choose a reason for hiding this comment

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

[...] passthe [...]

Copy link

Member

xabbuhJun 21, 2016

Choose a reason for hiding this comment

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

[...] be parsed asa constant.

Copy link

Member

fabpot commentedJun 22, 2016

@HeahDude Can you take@xabbuh comments into account?

HeahDude force-pushed thefeature/yaml-php-const branch 2 times, most recently from4f861c0 to17ec26eCompare

June 23, 2016 13:44

HeahDude added2 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

Copy link

Member

fabpot commentedJun 23, 2016

Thank you@HeahDude.

fabpot merged commit17ec26e intosymfony:master

Jun 23, 2016

fabpot added a commit that referenced this pull request

Jun 23, 2016

feature#18626[Yaml] Added support for parsing PHP constants (HeahDude)

6e03a42

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        | TODOCommits-------17ec26e [DI] added support for PHP constants in yaml configuration files02d1dea [Yaml] added support for parsing PHP constants

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 constantssymfony/symfony-docs#7178

Closed