[DependencyInjection] add `Autowire` parameter attribute #45657

kbond · 2022-03-06T13:03:58Z

Q	A
Branch?	6.1
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	n/a
License	MIT
Doc PR	todo

Replaces #45573 & #44780 with a single new Autowire attribute:

class MyService
{
    public function __construct(
        #[Autowire(service: 'some_service')]
        private $service1,

        #[Autowire(expression: 'service("App\\Mail\\MailerConfiguration").getMailerMethod()')
        private $service2,

        #[Autowire(value: '%env(json:file:resolve:AUTH_FILE)%')]
        private $parameter1,

        #[Autowire(value: '%kernel.project_dir%/config/dir')]
        private $parameter2,
    ) {}
}

Works with controller arguments as well:

class MyController
{
    public function someAction(
        #[Autowire(service: 'some_service')]
        $service1,

        #[Autowire(expression: 'service("App\\Mail\\MailerConfiguration").getMailerMethod()')
        $service2,

        #[Autowire(value: '%env(json:file:resolve:AUTH_FILE)%')]
        $parameter1,

        #[Autowire(value: '%kernel.project_dir%/config/dir')]
        $parameter2,
    ): Response {}
}

nicolas-grekas

~~What about calling Yaml::parse() before calling YamlFileLoader::resolveServices()?~~

src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php

nicolas-grekas · 2022-03-07T08:20:49Z

What about calling Yaml::parse() before calling YamlFileLoader::resolveServices()?

I'm withdrawing this comment, bad idea :)

Another idea, using named arguments:

#[Autowire(service: MailerConfiguration::class)
#[Autowire(expr: 'service("App\Mail\MailerConfiguration").getMailerMethod()')
#[Autowire(value: '%kernel.project_dir%'/var/foo')

I think that's my preference: no custom yaml-like strings and more explicit than standalone attributes.

kbond · 2022-03-07T13:06:53Z

I've made this change and also added the raw parameter argument (updated PR description).

What about controller action arguments? Separate PR?

nicolas-grekas

What about controller action arguments? Separate PR?

same PR to me, it's the same topic

src/Symfony/Component/DependencyInjection/Attribute/Autowire.php

src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php

nicolas-grekas

Here are some minor comments. I like it. How about you? Let's close the two previous ones?

src/Symfony/Component/DependencyInjection/Attribute/Autowire.php

kbond · 2022-03-07T16:38:35Z

Here are some minor comments. I like it. How about you? Let's close the two previous ones?

Made these changes. I am happy with it as well - let's close them!

Few questions:

Should I change the service Reference behaviour for nullable parameters?
For controller arguments: are only service's allowed? No expressions/parameters?

nicolas-grekas · 2022-03-07T16:54:32Z

Should I change the service Reference behaviour for nullable parameters?

That'd make sense yes

For controller arguments: are only service's allowed? No expressions/parameters?

expressions/parameters should be allowed. There is a hack using 'current' as factory right now, you'll see it in the pass.

src/Symfony/Component/DependencyInjection/Attribute/Autowire.php

src/Symfony/Component/DependencyInjection/Tests/Fixtures/includes/autowiring_classes_80.php

src/Symfony/Component/DependencyInjection/Attribute/Autowire.php

kbond · 2022-03-07T18:14:29Z

Ok, I have the controller argument support working but implementation might need some work.

src/Symfony/Component/HttpKernel/DependencyInjection/RegisterControllerArgumentLocatorsPass.php

...omponent/HttpKernel/Tests/DependencyInjection/RegisterControllerArgumentLocatorsPassTest.php

src/Symfony/Component/HttpKernel/DependencyInjection/RegisterControllerArgumentLocatorsPass.php

nicolas-grekas

I like it, thanks!
(I pushed some tweaks)

fabpot · 2022-03-18T06:51:56Z

Thank you @kbond.

@nicolas-grekas

…ementation (kbond) This PR was merged into the 6.1 branch. Discussion ---------- [DependencyInjection] adjust `Autowire` attribute implementation | Q | A | ------------- | --- | Branch? | 6.1 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | #45657 (comment) | License | MIT | Doc PR | todo Per discussion with @nicolas-grekas, we've decided to improve the DX of the attribute a bit. The implementation from #45657 still works as described there. This allows for the first `Autowire` constructor argument to be used for services/expressions by adding a _familiar_ prefix (`@` for services, `@=` for expressions): ```php class MyService { public function __construct( #[Autowire('@some_service')] private $service1, #[Autowire('@=service("App\\Mail\\MailerConfiguration").getMailerMethod()') private $service2, #[Autowire('%env(json:file:resolve:AUTH_FILE)%')] private $parameter1, ) {} } ``` Commits ------- c86aa7a [DependencyInjection] adjust `Autowire` attribute implementation

