Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[DependencyInjection] Add support of PHP enumerations#40857
Uh oh!
There was an error while loading.Please reload this page.
Conversation
78c44fb to9d8fa01Comparecarsonbot commentedApr 19, 2021
Hey! I think@atailouloute has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
2ff81ae tod3e9060Comparesrc/Symfony/Component/DependencyInjection/Tests/Loader/XmlFileLoaderTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
d3e9060 toc4adfe1Compare6abf811 to40ababdComparestof commentedJun 4, 2021
@nicolas-grekas@fabpot should this be merged in 4.4 as it is about supporting a new PHP version ? |
| returnself::dumpNull($flags); | ||
| case$valueinstanceof \DateTimeInterface: | ||
| return$value->format('c'); | ||
| case$valueinstanceof \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 onehere. Not sure if there's another one to add? This one should be enough if I'm right.
| returnself::dumpNull($flags); | ||
| case$valueinstanceof \DateTimeInterface: | ||
| return$value->format('c'); | ||
| case$valueinstanceof \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 currentcomposer.json seems good with"^4.4|^5.0"
40ababd to5248a43Compare5248a43 to1a36befCompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
1a36bef to1c43829Compare8bb428a tobb4eb0dCompare
nicolas-grekas left a comment
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 :)
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
bb4eb0d to8f29ff3Compare8f29ff3 to88c69c0Comparenicolas-grekas commentedJun 23, 2021
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/AWhile#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:```phpnamespace App;enum Status { case Draft; case Deleted; case Published;}``````yaml# services.yamlparameters: 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
Uh oh!
There was an error while loading.Please reload this page.
Added support of enums using
!php/consttag, as they work the same way.