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] Support for parameter injection#20738

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

Closed

Conversation

@dunglas
Copy link
Member

@dunglasdunglas commentedDec 3, 2016
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketsn/a
LicenseMIT
Doc PRn/a

It's basically the same idea than request's attributes injection as controller's parameters: if a service is autowired and a container's parameter match, this parameter is injected. Example:

# app/config/services.ymlparameters:hello:SymfonyConservices:'Foo\Action\Homepage':class:'Foo\Action\Homepage'autowire:true
// src/Foo/Action/Homepage.phpnamespaceFoo\Action;class Homepage{publicfunction__construct($hello/*, Acme $anOtherService*/)    {// $hello value is 'SymfonyCon'    }}

ro0NL and TomasVotruba reacted with thumbs up emojihason, robfrawley, jvasseur, pamil, Koc, chalasr, COil, dewos, and sc0rp10 reacted with thumbs down emojiogizanagi reacted with confused emoji
@kasparsklavins
Copy link

How does this deal with snake_case parameter names?

@theofidry
Copy link
Contributor

There may be some interests and advantages to it, but I would at least add a configuration parameter besides just settingautowire: true, otherwise there is way too much side effect IMO.

robfrawley and TomasVotruba reacted with thumbs up emoji

@dunglas
Copy link
MemberAuthor

@kasparsklavins it doesn't deal with it, just like for request attributes, if your parameter isfoo_bar, the variable must be$foo_bar (and you actually doesn't want to do that, just usefooBar as parameter name).

chalasr reacted with confused emoji

@ogizanagi
Copy link
Contributor

ogizanagi commentedDec 3, 2016
edited
Loading

I may not be the most objective person to argue about this, because I don't like much autowiring (it's quite opinionated, I know), but I wonder if this isn't going too far with this 😅 .
In practice, you'll often have to deal with common arguments names that may conflicts with each others, and the only solutions would be (in order to avoid collisions):

  1. Design your services with that in mind, and name constructors/methods' arguments against a well-identified container parameter.
  2. Not use parameter injection.

Considering this, my own preference clearly is to not deal with parameters injection at all 😄

robfrawley, jvasseur, and chalasr reacted with thumbs up emoji

@dunglas
Copy link
MemberAuthor

@ogizanagi your concerns are justified and 1) should definitely be avoided. However, I don't think that it's so common to have several arguments/parameters with the same name and in such case (as for all "complex" cases), the user should note use autowiring at all and switch to the classic definition.

I guess that conventions will change and that the common usage when using autowiring will become something like:

namespacePaymentClient;class PayPalClient{publicfunction__construct($PayPalApiKey) {}// instead of// public function __construct($apiKey) {}}
namespacePaymentClient;class StripeClient{publicfunction__construct($stripeApiKey) {}// instead of// public function __construct($apiKey) {}}
# app/config/config.ymlparameters:payPalApiKey:azestripeApiKey:rty

If (and only if), services are build automatically (like when using ActionBundle) with 0 config, I think it worths it.
Keep in mind that targets are basically:

  1. Newcomers and junior developers
  2. Small apps, prototypes, first iterations or projects

Thanks to this set of features, it will be very easy to create (and refactor) services, without having to know about the container (the service container becomes a framework internal and will never be used directly by the user): you just have to create the class and it works, Symfony wires everything for you.
I hope it will encourage newcomers to actually create services and that we'll see less huge controllers containing all the business logic (and 0 services).

And after all, it's just a parameter name, it's not part of the public API of the class, it's an internal. When the application starts to mature or to grow, you can still disable autowiring and rename the parameter without breaking anything.

I predict that we'll soon have a command to "write" the definition of an autowired service to transform it in a standard (not autowired) service. It will help too.

@robfrawley
Copy link
Contributor

Keep in mind that targets are basically:

  • Newcomers and junior developers
  • Small apps, prototypes, first iterations or projects

Thanks to this set of features, it will be very easy to create (and refactor) services, without having to know about the container (the service container becomes a framework internal and will never be used directly by the user): you just have to create the class and it works, Symfony wires everything for you.
I hope it will encourage newcomers to actually create services and that we'll see less huge controllers containing all the business logic (and 0 services).

I know the intention of all the recent "magic" functionality is to improve DX and lessen the learning curve to help newcomers.This general goal is great. Don't get me wrong, I love the idea of easing the on-boarding for new developers, but...I don't agree that adding more "magic" and hiding away anything complex will necessarily accomplishes this goal. In fact, I think in some ways, it is detrimental to it.

[...] without having to know about the container

No! Please, no!Don't remove the need to understand the container! Doing do will result in new developers who are blindly ignorant as to the true flexibility and power that is available to them (thanks to the amazing architecture of this framework).

Honestly, in the years I've been using Symfony, one of the most profound moments --- the one that I believe propelled me from an inexperienced Symfony developer to a knowledgeable, skilled one --- was the day that all the basic building blocks Symfony is comprised of finally "clicked." It wasn't until after this experience that I would consider any of my projects with this framework as "good". And it wasn't until after this experience that I truly hadfun creating Symfony projects.

Trying to shield developers from some of the most significant features of Symfony is not how we should go about attracting new ones, IMHO. These are the same aspects and features that have continued to make Symfonythe most compelling choice as a web framework over the last few years. We want to make sure peopledo have to learn about these things. Until they do, they will only be scratching the serfice of what is possible in Symfony.

When I first started using Symfony, sometime around 2011 with the introduction of v2.0 (never used 1.0), the documentation was solid and only getting better. By the time I was using Symfony in a large project in 2012, the documentation was extensive. Now, since then, the documentation has only continued to improve and develop into, what I consider, one of the most extensive, rich, and full-featured reference manuals for any open source framework---one that even bests a large majority of commercial frameworks!

This is the most important piece to keep current and improve upon to help newcomers. (IMHO) Performing "magic" actions for the user is always sketchy in my book, regardless of the intent, so I don't like this to begin with, likely skewing my reaction to this PR, but I simply feel like we should work toward better real-life examples that show proper usage of complex components instead of try to make the framework do everything for the user. I don't wan to allow newcomers to ignore important components and concepts to their own workflow determent.


As for this PR in general, I'd love to see it written in such a way as to incorporate scalar type hinting (when available) and then fallback to the simple variable name matching currently implemented. It would be great if it knew not to pass a matching parameter/variable if the parameter type doesn't match the expected variable type.

Also, I feelvery strongly that this should complement the currentautowire: true configuration option as its own, or possibly as an amendment to it, versus having this functionality blindly turn on for those already utalizing the autowire functionality. Perhaps changingautowire to allow for the expected values offalse andtrue (which continue to operate exactly the same as prior) while also allowing a new value type forautowire as an array of features to enable likeautowire: [services, parameters] or something. I'm fairly certain the configuration parser can be written in such a way to preserve the prior boolean choice while allowing for more complex values moving forward, which wouldn't constitute a BC break, right? Anyway, my 2 cents...

chalasr, pamil, deltab, and marcverney reacted with thumbs up emoji

@ro0NL
Copy link
Contributor

ro0NL commentedDec 4, 2016
edited
Loading

I kinda like it :)) but it should go after#20167 imo. So it only applies to white listed methods. Parameter/variable naming is less an issue then, as it couldnt be autowired otherwise anyway.

