Override callback resolver #10
Conversation
|
Thank you for the PR! Could you also add a functional test in this class to cover the behavior you described in #9? i.e. that something like this works: $app->get('/{name}', HelloName::class)
->convert('req', HelloNameConverter::class);I have a few other comments that I'll write inline the diff. |
|
|
||
| /** | ||
| * Class CallbackResolver | ||
| * @package DI\Bridge\Silex |
There was a problem hiding this comment.
No need for the @package annotation which is covered by the PHP namespace. Also could you update the description to explain why this class exists? e.g. This alternative resolver uses the generic Invoker to support PHP-DI's extended callable syntax
|
I don't have much time right now to check, do you know if the
Anyway I'm looking forward to merge and release this! |
|
Silex use |
|
👍 awesome |
|
ready for review :) |
| $this['callback_resolver'] = $this->share(function () { | ||
| return new CallbackResolver( | ||
| $this, | ||
| new CallableResolver($this->containerInteropProxy) |
There was a problem hiding this comment.
Could the callable resolver from line 54 be reused here? I don't think there's a need to create it twice.
There was a problem hiding this comment.
yep, sure should I register callable resolver in DI first?
There was a problem hiding this comment.
should I register callable resolver in DI first?
yep sounds good. Maybe name it something like phpdi.callable_resolver to avoid crowding the root "namespace" for users?
|
I'm going to take a deeper look when I'm at home this evening. |
|
|
||
| /** | ||
| * @param string|callable $name | ||
| * @return callable |
There was a problem hiding this comment.
Don't know whats the convention for return type hints for callables but it should be consistent with convertCallback(), I think.
There was a problem hiding this comment.
convertCallback always return callable whatever its form, but the old API only hinted as a callable array. Since we use PHPDI callable resolver return type is not limited to callable array, so I hinted callable as return type.
|
Some small nits and style consistency thingies. Otherwise it's fine, I think :) |
| // return immediately if $name is callable | ||
| if (is_callable($name)) { | ||
| return $name; | ||
| } |
There was a problem hiding this comment.
I'm afraid this might be an issue as method calls are considered as statically callable. For example ['Logger', 'warning'] is considered callable by PHP even though it's not a static method, so calling Logger::warning() would be wrong and the "correct" callable would be [$logger, 'warning']. Have a look at this line: https://github.com/PHP-DI/Invoker/blob/master/src/CallableResolver.php#L69
Anyway all that to say that this shortcut will create issues with some types of callables.
|
That was fast ;) That looks all good to me! @jdreesen? |
|
👍 from me :) |
|
👍 thanks @Atriedes |
|
I'll create a release tonight with a documentation update |
this PR will solve issue #9