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

[Yaml] parse PHP constants in mapping keys#22878

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

nicolas-grekas merged 1 commit intosymfony:3.3fromxabbuh:issue-22854

May 25, 2017

Merged

[Yaml] parse PHP constants in mapping keys#22878

nicolas-grekas merged 1 commit intosymfony:3.3fromxabbuh:issue-22854

May 25, 2017

Conversation

Copy link

Member

Q	A
Branch?	3.3
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#22854
License	MIT
Doc PR

Choose a reason for hiding this comment

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

the!php/const tag is a bit special as it is separated with a colon from its value instead of a space which is used when parsing other tags

Copy link

Member

chalasr commentedMay 23, 2017•
edited
Loading

@xabbuh with this the YAML will appear to be valid but, in addition to thePARSE_CONSTANTS flag, the DI YamlFIleLoader usesYaml::PARSE_KEYS_AS_STRING for parsing. Actually those aren't compatible for keys

Copy link

MemberAuthor

xabbuh commentedMay 23, 2017

Do you have a concrete example where this breaks a real application? This combination sounds like a real edge case to me.

Copy link

Member

chalasr commentedMay 23, 2017•
edited
Loading

I mean that in 3.2, the following service:

foo:class:AppBundle\Fooarguments:        -!php/const:PHP_SAPI: bar

would have the following injected as first argument:

array('cli' =>'bar');

in 3.3, even with this, it would inject:

array(' !php/const:PHP_SAPI' =>'bar');

the DI file loader uses this edge combination since 3.3, so consts arent't resolved (the transition would be named!php/const:...)

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

May 24, 2017

Copy link

Contributor

tgalopin commentedMay 24, 2017

After solving the other problem described in the issue, I did encounter the problem explained by@chalasr:

                                                                                                                            [Symfony\Component\Workflow\Exception\InvalidArgumentException]                                                           The transition "!php/const:AppBundle\TonMacron\InvitationProcessor::TRANSITION_FILL_INFO" contains invalid characters.                                                                                                                            Exception trace: () at vendor/symfony/symfony/src/Symfony/Component/Workflow/Transition.php:34 Symfony\Component\Workflow\Transition->__construct() at n/a:n/a ReflectionClass->newInstanceArgs() at vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php:1078 Symfony\Component\DependencyInjection\ContainerBuilder->createService() at vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php:1195 Symfony\Component\DependencyInjection\ContainerBuilder->resolveServices() at vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php:1135 Symfony\Component\DependencyInjection\ContainerBuilder->resolveServices() at vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php:1135 Symfony\Component\DependencyInjection\ContainerBuilder->resolveServices() at vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php:1057 Symfony\Component\DependencyInjection\ContainerBuilder->createService() at vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php:583 Symfony\Component\DependencyInjection\ContainerBuilder->get() at vendor/symfony/symfony/src/Symfony/Component/Workflow/DependencyInjection/ValidateWorkflowsPass.php:47 Symfony\Component\Workflow\DependencyInjection\ValidateWorkflowsPass->process() at vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/Compiler/Compiler.php:143 Symfony\Component\DependencyInjection\Compiler\Compiler->compile() at vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php:736 Symfony\Component\DependencyInjection\ContainerBuilder->compile() at vendor/symfony/symfony/src/Symfony/Component/HttpKernel/Kernel.php:561 Symfony\Component\HttpKernel\Kernel->initializeContainer() at vendor/symfony/symfony/src/Symfony/Component/HttpKernel/Kernel.php:120 Symfony\Component\HttpKernel\Kernel->boot() at vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Console/Application.php:68 Symfony\Bundle\FrameworkBundle\Console\Application->doRun() at vendor/symfony/symfony/src/Symfony/Component/Console/Application.php:130 Symfony\Component\Console\Application->run() at bin/console:28

Copy link

MemberAuthor

xabbuh commentedMay 24, 2017

This is indeed an issue when thePARSE_CONSTANTandPARSE_KEYS_AS_STRINGS flags are used together. I have a failing test case locally and will look into a proper fix tonight.

xabbuh force-pushed theissue-22854 branch from5ea41c1 todb72005Compare

May 24, 2017 19:39

[Yaml] parse PHP constants in mapping keys

ae52fe6

xabbuh force-pushed theissue-22854 branch fromdb72005 toae52fe6Compare

May 24, 2017 19:40

Copy link

MemberAuthor

xabbuh commentedMay 24, 2017

constants should now be evaluated in mapping keys

@tgalopin Can you give this another try?

Status: Needs Review

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

May 24, 2017

chalasr approved these changes

May 25, 2017

View reviewed changes

Copy link

Member

chalasr left a comment

Choose a reason for hiding this comment

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

Works for me 👍

Copy link

Contributor

tgalopin commentedMay 25, 2017

Works for me too, thanks@xabbuh !

Copy link

MemberAuthor

xabbuh commentedMay 25, 2017

Thanks for the confirmation@chalasr and@tgalopin!

tgalopin mentioned this pull request

May 25, 2017

Upgrade to Symfony 3.3parti-renaissance/espace-adherent#1059