Foo $foo, $bar
@foo, %bar%

Makes sense to me.

@dunglas
Copy link
MemberAuthor

I agree with@theofidry and@ro0NL, this feature should be optional. It should be possible to use service autowiring and parameter autowiring separately. I propose the following config:

# app/config/services.ymlparameters:hello:SymfonyConservices:'Foo\Action\Homepage':autowire:true# Default to constructor autowiring + parameter autowiring# Alternativeautowire:methods:[__construct, set*]parameters:false

And indeed,#20167 must be merged first.

robfrawley and theofidry reacted with thumbs up emoji

@dunglas
Copy link
MemberAuthor

dunglas commentedDec 4, 2016
edited
Loading

By introducing this newautowire key, we can also provide a better alternative toautowiring_types, closer of how Laravel works:

services:'MyLogger':autowire:bind:Psr\Log\LoggerInterfacebind:MyCustomInterface

@ro0NL
Copy link
Contributor

ro0NL commentedDec 4, 2016
edited
Loading

I think addingparameters: true|false makes it unnecessarily complex.

methodName(Foo $foo, $bar)

If you autowiremethodName it makes sense$bar is autowired as well (ie.%bar%). Not sure what would happen otherwise... skip the method silently? crash?

@dunglas
Copy link
MemberAuthor

@ro0NL, the default value iftrue, so - by default -, it behaves as you describe, while letting the users disabling this feature if he doesn't like, want to allow this.

@theofidry
Copy link
Contributor

theofidry commentedDec 5, 2016
edited
Loading

@dunglas about the last suggestion what about:

services:'MyLogger':autowire:bindings:                 -Psr\Log\LoggerInterface                -MyCustomInterface

?

dunglas reacted with thumbs up emoji

@nicolas-grekasnicolas-grekas added this to the3.x milestoneDec 6, 2016
fabpot added a commit that referenced this pull requestFeb 13, 2017
…(dunglas, nicolas-grekas)This PR was merged into the 3.3-dev branch.Discussion----------[DependencyInjection] Add support for named arguments| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | todoThis PR introduces named arguments for services definitions. It's especially useful to inject parameters in an autowired service. It is (at least partially) an alternative to#21376 and#20738.Usage:```ymlservices:    _defaults: { autowire: true }    Acme\NewsletterManager: { $apiKey: "%mandrill_api_key%" }# Alternative (traditional) syntaxservices:    newsletter_manager:        class: Acme\NewsletterManager        arguments:            $apiKey: "%mandrill_api_key%"        autowire: true``````phpuse Doctrine\ORM\EntityManager;use Psr\Log\LoggerInterface;namespace Acme;class NewsletterManager{    private $logger;    private $em;    private $apiKey;    public function __construct(LoggerInterface $logger, EntityManager $em, $apiKey)    {        $this->logger = $logger;        $this->em = $em;        $this->apiKey = $apiKey;    }}```Commits-------8a126c8 [DI] Deprecate string keys in arguments2ce36a6 [DependencyInjection] Add a new pass to check arguments validity6e50129 [DependencyInjection] Add support for named arguments
@nicolas-grekasnicolas-grekas modified the milestones:3.x,3.3Mar 24, 2017
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

3.3

Development

Successfully merging this pull request may close these issues.

8 participants

@dunglas@kasparsklavins@theofidry@ogizanagi@robfrawley@ro0NL@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp