[DI] Service decoration: autowire the inner service#25631
[DI] Service decoration: autowire the inner service#25631dunglas wants to merge 2 commits intosymfony:masterfrom
Conversation
fdf91e0 to
8b75121
Compare
|
|
||
| if (null !== $innerIndex) { | ||
| // There is more than one argument of the type of the decorated class | ||
| return; |
There was a problem hiding this comment.
Maybe this case should result in an exception with helpful feedback for the developer. The developer obviously tried to autowire the decorator, so I think we should tell them why it fails.
There was a problem hiding this comment.
It would be a BC break: it currently tries to autowire these services "the regular way", if we throw an exception, it would not be the case anymore.
|
Good idea. I needed that feature recently. |
|
Oh this is a nice improvement! But what about the following?
Often enough I decorate because of logging or caching, which would still make me configure everything. In your commit it does seem to work though, or am I mistaken? https://github.com/symfony/symfony/pull/25631/files#diff-0cd60484959ffb82f3498fcc8bdcac11R21 |
|
@iltar I mean: // This is handled
class BarDecorator implements BarInterface
{
public function __construct(LoggerInterface $logger, BarInterface $decorated)
{
}
}
// This is ignored
class BarDecorator implements BarInterface
{
public function __construct(BarInterface $decorated, BarInterface $anotherBar)
{
}
} |
|
Ah okay, so I just misunderstood your sentence! In that case, big fat 👍 ! |
| } | ||
|
|
||
| if (null !== $innerIndex) { | ||
| $definition->setArgument($innerIndex, new Reference($renamedId)); |
|
I really like the idea. |
|
@nicolas-grekas, good idea I'll try that. The only difference I can see is this case: class BarDecorator implements BarInterface
{
public function __construct(BarInterface $decorated, BarInterface $anotherBar)
{
}
}With the current implementation, it's ignored (and I think it's the right thing to do), with bindings, the decorated service will be injected in both parameters. IMO it's an edge case, if we're fine with that, I'll update the code. |
|
In fact it's not possible to use bindings because they exact match the type. Here we need to use |
There was a problem hiding this comment.
Minor CS comments. This misses checking for @required setters, where other passes deal with it AFAIK. Should maybe be fixed.
| use Psr\Log\LoggerInterface; | ||
|
|
||
| /** | ||
| * @author Kévin Dunglas <dunglas@gmail.com> |
There was a problem hiding this comment.
Not sure about this line, that's a fixture :)
|
|
||
| private function autowire(ContainerBuilder $container, Definition $definition, string $renamedId): void | ||
| { | ||
| if ( |
There was a problem hiding this comment.
CS wise I think we prefer putting the next line on this one
| !$definition->isAutowired() || | ||
| null === ($innerClass = $container->findDefinition($renamedId)->getClass()) || | ||
| !($reflectionClass = $container->getReflectionClass($definition->getClass())) || | ||
| !$reflectionMethod = $reflectionClass->getConstructor() |
|
|
||
| $innerIndex = null; | ||
| foreach ($reflectionMethod->getParameters() as $index => $parameter) { | ||
| if ( |
Actually I omitted setters willingly. Setter injection is sometime legit, but for a decorator it's more than weird to inject the decorated service this way. I think we should only support constructors (if someone has this specific use case, he can still use manual wiring). |
aaddb17 to
3e06dd4
Compare
Seems arbitrary to me, and inconsistent, we should support setters everywhere IMHO. |
|
Ok I'll add support for setters, it cannot hurt if you don't use them :) |
|
@Tobion this PR changes nothing regarding service decoration. It will work as excepted. |
|
@chalasr I added support for renamed id in an easy but a bit hacky way. WDYT? |
|
@dunglas Looks good enough to me. |
|
@chalasr AFAIK, this PR doesn't change anything to this behavior. |
ead558c to
ecd14aa
Compare
|
Failures not related, ping @symfony/deciders. It would be great to have it in 4.1. |
There was a problem hiding this comment.
DX-wise this is fantastic! I love it ... but I left a minor comment.
| * | ||
| * Used to store the name of the renamed id when using service decoration together with autowiring | ||
| */ | ||
| public $previousRenamedId; |
There was a problem hiding this comment.
The name of this property is confusing to me: previousRenamedId "previous" and "renamed" together is confusing: is this the previous ID before the renaming? Is this the new renamed ID different from the previous one? Is this the first renamed ID and now we are doing the second renaming (so this is the "previous rename"? etc.
There was a problem hiding this comment.
I think it's a rest of the previous implementation (where renamedId was already used). is previousrenamedId ok to you?
There was a problem hiding this comment.
It depends on what this property really is (sorry, but its PHPdoc is confusing to me too). Here are some alternative proposals:
$decoratedServiceOriginalId$decoratedServiceNewId$decoratedServiceId$innerServiceId$decoratingServiceId- ...
There was a problem hiding this comment.
Done (I picked innerServiceId). Is it good for you guys?
5da2606 to
28bdc87
Compare
|
|
||
| * added support for variadics in named arguments | ||
| * added PSR-11 `ContainerBagInterface` and its `ContainerBag` implementation to access parameters as-a-service | ||
| * added support for autowiring to service's decorators |
There was a problem hiding this comment.
added support for service's decorators autowiring?
| /** | ||
| * @internal | ||
| * | ||
| * Used to store the name of the renamed id when using service decoration together with autowiring |
89ef67b to
9a6a596
Compare
|
Thank you @dunglas. |
…bbuh) This PR was merged into the 4.1-dev branch. Discussion ---------- [DependencyInjection] fix expected exception message | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Updates a test that was introduced in #25631 to match the changes made in #26595. Commits ------- 042eb4f fix expected exception message
This PR was merged into the 4.1 branch. Discussion ---------- [DI] Fix autowire inner service | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #25631 | License | MIT | Doc PR | - This PR fix multiple levels of decoration. Unfortunately, this [good question](#25631 (comment)) in origin PR has not been heard 🎧 😄. @dunglas @chalasr Commits ------- b79d097 [DI] Fix autowire inner service
This PR was merged into the 4.1 branch. Discussion ---------- [DI] Fix autowire inner service | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #25631 | License | MIT | Doc PR | - This PR fix multiple levels of decoration. Unfortunately, this [good question](symfony/symfony#25631 (comment)) in origin PR has not been heard 🎧 😄. @dunglas @chalasr Commits ------- b79d097c2a [DI] Fix autowire inner service
Try to automatically inject the decorated service.
Before:
After:
To trigger the autowiring, the following conditions must be met: