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] addAutowire parameter attribute#45657

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

Merged
fabpot merged 1 commit intosymfony:6.1fromkbond:autowire-attribute
Mar 18, 2022

Conversation

@kbond
Copy link
Member

@kbondkbond commentedMar 6, 2022•
edited
Loading

QA
Branch?6.1
Bug fix?no
New feature?yes
Deprecations?no
Ticketsn/a
LicenseMIT
Doc PRtodo

Replaces#45573 &#44780 with a single newAutowire attribute:

class MyService{publicfunction__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{publicfunctionsomeAction(        #[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 {}}

GromNaN, fancyweb, BafS, bresam, and ging-dev reacted with thumbs up emojiruudk, fancyweb, GromNaN, and ging-dev reacted with heart emoji
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment•
edited
Loading

Choose a reason for hiding this comment

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

What about callingYaml::parse() before callingYamlFileLoader::resolveServices()?

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedMar 7, 2022•
edited
Loading

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, derrabus, and yceruto reacted with thumbs up emoji

@kbond
Copy link
MemberAuthor

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

What about controller action arguments? Separate PR?

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.

What about controller action arguments? Separate PR?

same PR to me, it's the same topic

kbond reacted with thumbs up emoji
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.

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

@kbondkbondforce-pushed theautowire-attribute branch 2 times, most recently from735a6ee tod3a1794CompareMarch 7, 2022 16:34
@kbond
Copy link
MemberAuthor

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:

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

@nicolas-grekas
Copy link
Member

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.

@kbondkbondforce-pushed theautowire-attribute branch 2 times, most recently from45d49dc toac210cfCompareMarch 7, 2022 18:12
@kbond
Copy link
MemberAuthor

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

@kbondkbondforce-pushed theautowire-attribute branch 2 times, most recently from63094c4 tod9b0417CompareMarch 8, 2022 19:01
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment•
edited
Loading

Choose a reason for hiding this comment

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

I like it, thanks!
(I pushedsome tweaks)

kbond reacted with thumbs up emoji
@fabpot
Copy link
Member

Thank you@kbond.

@fabpotfabpot merged commit7248e16 intosymfony:6.1Mar 18, 2022
@kbondkbond deleted the autowire-attribute branchMarch 18, 2022 13:54
fabpot added a commit that referenced this pull requestMar 22, 2022
…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        | todoPer 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):```phpclass 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
@fabpotfabpot mentioned this pull requestApr 15, 2022
wouterj added a commit to symfony/symfony-docs that referenced this pull requestMay 1, 2022
…attribute (kbond)This PR was merged into the 6.1 branch.Discussion----------[DependencyInjection][HttpKernel] document `Autowire` attributeSymfony PRs:symfony/symfony#45657 &symfony/symfony#45783Closes#16625.Closes#16636Commits-------bf8be7e Move the autowire attribute sectionsf32aec1 [DI][HttpKernel] document `Autowire` attribute
*
* @author Kevin Bond <kevinbond@gmail.com>
*/
#[\Attribute(\Attribute::TARGET_PARAMETER)]
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

@kbondkbondMay 31, 2022•
edited
Loading

Choose a reason for hiding this comment

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

qdequippe reacted with thumbs up emoji

Choose a reason for hiding this comment

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

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

qdequippe and derrabus reacted with thumbs up emoji
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Seehttps://3v4l.org/gDeYA what I mean.

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

cc@kbond@nicolas-grekas@qdequippe

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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. ;)

Copy link
Member

@nicolas-grekasnicolas-grekasJun 20, 2022•
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

@derrabusderrabusJun 21, 2022•
edited
Loading

Choose a reason for hiding this comment

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

Seehttps://3v4l.org/gDeYA what I mean.

CallingnewInstance() 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 callnewInstance() 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.

nicolas-grekas and jvasseur 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.

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

GromNaN added a commit to GromNaN/symfony that referenced this pull requestMar 28, 2025
Introduced insymfony#45657symfony/dependency-injection 6.4+ is required, so the class always exists
GromNaN added a commit to GromNaN/symfony that referenced this pull requestMar 28, 2025
Introduced insymfony#45657symfony/dependency-injection 6.4+ is required, so the class always exists
GromNaN added a commit that referenced this pull requestMar 28, 2025
…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       | MITThis condition was introduced in#45657The 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
symfony-splitter pushed a commit to symfony/http-kernel that referenced this pull requestMar 28, 2025
Introduced insymfony/symfony#45657symfony/dependency-injection 6.4+ is required, so the class always exists
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@derrabusderrabusderrabus left review comments

@fabpotfabpotfabpot approved these changes

@GromNaNGromNaNGromNaN approved these changes

@chalasrchalasrchalasr approved these changes

@dunglasdunglasAwaiting requested review from dunglasdunglas is a code owner

+2 more reviewers

@qdequippeqdequippeqdequippe left review comments

@michaljusiegamichaljusiegamichaljusiega left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

6.1

Development

Successfully merging this pull request may close these issues.

9 participants

@kbond@nicolas-grekas@fabpot@GromNaN@derrabus@qdequippe@chalasr@michaljusiega@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp