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] parse PHP constants in mapping keys #22878

Merged
merged 1 commit into from May 25, 2017

Conversation

Projects
None yet
6 participants
@xabbuh
Member

xabbuh commented May 23, 2017

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
@@ -208,7 +208,7 @@ private function doParse($value, $flags)
$this->refs[$isRef] = end($data);
}
} 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)

This comment has been minimized.

@xabbuh

xabbuh May 23, 2017

Member

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

@xabbuh

xabbuh May 23, 2017

Member

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

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr May 23, 2017

Member

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

Member

chalasr commented May 23, 2017

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

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh May 23, 2017

Member

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

Member

xabbuh commented May 23, 2017

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

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr May 23, 2017

Member

I mean that in 3.2, the following service:

foo:
    class: AppBundle\Foo
    arguments:
        -
            !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:...)

Member

chalasr commented May 23, 2017

I mean that in 3.2, the following service:

foo:
    class: AppBundle\Foo
    arguments:
        -
            !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:...)

@tgalopin

This comment has been minimized.

Show comment
Hide comment
@tgalopin

tgalopin May 24, 2017

Contributor

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
Contributor

tgalopin commented May 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
@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh May 24, 2017

Member

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

Member

xabbuh commented May 24, 2017

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

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh May 24, 2017

Member

constants should now be evaluated in mapping keys

@tgalopin Can you give this another try?

Status: Needs Review

Member

xabbuh commented May 24, 2017

constants should now be evaluated in mapping keys

@tgalopin Can you give this another try?

Status: Needs Review

@chalasr

Works for me 👍

@tgalopin

This comment has been minimized.

Show comment
Hide comment
@tgalopin

tgalopin May 25, 2017

Contributor

Works for me too, thanks @xabbuh !

Contributor

tgalopin commented May 25, 2017

Works for me too, thanks @xabbuh !

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh May 25, 2017

Member

Thanks for the confirmation @chalasr and @tgalopin!

Member

xabbuh commented May 25, 2017

Thanks for the confirmation @chalasr and @tgalopin!

@GuilhemN

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

@@ -221,7 +221,14 @@ private function doParse($value, $flags)
try {
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);

This comment has been minimized.

@GuilhemN

GuilhemN May 25, 2017

Contributor

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?

@GuilhemN

GuilhemN May 25, 2017

Contributor

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?

This comment has been minimized.

@xabbuh

xabbuh May 25, 2017

Member

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

@xabbuh

xabbuh May 25, 2017

Member

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

This comment has been minimized.

@GuilhemN

GuilhemN May 25, 2017

Contributor

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

@GuilhemN

GuilhemN May 25, 2017

Contributor

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

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas May 25, 2017

Member

Thank you @xabbuh.

Member

nicolas-grekas commented May 25, 2017

Thank you @xabbuh.

@nicolas-grekas nicolas-grekas merged commit ae52fe6 into symfony:3.3 May 25, 2017

2 of 3 checks passed

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

nicolas-grekas added a commit that referenced this pull request May 25, 2017

bug #22878 [Yaml] parse PHP constants in mapping keys (xabbuh)
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 xabbuh deleted the xabbuh:issue-22854 branch May 25, 2017

@fabpot fabpot referenced this pull request May 29, 2017

Merged

Release v3.3.0 #22949

xabbuh added a commit that referenced this pull request Aug 4, 2017

minor #22913 [Yaml] Deprecate tags using colon (GuilhemN)
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)
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 symfony/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)
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 symfony/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)
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 symfony/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)
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 symfony/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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment