• Fabien Potencier's avatar
    merged branch DrBenton/after-on-controllers (PR #272) · 3e6fe5e9
    Fabien Potencier authored
    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 ! :-)
    3e6fe5e9
Name
Last commit
Last update
doc Loading commit data...
src/Silex Loading commit data...
tests/Silex/Tests Loading commit data...
vendor Loading commit data...
.gitignore Loading commit data...
LICENSE Loading commit data...
README.md Loading commit data...
compile Loading commit data...
composer.json Loading commit data...
composer.lock Loading commit data...
example.htaccess Loading commit data...
phpunit.xml.dist Loading commit data...