[DependencyInjection] Fixed resolving of service configurators containing Definition objects #14835
Conversation
| Q | A |
|---|---|
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #14834 |
| License | MIT |
| Doc PR | - |
| if (is_array($callable)) { | ||
| $callable[0] = $callable[0] instanceof Reference ? $this->get((string) $callable[0]) : $parameterBag->resolveValue($callable[0]); | ||
| } | ||
| $callable = $this->resolveServices($parameterBag->resolveValue($callable)); |
There was a problem hiding this comment.
this now allows expressions too, which looks wrong to me
There was a problem hiding this comment.
why not be consistent with the rest?
There was a problem hiding this comment.
Expressions are not supported in configurators in other places. They are only supported in arguments. So you don't introduce consistency but inconsistency (and using an expression for the service instance of the callable does not make sense either btw)
There was a problem hiding this comment.
Alright. What solution do you propose then? Duplicate resolveServices() and remove expression support from the duplicate? Any naming suggestions?
There was a problem hiding this comment.
well, we don't need to support the case of arrays either for configurators first element. So this is just a matter of adding the support of Definition object in the current code (which already supports references)
There was a problem hiding this comment.
Yes, but that means we still have duplicated and potentially buggy code. If somebody adds functionality to resolveServices(), they need to think about changing this code too (like happened before). IMO this should be fixed.
…ning Definition objects
|
I updated the PR to match @stof's comments. I don't like this solution however, because the duplicated code in |
|
👍 |
|
Thank you @webmozart. |
…tors containing Definition objects (webmozart) This PR was merged into the 2.7 branch. Discussion ---------- [DependencyInjection] Fixed resolving of service configurators containing Definition objects | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #14834 | License | MIT | Doc PR | - Commits ------- 6ebcddd [DependencyInjection] Fixed resolving of service configurators containing Definition objects