search

slimphp / Slim

Slim is a PHP micro framework that helps you quickly write simple yet powerful web applications and APIs.
http://slimframework.com
MIT License
11.94k stars 1.95k forks source link

Resolution of 'class:method' string in Route #1081

Closed akrabat closed 9 years ago

akrabat commented 9 years ago

I've been looking at the resolution of a callable string of type 'class:method' which is in the Route class at https://github.com/slimphp/Slim/blob/develop/Slim/Route.php#L101-L118.

It would be nice if this resolution could look in the DI container to retrieve the class rather than instantiating it directly. However, getting an instance of Pimple here is non-trivial as it would have to be passed through the Router class which instantiates Route directly when it needs to.

Thinking some more, I'm wondering if the resolution of the string to a Closure should happen earlier in the process in App::mapRoute()?

Something like this: https://github.com/slimphp/Slim/compare/develop...akrabat:di-resolution-of-callable-string-3?expand=1

The main advantages are that we have access to the DI container in App and if the user wants a different strategy, they can simply extend App and override the resolveCallable() method.

Another option would be to delegate any resolution of the callable parameter to a extended App class and have the code Slim have no checks other than to ensure that $callable is callable.

Thoughts?

codeguy commented 9 years ago

Currently, it lazily resolves and instantiates callables. Moving this into the mapRoute method does give us the DI container, but it gives us additional overhead of instantiating callables that may never be used. It's a trade-off for sure.

codeguy commented 9 years ago

Personally, I like the benefits of having the DI container access vs. saving a few microseconds for performance. What do others think?

akrabat commented 9 years ago

My understanding of the current code is that we create & instantiate the callable even its not used as it's done in Router::setCallable(). Literally, all I've done in this branch is move it up the call chain and add the Pimple check within the closure.

codeguy commented 9 years ago

hmm ok. Guess I misunderstood. This seems fine! I'll merge in later this morning.