Refactored CallableResolver into a stateless service focused on resolving callables only #1448
Conversation
…ving callables only This will lead to less object instantiations (since the service is now stateless instead of wrapping every other callable), as well as better separation of concerns since the class' responsibility is now only to resolve callables. It isn't responsible of invoking callables anymore. One detail is that I have moved the `$callable->bindTo($container)` calls into the CallableResolver itself since it belongs there (because it's about preparing callables for being invokable correctly). That avoids having "callable resolution" code all over the place (App and Route).
| @@ -153,6 +154,8 @@ public function testAddMiddlewareAsString() | |||
| $route = $this->routeFactory(); | |||
|
|
|||
| $container = new Container(); | |||
| $container['MiddlewareStub'] = new MiddlewareStub(); | |||
|
|
|||
There was a problem hiding this comment.
Could someone explain to me how it was working before? I'm afraid I introduced a regression because now I have to register the middleware in the container (and it's not necessary on 3.x).
There was a problem hiding this comment.
It worked before because the CallableResolver was never really invoked, so it did not try to find the MiddlewareStub.
There was a problem hiding this comment.
OK so that's an improvement then ;)
|
Also the next step IMO is to remove the |
| { | ||
| $this->toResolve = $toResolve; | ||
| } | ||
|
|
||
| /** | ||
| * Resolve toResolve into a closure that that the router can dispatch. |
There was a problem hiding this comment.
The new signature of the method is public function resolve($toResolve) so it makes sense right?
|
Maybe a final check should happen in Currently, anything you pass that is not a string or callable will be returned without modification. And since the Also I agree on getting rid of the One problem with your current implementation though! Closures will never be bound to the container since it wont get to the Apart from that I think this is an improvement :) Makes much more sense for the |
Even if a route or middleware is callable, it needs to go through the CallableResolver so that closures are re-bound to the container.
|
Thanks for the review!
Done in ba08a05, an exception will be thrown if the CallableResolver cannot return a real callable.
Oh yeah good catch, I fixed it in e87d996. I also added a test for that to make sure it's staying. |
|
@ppetermann, good question ... It didn't change in this PR but was already bound to container instead of app. No sure where and why it was changed. All given examples in de docs assume closures are indeed bound to app. Edit; found the confusion: Middleware is bound to the container, route callable bound to app. So this PR should keep that same behaviour. Currently this PR will break this behaviour. But in the So in the end route callable will be found to container instead of app, which if wrong. Edit2: |
|
|
silentworks
added
pending response
…ending on the case)
|
Thank you for noticing that. I've restored the way it was before regarding binding Closures to app or container. |
|
Looks good to me. |
akrabat
removed
the
pending response
|
Thanks @mnapoli |
Per discussions on IRC, I refactored CallableResolver into a stateless service focused on resolving callables only (not wrapping and invoking them).
This will lead to less object instantiations (since the service is now stateless instead of wrapping every middleware/controller callable), as well as better separation of concerns since the class' responsibility is now only to resolve callables. It isn't responsible of invoking callables anymore (responsibility which was spread between here and also the invokation strategy and middleware stack too IIRC).
One detail is that I have moved the
$callable->bindTo($container)calls into the CallableResolver itself since it belongs there IMO (because it's about preparing callables for being invokable correctly). That avoids having "callable resolution" code all over the place (App and Route).Feedback welcome. I'll probably be able to answer and update the PR in 2 days.