Cleanup CallableResolver #2110
Cleanup CallableResolver #2110
Conversation
|
gotta add full test coverage |
ghost
closed this
|
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
ghost
reopened this
|
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]; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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? :)
There was a problem hiding this comment.
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!