NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[DependencyInjection][FrameworkBundle] Fix using PHP 8.1 enum as parameters#44868

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

Jump to bottom

Merged

nicolas-grekas merged 0 commits intosymfony:4.4fromogizanagi:enum-di-parameters

Feb 4, 2022

Merged

[DependencyInjection][FrameworkBundle] Fix using PHP 8.1 enum as parameters#44868

nicolas-grekas merged 0 commits intosymfony:4.4fromogizanagi:enum-di-parameters

Feb 4, 2022

Conversation

Copy link

Contributor

ogizanagi commentedDec 30, 2021•
edited
Loading

Q	A
Branch?	4.4
Bug fix?	yesish
New feature?	no
Deprecations?	no
Tickets	Fix#44834
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:

namespaceApp;enum Status {case Draft;case Deleted;case Published;}

# services.yamlparameters:default_status:!php/const App\Status::Draft

The exception happens because thePhpDumper misses the leading slash:

    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,
theParameterBagInterface::get/set andContainerInterface::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)

carsonbot added Status: Needs Review Bug DependencyInjection labels

Dec 30, 2021

carsonbot added this to the4.4 milestone

Dec 30, 2021

ogizanagi added the RFCRFC = Request For Comments (proposals about features that you want to be discussed) label

Dec 30, 2021

Copy link

Member

derrabus commentedDec 30, 2021

Either way, allowing enums in parameters would be a new feature. I don't think we should add that functionality to 4.4.

Copy link

ContributorAuthor

ogizanagi commentedDec 31, 2021

I kind of agree, despite#40857 being on 4.4. But the difference is parameters never supported objects.
So let's make that clear by throwing in 4.4 and see what we can do/if we want to support enum in parameters later?

Copy link

Member

derrabus commentedDec 31, 2021

Sounds good. 🙂

Copy link

Member

nicolas-grekas commentedJan 12, 2022

How do we move forward on this one?

Copy link

ContributorAuthor

ogizanagi commentedJan 12, 2022

Sorry, didn't have time to go further. The plan was to throw either in theParameterBag or thePhpDumper so that's explicit enum types are not supported as parameters ; since it even goes against theParameterBagInterface::get/set contract.

ogizanagi removed the RFCRFC = Request For Comments (proposals about features that you want to be discussed) label

Jan 12, 2022

ogizanagi changed the title~~[DependencyInjection] Fix using PHP 8.1 enum as parameters~~[DependencyInjection] Throw on attempting to set enum parameters

Jan 26, 2022

ogizanagi force-pushed theenum-di-parameters branch 2 times, most recently from2a73c69 tod2bea6aCompare

January 26, 2022 09:58

Copy link

ContributorAuthor

ogizanagi commentedJan 26, 2022

Updated the PR so that we simply throw on attempting to set an enum or array containing an enum as a DI parameter : this should be enough for our goal: explicit the fact enums aren't supported as such.
Of course, the phpdoc already tells this, and we won't check for any other infringement to it ; but since the DI loaders allow such a thing indirectly:

# services.yamlparameters:default_status:!php/const App\Status::Draftstatuses:         -!php/constApp\Status::Draft        -!php/constApp\Status::Deleted        -!php/constApp\Status::Published

we might want to handle this specific case here so the user notices it.

Copy link

Member

nicolas-grekas commentedJan 28, 2022•
edited
Loading

I'm sorry I'm not super convinced by this. It would be strange to me to allow direct references to enums in service arguments, but to disallow indirect references via DI parameters:

parameters:foo:!php/const Some\Enum::Caseservices:bar:['%foo%']

I'd prefer to fix supporting them when dumping, and widen the types on 6.0. I don't think anybody is going to be hit by the BC break.

Copy link

ContributorAuthor

ogizanagi commentedJan 28, 2022

Honestly, I'd be way more satisfied this way too :)
I'll revert to the previous state of this PR.

ogizanagi force-pushed theenum-di-parameters branch fromd2bea6a toa3d95d6Compare

January 31, 2022 17:45

ogizanagi changed the title~~[DependencyInjection] Throw on attempting to set enum parameters~~[DependencyInjection] Fix using PHP 8.1 enum as parameters

Jan 31, 2022

ogizanagi added the FrameworkBundle label

Jan 31, 2022

carsonbot changed the title~~[DependencyInjection] Fix using PHP 8.1 enum as parameters~~[DependencyInjection][FrameworkBundle] Fix using PHP 8.1 enum as parameters

Jan 31, 2022

ogizanagi force-pushed theenum-di-parameters branch 3 times, most recently from5a18657 to4eaf136Compare

January 31, 2022 18:18

Copy link

ContributorAuthor