Cleanup CallableResolver #2110
Cleanup CallableResolver #2110
Conversation
|
gotta add full test coverage |
|
You can simply add to this branch and as you push, it'll re-run the CI tests |
|
@akrabat yes thanks I know. Just wanted to keep the PR list clean, as it will take some time, before I can work on it. I will reopen it soon |
…s://github.com/0x13a/Slim into feature-refactor-callable-resolver
|
The un-covered line here it seems to be like a dead code, and moreover it's not appearing on my local phpunit 4.8.33 running with PHP7 7.0.14, where I've 100% code coverage for the Slim Callable Resolver. Any ideas ? |
| protected function resolveCallable($class, $method = '__invoke') | ||
| { | ||
| if ($this->container->has($class)) { | ||
| return [$this->container->get($class), $method]; |
akrabat
Feb 6, 2017
Member
This changes the behaviour from the current code.
At the moment, when there isn't a method specified, we return the class object and let PHP resolve to __invoke. This code explicitly returns an array with the second parameter as "__invoke".
Are there any implications as a result of this change?
This changes the behaviour from the current code.
At the moment, when there isn't a method specified, we return the class object and let PHP resolve to __invoke. This code explicitly returns an array with the second parameter as "__invoke".
Are there any implications as a result of this change?
ghost
Feb 7, 2017
Author
Hey,
I don’t think it’s going to affect the behaviour, since as we must return a valid callback, when a method is not provided, the class must be invokable.
Given that, we return a callable defined as array which has at index 0 the class and index 1 the method to be invoked, which we explicitly define to be __invoke’ https://3v4l.org/Uvj29
I’m going to test it deeply, but I’d say wouldn’t be an issue, isn’t it? :)
Hey,
I don’t think it’s going to affect the behaviour, since as we must return a valid callback, when a method is not provided, the class must be invokable.
Given that, we return a callable defined as array which has at index 0 the class and index 1 the method to be invoked, which we explicitly define to be __invoke’ https://3v4l.org/Uvj29
I’m going to test it deeply, but I’d say wouldn’t be an issue, isn’t it? :)
akrabat
Feb 7, 2017
Member
Double check performance too.
Double check performance too.
ghost
Feb 7, 2017
Author
do you have any benchmark suite ?
do you have any benchmark suite ?
akrabat
Feb 7, 2017
Member
I've asked around and general view is that it won't make any difference.
I've asked around and general view is that it won't make any difference.
I can't see why that closing |
|
Thanks. |
Hi There,
I was taking a look at the Callable Resolver and I've noticed how the indentation levels were nested and all coupled into one single method. I've also noticed how some checks and logic were
intrinsically structured.
Moreover the callable pattern, even though is not used anywhere else I think it's better to have it as a constant, since it's never going to change.
I've run the unit tests. Hope it's opened for discussions, glad to have your positive or negative feedback, thanks!