[DependencyInjection] Handle ServiceClosureArgument for callable in container linting #35225
Conversation
| @@ -219,6 +220,10 @@ private function checkType(Definition $checkedDefinition, $value, \ReflectionPar | |||
| return; | |||
| } | |||
|
|
|||
| if ('callable' === $type && $value instanceof ServiceClosureArgument) { | |||
There was a problem hiding this comment.
Could you maybe add this case in the previous condition so we only have 1 if for callable?
There was a problem hiding this comment.
I thought about that, but also considered that it might make that clause unreadable. I'll happily add it in though if that's the consensus here.
There was a problem hiding this comment.
should we accept \Closure::class also btw?
There was a problem hiding this comment.
@shieldo I mean:
if ('callable' === $type) {
if (\is_array($value)/* etc. */) {
return;
}
if ($value instanceof ServiceClosureArgument) {
return;
}
}And that's just a suggestion ofc :)
There was a problem hiding this comment.
@nicolas-grekas I think that was done in the previous commit d6a7fbf unless I'm mistaken?
(Do you think I should follow the suggestion from @fancyweb above?)
There was a problem hiding this comment.
I mean:
if ('callable' === $type || 'Closure' === $type) && $value instanceof ServiceClosureArgument) {
(about the CS, I'm fine with the original proposal personally.)
There was a problem hiding this comment.
OK @fancyweb I've updated as per your suggestion - I think that's a pretty happy medium :)
There was a problem hiding this comment.
@nicolas-grekas I'd agree with that (and would also undo my previous change to make a distinct case in the code for that, as well as providing a new test). @fancyweb Make sense to you?
There was a problem hiding this comment.
Do it @shieldo, we indeed need to support the \Closure case here.
There was a problem hiding this comment.
Updated to handle a \Closure type hint
9f49d73 to
03cbbf5
Compare
src/Symfony/Component/DependencyInjection/Compiler/CheckTypeDeclarationsPass.php
Outdated
Show resolved
Hide resolved
03cbbf5 to
11e4fe1
Compare
11e4fe1 to
e48829e
Compare
|
Thank you @shieldo. |
…llable in container linting (shieldo) This PR was merged into the 4.4 branch. Discussion ---------- [DependencyInjection] Handle ServiceClosureArgument for callable in container linting | Q | A | ------------- | --- | Branch? | 4.4 (+) | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | (none) | License | MIT Making use of `ServiceClosureArgument` instances in service definitions was not accounted for in container linting when a service type-hints for `callable` in an argument - adding this check ensures that `ServiceClosureArgument` instances are recognised correctly as callables (once they are resolved). Commits ------- e48829e [DependencyInjection] Handle ServiceClosureArgument for callable in container linting
Making use of
ServiceClosureArgumentinstances in service definitions was not accounted for in container linting when a service type-hints forcallablein an argument - adding this check ensures thatServiceClosureArgumentinstances are recognised correctly as callables (once they are resolved).