Merged

GuilhemN mentioned this pull request

May 25, 2017

[Yaml] Deprecate tags using colon#22913

Closed

GuilhemN reviewed

May 25, 2017

View reviewed changes

Copy link

Contributor

GuilhemN left a comment

Choose a reason for hiding this comment

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

👍, I opened#22913 to get rid of this fix in 4.0.

src/Symfony/Component/Yaml/Parser.php

		Inline::$parsedLineNumber =$this->getRealCurrentLineNb();
		$i =0;
		$key = Inline::parseScalar($values['key'],0,null,$i, !(Yaml::PARSE_KEYS_AS_STRINGS &$flags));
		$evaluateKey = !(Yaml::PARSE_KEYS_AS_STRINGS &$flags);

Copy link

Contributor

GuilhemNMay 25, 2017•
edited
Loading

Choose a reason for hiding this comment

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

I really don't get why we would ever want to use this flag, this is like saying "I don't want to respect the spec", don't you think we should just deprecate it?

Copy link

MemberAuthor

xabbuhMay 25, 2017

Choose a reason for hiding this comment

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

We need this to deprecate the previous behaviour. Otherwise we have no smooth upgrade path in 3.4.

Copy link

Contributor

GuilhemNMay 25, 2017

Choose a reason for hiding this comment

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

#21774 introduced a small bc break anyway and this only allows people to go back to the old behavior by using this flag, right?

Copy link

Member

nicolas-grekas commentedMay 25, 2017

Thank you@xabbuh.

nicolas-grekas merged commitae52fe6 intosymfony:3.3

May 25, 2017

nicolas-grekas added a commit that referenced this pull request

May 25, 2017

bug#22878[Yaml] parse PHP constants in mapping keys (xabbuh)

1eac150

This PR was merged into the 3.3 branch.Discussion----------[Yaml] parse PHP constants in mapping keys| Q             | A| ------------- | ---| Branch?       | 3.3| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#22854| License       | MIT| Doc PR        |Commits-------ae52fe6 [Yaml] parse PHP constants in mapping keys

xabbuh deleted the issue-22854 branch

May 25, 2017 13:40

fabpot mentioned this pull request

May 29, 2017

Release v3.3.0#22949

Merged

xabbuh added a commit that referenced this pull request

Aug 4, 2017

minor#22913[Yaml] Deprecate tags using colon (GuilhemN)

baa1e7f

