merged branch DrBenton/after-on-controllers (PR #272)
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 ! :-)
Showing
Please register or sign in to comment