…attribute (kbond) This PR was merged into the 6.1 branch. Discussion ---------- [DependencyInjection][HttpKernel] document `Autowire` attribute Symfony PRs: symfony/symfony#45657 & symfony/symfony#45783 Closes #16625. Closes #16636 Commits ------- bf8be7e Move the autowire attribute sections f32aec1 [DI][HttpKernel] document `Autowire` attribute

qdequippe · 2022-05-31T14:53:26Z

src/Symfony/Component/DependencyInjection/Attribute/Autowire.php

+ *
+ * @author Kevin Bond <kevinbond@gmail.com>
+ */
+#[\Attribute(\Attribute::TARGET_PARAMETER)]


Do you think this #[\Attribute(\Attribute::TARGET_PARAMETER, \Attribute::TARGET_PROPERTY)] can be used here? When using Attribute with promoted property constructor.

#[\Attribute(\Attribute::TARGET_PARAMETER] can be used on promoted properties. See https://github.com/symfony/symfony/pull/45657/files#diff-a4f7e7528218aa6d151b1e68834393c6ae958540ec31ffa49ce3207998e77dd2R33

why would we? TARGET_PARAMETER is enough to target a promoted argument

Yes you're right TARGET_PARAMETER is enough here. Thank you for your quick response :)

I'm facing the same problem.

TARGET_PARAMETER is working as well with properties for constructor, however not working especially with reflection mechanism on reflection property.

See https://3v4l.org/gDeYA what I mean.

Also PHPStorm suggests not to use this attribute on property promoted:

cc @kbond @nicolas-grekas @qdequippe

Could this be a bug in PHP or a missing detail in the implementation? The opposite is a problem too: https://3v4l.org/tBjj2. Somewhat related PHPStan issue: phpstan/phpstan#4418.

As far as I understand, PHPStan literally "silence" this situation, right?

For me isn't looks like a PHP bug, in my opinion we should add TARGET_PROPERTY flag. ;)

To me it's a limitation of the engine and the engine should be improved: from a descriptive pov, both variants give all the possible hints to make things clear. The engine is not clever enough yet.
The alternative is to not use CPP but it would be a shame to accept this state of things :) Patching the attribute would be wrong: Autowire should NOT be allowed on properties.
Could you please open a bug report about this on php-src?

See https://3v4l.org/gDeYA what I mean.

Calling newInstance() for arbitrary attributes is a footgun. Attributes could point to undefined classes and the attribute constructor could have side effects. The reflection API by design provides you with enough information to decide whether you need an actual instance of an attribute. Only call newInstance() on attributes you know and that your code actually needs to work with.

I agree with @nicolas-grekas that there's nothing we should do on our side.

I agree as well. At least until a time where the attribute can actually be used with property injection.

Introduced in symfony#45657 symfony/dependency-injection 6.4+ is required, so the class always exists

…f DI `Autowire` class (GromNaN) This PR was merged into the 7.3 branch. Discussion ---------- [HttpKernel] Remove always-true condition on existence of DI `Autowire` class | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | - | License | MIT This condition was introduced in #45657 The class `RegisterControllerArgumentLocatorsPass` requires the DI component, which cannot be < 6.4 due to a [conflict rule in composer.json](https://github.com/symfony/symfony/blob/79ea49c772ce4b39f414cde5648ad347c3bbcfd7/src/Symfony/Component/HttpKernel/composer.json#L33). So the `Autoconfigure` class always exists. Commits ------- 3975043 Remove always-true condition

Introduced in symfony/symfony#45657 symfony/dependency-injection 6.4+ is required, so the class always exists

kbond requested a review from dunglas as a code owner March 6, 2022 13:03

carsonbot added Status: Needs Review DependencyInjection Feature labels Mar 6, 2022

carsonbot added this to the 6.1 milestone Mar 6, 2022

kbond force-pushed the autowire-attribute branch from 2824eb9 to fc249f1 Compare March 6, 2022 13:06

kbond requested a review from nicolas-grekas March 6, 2022 13:07

nicolas-grekas reviewed Mar 6, 2022

View reviewed changes

src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php Outdated Show resolved Hide resolved

kbond force-pushed the autowire-attribute branch from fc249f1 to 2fa653b Compare March 7, 2022 12:57