Skip to content
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][HttpKernel] Fix enum typed bindings #44838

Merged
merged 1 commit into from Dec 29, 2021

Conversation

ogizanagi
Copy link
Member

@ogizanagi ogizanagi commented Dec 29, 2021

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets N/A
License MIT
Doc PR symfony/symfony-docs#...

Relates to #44834

While:

# services.yaml
services:
    _defaults:
        autowire: true     
        autoconfigure: true
        bind:
            $myParam: !php/const App\Status::Deleted

is working for me, the following isn't:

# services.yaml
services:
    _defaults:
        autowire: true     
        autoconfigure: true
        bind:
-            $myParam: !php/const App\Status::Deleted
+            App\Status $myParam: !php/const App\Status::Deleted

->

Invalid value for binding key "App\Status $myParam" for service […]: expected "Symfony\Component\DependencyInjection\Reference", "Symfony\Component\DependencyInjection\Definition", "Symfony\Component\DependencyInjection\Argument\TaggedIteratorArgument", "Symfony\Component\DependencyInjection\Argument\ServiceLocatorArgument" or null, "App\Status" given.

This attempts to fix it by accounting for \UnitEnum in the ResolveBindingPass,
as well as re-allowing enum types already present in bindings after #44826.

@ogizanagi ogizanagi force-pushed the enum-bindings-with-type branch from 1d7b71e to c32d749 Compare Dec 29, 2021
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Dec 29, 2021

Thank you @ogizanagi.

@nicolas-grekas nicolas-grekas merged commit 30594b2 into symfony:4.4 Dec 29, 2021
9 of 11 checks passed
@ogizanagi ogizanagi deleted the enum-bindings-with-type branch Dec 29, 2021
@ismail1432
Copy link
Contributor

ismail1432 commented Dec 29, 2021

Thanks @ogizanagi

nicolas-grekas added a commit that referenced this pull request Feb 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/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](https://user-images.githubusercontent.com/2211145/147762495-f57f1fff-f56a-4e87-bbc4-3bba97a8e81b.png)

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants