Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Conversation

@alexandre-daubois
Copy link
Member

@alexandre-dauboisalexandre-daubois commentedApr 18, 2021
edited by nicolas-grekas
Loading

QA
Branch?4.4
Bug fix?yes (new PHP version compatibility)
New feature?no
Deprecations?no
TicketsFix#40233
LicenseMIT
Doc PR(see below)

Added support of enums using!php/const tag, as they work the same way.

ostrolucky reacted with thumbs up emoji
@carsonbot
Copy link

Hey!

I think@atailouloute has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@alexandre-dauboisalexandre-dauboisforce-pushed thefeat-dependency-injection-enum-support branch 2 times, most recently from2ff81ae tod3e9060CompareApril 19, 2021 11:16
@derrabusderrabus added this to the5.x milestoneApr 19, 2021
@alexandre-dauboisalexandre-dauboisforce-pushed thefeat-dependency-injection-enum-support branch fromd3e9060 toc4adfe1CompareApril 19, 2021 14:30
@alexandre-dauboisalexandre-daubois changed the base branch from5.4 to5.3June 4, 2021 06:34
@alexandre-dauboisalexandre-daubois changed the title[DependencyInjection] Add support of PHP 8.1 enumerations[DependencyInjection] Add support of PHP enumerationsJun 4, 2021
@alexandre-dauboisalexandre-dauboisforce-pushed thefeat-dependency-injection-enum-support branch from6abf811 to40ababdCompareJune 4, 2021 06:48
@stof
Copy link
Member

stof 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:
Copy link
Member

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

derrabus reacted with thumbs up emoji
Copy link
MemberAuthor

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:
Copy link
Member

@derrabusderrabusJun 6, 2021
edited
Loading

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?

Copy link
MemberAuthor

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"

@alexandre-dauboisalexandre-dauboisforce-pushed thefeat-dependency-injection-enum-support branch from40ababd to5248a43CompareJune 7, 2021 10:57
@alexandre-dauboisalexandre-dauboisforce-pushed thefeat-dependency-injection-enum-support branch from1a36bef to1c43829CompareJune 22, 2021 06:36
@alexandre-dauboisalexandre-daubois changed the base branch from5.3 to4.4June 22, 2021 06:36
@alexandre-dauboisalexandre-dauboisforce-pushed thefeat-dependency-injection-enum-support branch 2 times, most recently from8bb428a tobb4eb0dCompareJune 22, 2021 16:15
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

Last nitpicking :)

alexandre-daubois reacted with eyes emoji
@alexandre-dauboisalexandre-dauboisforce-pushed thefeat-dependency-injection-enum-support branch frombb4eb0d to8f29ff3CompareJune 22, 2021 18:06
@nicolas-grekasnicolas-grekasforce-pushed thefeat-dependency-injection-enum-support branch from8f29ff3 to88c69c0CompareJune 23, 2021 19:07
@nicolas-grekas
Copy link
Member

Thank you@alexandre-daubois.

alexandre-daubois reacted with thumbs up emoji

@nicolas-grekasnicolas-grekas merged commitef06f33 intosymfony:4.4Jun 23, 2021
@alexandre-dauboisalexandre-daubois deleted the feat-dependency-injection-enum-support branchJune 23, 2021 21:48
This was referencedJun 30, 2021
nicolas-grekas added a commit that referenced this pull requestFeb 4, 2022
…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):![image](https://user-images.githubusercontent.com/2211145/147762495-f57f1fff-f56a-4e87-bbc4-3bba97a8e81b.png)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
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@xabbuhxabbuhxabbuh left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@stofstofstof approved these changes

@derrabusderrabusAwaiting requested review from derrabus

+1 more reviewer

@stloydstloydstloyd left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

[DI] Support PHP 8.1 enums when dumping the container

7 participants

@alexandre-daubois@carsonbot@stof@nicolas-grekas@stloyd@derrabus@xabbuh

[8]ページ先頭

©2009-2025 Movatter.jp