8000 [DependencyInjection][FrameworkBundle] Fix using PHP 8.1 enum as parameters by ogizanagi · Pull Request #44868 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 0 commits into from
Feb 4, 2022
Merged

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

merged 0 commits into from
Feb 4, 2022

Conversation

ogizanagi
Copy link
Contributor
@ogizanagi ogizanagi commented Dec 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):

image

given:

namespace App;

enum Status {
    case Draft;
    case Deleted;
    case Published;
}
# services.yaml
parameters:
    default_status: !php/const App\Status::Draft

The exception happens because the PhpDumper 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,
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)

@carsonbot carsonbot added this to the 4.4 milestone Dec 30, 2021
@ogizanagi ogizanagi added the RFC RFC = Request For Comments (proposals about features that you want to be discussed) label Dec 30, 2021
@derrabus
Copy link
Member

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

@ogizanagi
Copy link
Contributor Author

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?

@derrabus
Copy link
Member

Sounds good. 🙂

@nicolas-grekas
Copy link
Member

How do we move forward on this one?

@ogizanagi
Copy link
Contributor Author

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

@ogizanagi ogizanagi removed the RFC RFC = Request For Comments (proposals about features that you want to be discussed) label Jan 12, 2022
@ogizanagi 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 ogizanagi force-pushed the enum-di-parameters branch 2 times, most recently from 2a73c69 to d2bea6a Compare January 26, 2022 09:58
@ogizanagi
Copy link
Contributor Author

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.yaml
parameters:
    default_status: !php/const App\Status::Draft
    statuses: 
        - !php/const App\Status::Draft
        - !php/const App\Status::Deleted
        - !php/const App\Status::Published

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

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Jan 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::Case

services:
    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.

@ogizanagi
Copy link
Contributor Author

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

@ogizanagi ogizanagi changed the title [DependencyInjection] Throw on attempting to set enum parameters [DependencyInjection] Fix using PHP 8.1 enum as parameters Jan 31, 2022
@carsonbot 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 ogizanagi force-pushed the enum-di-parameters branch 3 times, most recently from 5a18657 to 4eaf136 Compare January 31, 2022 18:18
@ogizanagi
Copy link
Contributor Author

Alright, PR updated. Hopefully I haven't missed anything.

@ogizanagi ogizanagi force-pushed the enum-di-parameters branch 4 times, most recently from f0acb77 to da3f323 Compare February 1, 2022 12:56
nicolas-grekas
nicolas-grekas reviewed Feb 1, 2022
View reviewed changes
@fabpot
Copy link
Member
fabpot commented Feb 4, 2022

We stopped adding new features for supporting newer PHP versions a while ago, so this is a new feature that should go into 6.1.

@nicolas-grekas
Copy link
Member

This is edgy, but after a private chat with @fabpot, I've got a go for merging. Now doing it.

@nicolas-grekas
Copy link
Member

Thank you @ogizanagi.

@nicolas-grekas nicolas-grekas merged commit c2e179e into symfony:4.4 Feb 4, 2022
@ogizanagi ogizanagi deleted the enum-di-parameters branch February 4, 2022 14:25
This was referenced Feb 28, 2022
Merged
Merged
Merged
@BafS BafS mentioned this pull request Mar 10, 2022
Closed
@fancyweb fancyweb mentioned this pull request Feb 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@derrabus derrabus derrabus approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
Bug DependencyInjection FrameworkBundle Status: Reviewed
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
6 participants
@ogizanagi @derrabus @nicolas-grekas @fabpot @chalasr @carsonbot
0