[DependencyInjection] Add autowiring capabilities #15613
Conversation
| */ | ||
| class AutowiringPass implements CompilerPassInterface | ||
| { | ||
| private $container; |
There was a problem hiding this comment.
why storing the container? Can't you just pass it explicitly to your private methods?
There was a problem hiding this comment.
As this class already has a lot of states, it's just to avoid passing the container as argument of other methods.
I can rewrite this class without states but not sure it has any interest (and other passes also have states).
|
There is a problem with invalid definitions intended to be removed later by removing passes. It's why tests of FrameworkBundle fail: A solution is to explicitly mark with a tag definitions to autowire or to not autowire. IMO marking definitions to not autowire is more user friendly (it's an edge case) but it would be a (small) BC break. Bundles using black magic similar to what is done in Another possibility is to silently restore the initial definition if the autowiring fail. I think it would be less user friendly (no way to detect if the autowiring failed or not) but this approach is both BC and doesn't require to add specific tags. I suggest to require to mark services to autowire in 2.8 (BC) and to change the behavior in 3.0 by requiring to mark the service to not autowire (as it is just a tag, it's still possible to have a definition working for both 2.8 and 3.0 by adding the |
| { | ||
| $container = new ContainerBuilder(); | ||
|
|
||
| $container->register('coop_tilleuls', __NAMESPACE__.'\LesTilleuls'); |
There was a problem hiding this comment.
I hope you understand this comment right: I think that using the real name of your company in the tests is "not right". I'd prefer to keep using synthetic names for everything (acme, foo, bar, etc.)
|
This introduces a lot of magic and complexity for developers.
I can certainly see benefits for this such as If I understand it right, it will guess the dependency based on the class name. What if someone typehints an interface and decorates the dependency, will it create a reference to the decorated or decorator? Both are in the DIC and the I personally like verbosity. Writing down a service definition is a minute of work, but you won't have to really bother with it anymore. I do a lot of refactoring because development never stops. One thing I always do, is check if a service is used somewhere, which works better than checking against an interface. This will be virtually impossible because you lost control of where it's injected. It's a nice feature that certainly has potential, but at the moment I see far more downsides than benefits. Maybe you can open my eyes! |
It maybe introduces some "magic" but no complexity at all: this feature is fully optional and developers that don't like it can define their services as they have always done. IMO this feature simplify the use of the DI in the context of RAD (I'll get back to it).
I'm not sure to understand exactly what you mean: class A {}
class B { function __construct(A $a) {} }
class C { function __construct(A $a) {} }services:
b:
class: B
c:
class: CIt works without breaking anything. abstract class A {}
class B extends A {}
class C extends A {}
class Foo { function __construct(A $a) {} }services:
foo:
class: FooThis is not possible to guess what to inject so it throws an explicit There is two solutions here: Using the classical (explicit) definition: services:
b:
class: B
c:
class: C
foo:
class: Foo
arguments: [ @b ]It's the same old config available from the beginning of Symfony. An alternative is to define a default service to use for a given type (it will always be injected for that type unless an explicit service is passed in argument). services:
b:
class: B
types: [ A ]
c:
class: C
foo:
class: FooThis is useful when a service must be injected 90% of the time (it can be autowired) but in some definitions another implementation (decorator, debug or performance optimized version...) is needed.
Errors are raised at compile time (when
Basically, the autowiring system just complete service definitions during a compiler pass. After that, it's the same old system that is used. The impact in term of stability is minor and if an error occurs, it will occur during the container compilation and will be handled by the developper very early in the development process.
I'm not sure I understand what you mean. Can you give an example?
The system is smarter than just using the class name. I've posted some examples using interfaces in the PR description. To sum up:
As previously said, the current behavior is not deprecated and can still be used. It's a layer on top of what is already existing that is fully optional. Autowiring is a feature present in many DI systems (Spring, Laravel DI, PHP DI...). It's main advantage is to drastically reduce the configuration code needed to use DI (in many case, it allows to write no configuration all) and ease refactoring (when you add or remove a dependency to a class, you don't need to dive into all your service definitions to update the configuration, this is automatic). Personally, I always avoid using such things (autowiring, configuration annotations, even the DIC) for a large scale enterprise where maintainability matter and I know the code base will be huge. I prefer to be explicit and have chunks of code as independent and framework agnostic as possible. However I also often work on smaller projects where the goals are different: development as speed as possible with less line of code as possible. For such projects (prototypes, some micro services, weekend projects and some non-commercial projects...) I rely as much as possible on every feature that ease the devolvement and the refactoring including conventions, annotations and all that kind of "magic" (when you know how it works, there is not much magic). Autowiring is for that kind of projects. I hope it answers to your concerns. The summary:
|
|
@dunglas I agree 100% with the idea of this PR and I thank you for working on it. Regarding the "magic fear" expressed by @iltar, I have one question:
My question is: for those people who don't want this feature, how can they know that their configuration is wrong but it was "silently" fixed by the autowiring feature? |
|
@ITAR: autowiring only solves the simplest use cases, for the rest you can still configure explicitly the services. Now Symfony already has some history with auto wiring, but it didn't make it in the end: https://groups.google.com/forum/m/#!topic/symfony-devs/77GxWI8F8lU |
|
As this is mainly based on typehinting against implementations, which is a big no-go for services. I'm afraid this is actually promoting to typehint against an implementation instead of the interface, resulting in almost useless services... |
|
@javiereguiluz It's similar to what I've suggested here #15613 (comment). What to you think of the tag based approach? |
|
The qiestion @javiereguiluz is asking, is one of the things that concerns me as well, hen 8000 ce:
class A {}
class FooBar {
public function __construct(A $a);
}services:
a.foo:
class: A
a.bar:
class: A
foobar:
class: FooBar
class A {
public function __construct(RequestStack $rs, AB $a, BB $b);
}services:
ab.henk:
class: AB
ab.jan:
class: AB
bb:
class: BB
a.foo:
class A:
arguments:
# how do I wire the request stack in here?
- `@ab.henk` # cannot be autowired, 2x AB defined
# BB is autowired?
I agree with what WouterJ said. This is also what happens with the JMSDiExtraBundle, which is the exact reason I avoid it. Maybe this could be a RapidDevelopmentBundle though? It seems like very pluggable functionality, similar to the |
|
I have only one question, similar to the one raised by @javiereguiluz : for some reasons, in a project I don't want to use auto-wiring at all, I want to completely defines all my services. But, sometimes, I failed and I forget some things. How can I completely disable auto-wiring in order to see my mistakes? |
|
@iltar it's exactly the example I've given: class A {}
class FooBar {
public function __construct(A $a);
}services:
a.foo:
class: A
a.bar:
class: A
foobar:
class: FooBarYou'll get the following error (at compile time): For such cases:
For your second example, you can of course fallback to the explicit definition, but you can also use the following syntax (your snippet edited): services:
ab.henk:
class: AB
ab.jan:
class: AB
bb:
class: BB
a.foo:
class A:
arguments:
- '' # As it is empty, the autowiring will be triggered for request stack. Looks better in XML (I prefer XML...): <argument/>
- `@ab.henk` # cannot be autowired, 2x AB defined => explicit definition is OK
# Yes, BB will be autowired |
|
@Nicofuma in #15613 (comment) I've proposed the following:
It's not implemented yet and feedback will be very appreciated :) |
|
@dunglas I start liking XML as well (for opensource). But the recommendation and easy going way for your app is still yml. This means you still have to register the argument (or omit it).
What if I would want to add a 4th argument or my extension/compiler pass calls The biggest problem is that if this is enabled, it will lead to weird constructions in service definitions and headaches for the developers. It doesn't feel intuitive at all to me. I can already see questions coming to |
|
@dunglas that error is exactly what @javiereguiluz and @iltar mean. assume you had this config: services:
a.foo:
class: A
foobar:
class: FooBarEverything works nice! Now, one of your coworkers (or yourself) add a new service somewhere in a config file: services:
a.bar: { class: A }Whoa, foobar service throws an exception now and the application breaks! (only because I added an, apperently totally unrelated, a.bar service). |
|
The tag could solve some issue. But I think we still need a global flag (I wouldn't have to add a tag to each of my xxx services) |
|
Rebased, it looks OK now. |
|
|
||
| if (isset($service['autowiring_types'])) { | ||
| if (!is_array($service['autowiring_types'])) { | ||
| throw new InvalidArgumentException(sprintf('Parameter "autowiring_types" must be an array for service "%s" in %s. Check your YAML syntax.', $id, $file)); |
There was a problem hiding this comment.
As having only one type is quite common, I would allow a single string to be passed here.
|
|
||
| $arguments = $definition->getArguments(); | ||
| foreach ($constructor->getParameters() as $index => $parameter) { | ||
| if (!$typeHint = $parameter->getClass()) { |
There was a problem hiding this comment.
the case of non-existent classes still need to be handled
|
@stof should be OK now |
| throw new RuntimeException(sprintf('Unable to autowire argument of type "%s" for the service "%s".', $typeHint->name, $id)); | ||
| } | ||
|
|
||
| $argumentId = sprintf('autowired.%s', $typeHint->name); |
There was a problem hiding this comment.
This will register a service with an id like 'autowired.AppBundle\Foo' which is not consistent with Service Naming Conventions
There was a problem hiding this comment.
You're right but it is only intended for internal services name (they should not be used directly) so it should not be a big deal.
From a debug point of view, autowired.AppBundle\Foo is more explicit than autowired.app_bundle_foo or autowired.app_bundle.foo.
IMO it's better to keep it as is but I can change it if everyone think it's better to follow conventions here.
|
Thank you @dunglas. |
This PR was squashed before being merged into the 2.8 branch (closes #6032). Discussion ---------- [DependencyInjection] Autowiring doc | Q | A | ------------- | --- | Doc fix? | no | New docs? | yes symfony/symfony#15613 | Applies to | 2.8, 3.0 | Fixed tickets | n/a Commits ------- 995bd4f [DependencyInjection] Autowiring doc
This PR adds autowiring capabilities to the Dependency Injection component. It eases service registration by letting the component guessing dependencies to inject and even (under certain conditions) registering them using typehints of the constructor parameters.
The following usages are supported:
Automatic dependency registration
It will register
Fooas a private service (autowired.foo) and injects it as the first argument of thebarconstructor.This method works only for typehints corresponding to instantiable classes (interfaces and abstract classes are not supported).
Autocompletion of definition arguments
The autowiring system will find types of all services and completes constructor arguments of the
les_tilleulsservice definition using typehints.It works only if there is one service registered for a given type (if there are several services available for the same type and no explicit type definition, a
RuntimeExceptionis thrown).Explicit type definition
When a service is explicitly associated with a type, it is always used to fill a definition depending of this type, even if several services have this type. If several services are associated with the same type, the last definition takes the priority.
Of course explicit definitions are still supported.
YAML, XML and PHP loaders have been updated to supports the new
typeparameter.