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.98k stars 1.95k forks source link

Controller callables parameters #1186

Closed mnapoli closed 9 years ago

mnapoli commented 9 years ago

First I am sorry if this topic has already been discussed, I did a search but couldn't find something about it.

In Slim 3 it seems that controllers are supposed to be middlewares, i.e. have the following signature:

$app->get('/hello/{name}', function ($request, $response, $args) {
    // use $args['name'];
});

Is that required by PSR-7? I don't see the benefit of such a constraint, except maybe consistency with actual middlewares. But I think it's less interesting than, for example, how Silex or Lumen can work:

$app->get('/hello/{name}', function ($name) {
    // use $name;
});

This example above is much more explicit and simple. I would suggest to keep something like this for Slim 3.

Yet in order to leave the liberty to users to use the request, response and any parameter, it would also be good to allow any kind of combination like this:

$app->get('/hello/{name}', function ($response, $request, $name) {
    // use $name;
    // use $request to check a header (for example)
    // return the response
});

To do something like this I have been working on Invoker and I have used it for example in Silly (CLI micro-framework). I also plan to use it in PHP-DI 5. Slim could reuse the Invoker library (I would be OK to move the repository to the Slim organisation for example) or reuse the idea.

That library also provides a way to do have dependency injection in 2 different ways while staying decoupled from a container:

See also how dependency injection works in Silly using those features: http://mnapoli.fr/silly/docs/dependency-injection.html

opengeek commented 9 years ago

I'm going to have to :+1: this one. This seems like an extremely sensible way to resolve callables, allow a wide-range of DIC usage, and keep the framework agile to a variety of approaches with regard to dispatching. @codeguy Do you have any thoughts here?

codeguy commented 9 years ago

3.0.0 will work how it currently does. However, we can introduce alternative dispatching strategies in a minor point release that lets us swap in alt strategies that can rely on the container in ways described above.

mnapoli commented 9 years ago

I understand, in that case I would suggest to introduce an interface in Slim similar to InvokerInterface, and add a default implementation that simply does call_user_func_array().

The current behavior will be kept, the overhead will be invisible, but it can be possible from v3.0 to provide the features I listed above. I would be interested for example to try publishing a pre-configured version of Slim that comes with such features.

What I can do:

What do you think?

codeguy commented 9 years ago

I think RouterStrategyInterface is a good name. And +1 for getting this going on your fork. Will definitely be willing to merge upstream for a 3.x point release.

lalop commented 9 years ago

Hello, I may be wrong but that look like the CallableResolver https://github.com/slimphp/Slim/blob/develop/Slim/CallableResolver.php

If you add this class to the container and add an Interface we could easily make new implementation.

@codeguy a such Interface should manage route and middleware.

mnapoli commented 9 years ago

@lalop it's possible yes! I'm not entirely familiar with this part. There's also the ResolveCallable trait, between those two I'm still a little confused I need to spend more time in the code. The Slim\App still directly invokes the controller though, so the CallableResolver seems to only wrap a controller in another closure to add some behavior.

lalop commented 9 years ago

It's all new and yes there is a naming issue pointed here https://github.com/slimphp/Slim/pull/1180#issuecomment-94196541 and not solve yet. I hope to find the solution here in fact :)

ResolveCallable is the first implementation, it now uses CallableResolver to wrap and resolve.

mnapoli commented 9 years ago

OK thanks for the link. In any case I think this issue can be closed for now.