[Yaml] parse PHP constants in mapping keys #22878
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 |
xabbuh
commented
May 23, 2017
| @@ -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) | |||
There was a problem hiding this comment.
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 with this the YAML will appear to be valid but, in addition to the |
|
Do you have a concrete example where this breaks a real application? This combination sounds like a real edge case to me. |
|
I mean that in 3.2, the following service: foo:
class: AppBundle\Foo
arguments:
-
!php/const:PHP_SAPI: barwould 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 |
|
After solving the other problem described in the issue, I did encounter the problem explained by @chalasr: |
|
This is indeed an issue when the |
|
constants should now be evaluated in mapping keys @tgalopin Can you give this another try? Status: Needs Review |
|
Works for me too, thanks @xabbuh ! |
GuilhemN
reviewed
May 25, 2017
| @@ -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); | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
We need this to deprecate the previous behaviour. Otherwise we have no smooth upgrade path in 3.4.
There was a problem hiding this comment.
#21774 introduced a small bc break anyway and this only allows people to go back to the old behavior by using this flag, right?
|
Thank you @xabbuh. |
nicolas-grekas
added a commit
that referenced
this pull request
May 25, 2017
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
Merged
xabbuh
added a commit
that referenced
this pull request
Aug 4, 2017
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
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
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
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
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
fabpot
added a commit
that referenced
this pull request
Jun 25, 2022
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.