This PR was squashed before being merged into the 3.4 branch (closes#22913).Discussion----------[Yaml] Deprecate tags using colon| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | no| New feature?  | no <!-- don't forget updating src/**/CHANGELOG.md files -->| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets || License       | MIT| Doc PR        |Using a colon in a tag doesn't look like yaml and causes trouble (see#22878), so I propose to just deprecate these tags in favor of more consistent tags.```yml- !php/const:PHP_INT_MAX- !php/object:O:30:"Symfony\Component\Yaml\Tests\A":1:{s:1:"a";s:3:"foo";}```would become```yml- !php/const PHP_INT_MAX- !php/object O:30:"Symfony\Component\Yaml\Tests\A":1:{s:1:"a";s:3:"foo";}```Commits-------9815af3 [Yaml] Deprecate tags using colon

symfony-splitter pushed a commit to symfony/yaml that referenced this pull request

Aug 4, 2017

minor #22913 [Yaml] Deprecate tags using colon (GuilhemN)

1395ddb

This PR was squashed before being merged into the 3.4 branch (closes #22913).Discussion----------[Yaml] Deprecate tags using colon| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | no| New feature?  | no <!-- don't forget updating src/**/CHANGELOG.md files -->| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets || License       | MIT| Doc PR        |Using a colon in a tag doesn't look like yaml and causes trouble (seesymfony/symfony#22878), so I propose to just deprecate these tags in favor of more consistent tags.```yml- !php/const:PHP_INT_MAX- !php/object:O:30:"Symfony\Component\Yaml\Tests\A":1:{s:1:"a";s:3:"foo";}```would become```yml- !php/const PHP_INT_MAX- !php/object O:30:"Symfony\Component\Yaml\Tests\A":1:{s:1:"a";s:3:"foo";}```Commits-------9815af3 [Yaml] Deprecate tags using colon

symfony-splitter pushed a commit to symfony/serializer that referenced this pull request

Aug 4, 2017

minor #22913 [Yaml] Deprecate tags using colon (GuilhemN)

71730ef

This PR was squashed before being merged into the 3.4 branch (closes #22913).Discussion----------[Yaml] Deprecate tags using colon| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | no| New feature?  | no <!-- don't forget updating src/**/CHANGELOG.md files -->| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets || License       | MIT| Doc PR        |Using a colon in a tag doesn't look like yaml and causes trouble (seesymfony/symfony#22878), so I propose to just deprecate these tags in favor of more consistent tags.```yml- !php/const:PHP_INT_MAX- !php/object:O:30:"Symfony\Component\Yaml\Tests\A":1:{s:1:"a";s:3:"foo";}```would become```yml- !php/const PHP_INT_MAX- !php/object O:30:"Symfony\Component\Yaml\Tests\A":1:{s:1:"a";s:3:"foo";}```Commits-------9815af3 [Yaml] Deprecate tags using colon

symfony-splitter pushed a commit to symfony/dependency-injection that referenced this pull request

Aug 4, 2017

minor #22913 [Yaml] Deprecate tags using colon (GuilhemN)

673d592

This PR was squashed before being merged into the 3.4 branch (closes #22913).Discussion----------[Yaml] Deprecate tags using colon| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | no| New feature?  | no <!-- don't forget updating src/**/CHANGELOG.md files -->| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets || License       | MIT| Doc PR        |Using a colon in a tag doesn't look like yaml and causes trouble (seesymfony/symfony#22878), so I propose to just deprecate these tags in favor of more consistent tags.```yml- !php/const:PHP_INT_MAX- !php/object:O:30:"Symfony\Component\Yaml\Tests\A":1:{s:1:"a";s:3:"foo";}```would become```yml- !php/const PHP_INT_MAX- !php/object O:30:"Symfony\Component\Yaml\Tests\A":1:{s:1:"a";s:3:"foo";}```Commits-------9815af3 [Yaml] Deprecate tags using colon

symfony-splitter pushed a commit to symfony/validator that referenced this pull request

Aug 4, 2017

minor #22913 [Yaml] Deprecate tags using colon (GuilhemN)

5cefd3f

This PR was squashed before being merged into the 3.4 branch (closes #22913).Discussion----------[Yaml] Deprecate tags using colon| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | no| New feature?  | no <!-- don't forget updating src/**/CHANGELOG.md files -->| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets || License       | MIT| Doc PR        |Using a colon in a tag doesn't look like yaml and causes trouble (seesymfony/symfony#22878), so I propose to just deprecate these tags in favor of more consistent tags.```yml- !php/const:PHP_INT_MAX- !php/object:O:30:"Symfony\Component\Yaml\Tests\A":1:{s:1:"a";s:3:"foo";}```would become```yml- !php/const PHP_INT_MAX- !php/object O:30:"Symfony\Component\Yaml\Tests\A":1:{s:1:"a";s:3:"foo";}```Commits-------9815af3 [Yaml] Deprecate tags using colon

xabbuh mentioned this pull request

Jan 13, 2020

[Yaml] Fail properly when the !php/const tag is used in a mapping key#35208

Closed

nicolas-grekas mentioned this pull request

Jun 24, 2022

[Yaml] Remove legacy parsing rule#46768

Merged

fabpot added a commit that referenced this pull request

Jun 25, 2022

minor#46768[Yaml] Remove legacy parsing rule (nicolas-grekas)

69c5859

This PR was merged into the 6.2 branch.Discussion----------[Yaml] Remove legacy parsing rule| Q             | A| ------------- | ---| Branch?       | 6.2| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       | -| License       | MIT| Doc PR        | -As far as I understood correctly, this part of the regexp has been introduced in#22878 to support a syntax that has been deprecated in#22913 and removed in 4.0.Commits-------5d7f9f2 [Yaml] Remove legacy parsing rule

Labels

Bug Status: Needs Review Yaml

		}
		}elseif (
		self::preg_match('#^(?P<key>'.Inline::REGEX_QUOTED_STRING.'\|(?:![^\s]++\s++)?[^\'"\[\{!].?) \:(\s++(?P<value>.+))?$#u',rtrim($this->currentLine),$values)
		self::preg_match('#^(?P<key>'.Inline::REGEX_QUOTED_STRING.'\|(?:!?!php/const:)?(?:![^\s]++\s++)?[^\'"\[\{!].?) \:(\s++(?P<value>.+))?$#u',rtrim($this->currentLine),$values)

Movatterモバイル変換

Uh oh!

[Yaml] parse PHP constants in mapping keys#22878

[Yaml] parse PHP constants in mapping keys#22878

Uh oh!

Conversation

xabbuh commentedMay 23, 2017

Uh oh!

xabbuhMay 23, 2017

Choose a reason for hiding this comment

Uh oh!

chalasr commentedMay 23, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

xabbuh commentedMay 23, 2017

Uh oh!

chalasr commentedMay 23, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

tgalopin commentedMay 24, 2017

Uh oh!

xabbuh commentedMay 24, 2017

Uh oh!

xabbuh commentedMay 24, 2017

Uh oh!

chalasr left a comment

Choose a reason for hiding this comment

Uh oh!

tgalopin commentedMay 25, 2017

Uh oh!

xabbuh commentedMay 25, 2017

Uh oh!

GuilhemN left a comment

Choose a reason for hiding this comment

Uh oh!

GuilhemNMay 25, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuhMay 25, 2017

Choose a reason for hiding this comment

Uh oh!

GuilhemNMay 25, 2017

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commentedMay 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

chalasr commentedMay 23, 2017•
edited
Loading

chalasr commentedMay 23, 2017•
edited
Loading

GuilhemNMay 25, 2017•
edited
Loading