-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DependencyInjection] Add support of PHP enumerations #40857
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
[DependencyInjection] Add support of PHP enumerations #40857
Conversation
78c44fb
to
9d8fa01
Compare
Hey! I think @atailouloute has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/DependencyInjection/Dumper/YamlDumper.php
Outdated
Show resolved
Hide resolved
2ff81ae
to
d3e9060
Compare
src/Symfony/Component/DependencyInjection/Tests/Loader/XmlFileLoaderTest.php
Outdated
Show resolved
Hide resolved
d3e9060
to
c4adfe1
Compare
6abf811
to
40ababd
Compare
@nicolas-grekas @fabpot should this be merged in 4.4 as it is about supporting a new PHP version ? |
@@ -127,6 +127,8 @@ public static function dump($value, int $flags = 0): string | |||
return self::dumpNull($flags); | |||
case $value instanceof \DateTimeInterface: | |||
return $value->format('c'); | |||
case $value instanceof \UnitEnum: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we add new things in the Yaml component, I'd really appreciate to add some tests there covering this kind of feature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added one here. Not sure if there's another one to add? This one should be enough if I'm right.
@@ -127,6 +127,8 @@ public static function dump($value, int $flags = 0): string | |||
return self::dumpNull($flags); | |||
case $value instanceof \DateTimeInterface: | |||
return $value->format('c'); | |||
case $value instanceof \UnitEnum: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to adjust the DI component's composer.json?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, the current composer.json
seems good with "^4.4|^5.0"
40ababd
to
5248a43
Compare
5248a43
to
1a36bef
Compare
1a36bef
to
1c43829
Compare
8bb428a
to
bb4eb0d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last nitpicking :)
bb4eb0d
to
8f29ff3
Compare
8f29ff3
to
88c69c0
Compare
Thank you @alexandre-daubois. |
…num as parameters (ogizanagi) This PR was squashed before being merged into the 4.4 branch. Discussion ---------- [DependencyInjection][FrameworkBundle] Fix using PHP 8.1 enum as parameters | Q | A | ------------- | --- | Branch? | 4.4 <!-- see below --> | Bug fix? | yesish | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tickets | Fix #44834 <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead --> | License | MIT | Doc PR | N/A While #40857 allowed using enums in DI, it does not allow using these in parameters. This would be the actual fix for #44834, which shows the error you'll get trying to use enum as DI parameters (appart from the binding issue fixed in #44838):  given: ```php namespace App; enum Status { case Draft; case Deleted; case Published; } ``` ```yaml # services.yaml parameters: default_status: !php/const App\Status::Draft ``` The exception happens because the `PhpDumper` misses the leading slash: ```diff protected function getDefaultParameters(): array { return [ - 'default_status' => App\Status::Draft, + 'default_status' => \App\Status::Draft, ]; } ``` While this would fix using enums as DI parameters as of Symfony < 6, the `ParameterBagInterface::get/set` and `ContainerInterface::setParameter/getParameter` are not allowing `\UnitEnum` and adding these in phpdoc is an issue since actual typehints and return types were added as of Symfony 6. => So this PR is a BC break, but hopefully nobody will be hit by it 🤞🏻 (poke `@ismail1432` - https://twitter.com/SmaineDev/status/1476237072826613763?s=20) Commits ------- ac36617 [DependencyInjection][FrameworkBundle] Fix using PHP 8.1 enum as parameters
Added support of enums using
!php/const
tag, as they work the same way.