Commits
-------

6445b9e9 Add Routes Middlewares

Discussion
----------

add "after($callback)" to Controller in order to mimic a "Route Middleware" feature in Silex

Hi !

As discussed in #262 and in my previous similar PR (https://github.com/fabpot/Silex/pull/270) with @GromNaN, here is another implementation of the Routes Middlewares feature, shipped with its Unit Test.

The feature addition is still 100% backward compatible, but this time it may be more "Silex style" compliant - and it's "100% no performance loss" :-)

With this PR the Silex ```Controller``` class has a new ```before()``` method, which registers a callback to trigger when the Route is matched, just before the route callback itself.
This ```before()``` method can be called more than one time on a single Route definition, allowing multiple secondary callbacks (a.k.a. "Routes Middlewares") to be executed before the Route main callback.

Would this feature be eligible for Silex integration, like this ?

Olivier

---------------------------------------------------------------------------

by DrBenton at 2012-03-01T22:16:05Z

* The method is now named ```addMiddleware```, which may be more explicit for people used to "a la Sinatra" framewoks like _Node.js Express_  or _Slim_.
* I no longer use the Silex ControllerResolver, but a dedicated Application protected service instead, fired just before the Application "before" process (I could have added a new listener on the ```Kernel```, but since we already have one for the right event for the "before" process, I used this one for simpler code).
* I also added some checks on the ```$request``` in the Unit Test.

---------------------------------------------------------------------------

by fabpot at 2012-03-02T11:40:30Z

Can you add some information about this new feature in the documentation (with some typical usages)? Thanks for your work on this feature.

---------------------------------------------------------------------------

by DrBenton at 2012-03-02T12:35:44Z

**Done !** (the method is now named ```middleware```, and I've removed the useless comment)

I've tried my best for the feature documentation, but unfortunately my english is fairly poor... :-/

At this point, the Middlewares have access to the Request, but if I'm not wrong it can't be used for redirecting, for instance. Should I handle ```Response``` returns from middlewares, as in ```before``` filter ?

---------------------------------------------------------------------------

by GromNaN at 2012-03-02T12:39:00Z

I guess you can throw an exception (access denied for example) and manage this exception with the error handlers.

---------------------------------------------------------------------------

by DrBenton at 2012-03-02T12:44:38Z

Yes, in the sample usage of the documentation I've used exactly that ```Exception``` process.
But I don't know if this is ok for everyone ?

---------------------------------------------------------------------------

by igorw at 2012-03-02T15:08:29Z

Very nice feature. But it should really be possible to return a response from the handler, like you can do with before() filters.

---------------------------------------------------------------------------

by DrBenton at 2012-03-02T21:15:41Z

All right, I think I know how to handle Responses, thanks to the code of the before() filter ; I'll push soon a new commit with the last code syle fixes and this feature !

---------------------------------------------------------------------------

by DrBenton at 2012-03-03T11:34:44Z

Ok, I've fixed the code style problems, and handled the Responses from middlewares.
What do you think of it ?

---------------------------------------------------------------------------

by DrBenton at 2012-03-05T13:11:56Z

So... Are we OK for this feature, gentlemen ? :-)
Do you want me to push a squashed commit ?

---------------------------------------------------------------------------

by igorw at 2012-03-05T22:24:53Z

Apart from the minor comments I made I am quite happy with the implementation as it is. I guess when those are fixed it can be merged.

---------------------------------------------------------------------------

by DrBenton at 2012-03-05T22:58:23Z

Sorry for all these "code style" errors, I'm still not enough used to Symfony 2 coding standards - but I'll get them ! :-)

Well, all these errors should be fixed now. I also swapped  the order of execution with the Silex global ```before``` filters, as you asked it: the global filters are now triggered **before** the middlewares of the matched route.

---------------------------------------------------------------------------

by igorw at 2012-03-05T23:10:35Z

I'm not so sure about checking for `instanceof Response`. Maybe we should enforce a middleware to return either null or a Response to prevent unexpected behaviour.

---------------------------------------------------------------------------

by DrBenton at 2012-03-06T14:37:42Z

Well, I just copied the behaviour of the ```before``` filter :
```
if ($ret instanceof Response) {
    $event->setResponse($ret);
}```
(in Application.php, line 237)

Since route middlewares act as this ```before``` filter (in a "specific for each route" way), I thought we could copy the code of this ```before``` functionnality ?

---------------------------------------------------------------------------

by DrBenton at 2012-03-08T14:34:13Z

So ? Do you want me to change the ```instanceof Response```, or can we keep the current "copy of before filter" implementation ?

---------------------------------------------------------------------------

by igorw at 2012-03-08T15:57:00Z

IMO we should add something like this:

    if ($response instanceof Response) {
        ...
    } elseif (null !== $response) {
        throw new \RuntimeException('Before filter (or middleware for route foo) returned an invalid response value. Must return null or an instance of Response.');
    }

---------------------------------------------------------------------------

by DrBenton at 2012-03-08T16:26:54Z

Hum, I see. Sounds good to me, I'll push it soon with the documentation fixes :-)

---------------------------------------------------------------------------

by igorw at 2012-03-08T16:42:48Z

And tests please ;-)

---------------------------------------------------------------------------

by DrBenton at 2012-03-08T22:15:11Z

Here it is, sir ! :-)

---------------------------------------------------------------------------

by DrBenton at 2012-03-10T19:45:59Z

Shall I push in a new PR these 13 commits in a single squashed one ?

---------------------------------------------------------------------------

by igorw at 2012-03-10T19:51:53Z

👍

---------------------------------------------------------------------------

by stof at 2012-03-10T19:52:43Z

No need to create a new PR for this. Simply squash your commits and force the push to the branch

---------------------------------------------------------------------------

by DrBenton at 2012-03-10T21:59:12Z

All right, here is the squashed commit ! :-)