merged branch fabpot/middleware (PR #362)

Commits
-------

d53119eb added a route after middleware
794b3c28 renamed middleware() to before()

Discussion
----------

Implements route after middleware (closes #336)

This PR renames the `middleware` method to `before` and adds a new `after` route middleware.

These changes make Silex more consistent from a user point of view:

 * he can do something before the route callback on an application, a route collection, or just a route (by calling the `before` method on these objects);

 * he can do something after the route callback by calling the `after` method on an application, a route collection, or just a route.

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

by fabpot at 2012-06-13T18:20:37Z

I thought about using the `middleware` method for both before and after by passing the callback as an argument. But this has several drawback:

 * we loose the symmetry with the application before/after calls;
 * the developer must remember to manually call the callback (and symfony1 proves that many devs actually forgets about that);
 * this would act as a filter chain where the order of execution is not always clear when you have many filters.

doc/changelog.rst

@@ -3,6 +3,10 @@ Changelog
 
 This changelog references all backward incompatibilities as we introduce them:
 
+* **2012-06-13**: Added a route ``before`` middleware
+
+* **2012-06-13**: Renamed the route ``middleware`` to ``before``
+
 * **2012-06-13**: Added an extension for the Symfony Security component
 
 * **2012-05-31**: Made the ``BrowserKit``, ``CssSelector``, ``DomCrawler``,

doc/usage.rst

@@ -418,8 +418,13 @@ Route middlewares
 -----------------
 
 Route middlewares are PHP callables which are triggered when their associated
-route is matched. They are fired just before the route callback, but after the
-application ``before`` filters.
+route is matched:
+
+* ``before`` middlewares are fired just before the route callback, but after
+  the application ``before`` filters;
+
+* ``after`` middlewares are fired just after the route callback, but before
+  the application ``after`` filters.
 
 This can be used for a lot of use cases; for instance, here is a simple
 "anonymous/logged user" check::
@@ -439,33 +444,36 @@ This can be used for a lot of use cases; for instance, here is a simple
     $app->get('/user/subscribe', function () {
         ...
     })
-    ->middleware($mustBeAnonymous);
+    ->before($mustBeAnonymous);
 
     $app->get('/user/login', function () {
         ...
     })
-    ->middleware($mustBeAnonymous);
+    ->before($mustBeAnonymous);
 
     $app->get('/user/my-profile', function () {
         ...
     })
-    ->middleware($mustBeLogged);
+    ->before($mustBeLogged);
 
-The ``middleware`` function can be called several times for a given route, in
-which case they are triggered in the same order as you added them to the
-route.
+The ``before`` and ``after`` methods can be called several times for a given
+route, in which case they are triggered in the same order as you added them to
+the route.
 
-For convenience, the route middlewares functions are triggered with the
-current ``Request`` instance as their only argument.
+For convenience, the ``before`` middlewares are called with the current
+``Request`` instance as an argument and the ``after`` middlewares are called
+with the current ``Request`` and ``Response`` instance as arguments.
 
-If any of the route middlewares returns a Symfony HTTP Response, it will
+If any of the before middlewares returns a Symfony HTTP Response, it will
 short-circuit the whole rendering: the next middlewares won't be run, neither
 the route callback. You can also redirect to another page by returning a
 redirect response, which you can create by calling the Application
 ``redirect`` method.
 
-If a route middleware does not return a Symfony HTTP Response or ``null``, a
-``RuntimeException`` is thrown.
+.. note::
+
+    If a before middleware does not return a Symfony HTTP Response or
+    ``null``, a ``RuntimeException`` is thrown.
 
 Global Configuration
 --------------------
@@ -480,7 +488,7 @@ middleware, a requirement, or a default value), you can configure it on
         ->requireHttps()
         ->method('get')
         ->convert('id', function () { // ... })
-        ->middleware(function () { // ... })
+        ->before(function () { // ... })
     ;
 
 These settings are applied to already registered controllers and they become
@@ -651,7 +659,7 @@ would secure all controllers for the backend collection::
     $backend = new ControllerCollection();
 
     // ensure that all controllers require logged-in users
-    $backend->middleware($mustBeLogged);
+    $backend->before($mustBeLogged);
 
 .. tip::
 

src/Silex/Application.php

@@ -14,7 +14,6 @@ namespace Silex;
 use Symfony\Component\HttpKernel\HttpKernel;
 use Symfony\Component\HttpKernel\HttpKernelInterface;
 use Symfony\Component\HttpKernel\TerminableInterface;
-use Symfony\Component\HttpKernel\Event\KernelEvent;
 use Symfony\Component\HttpKernel\Event\GetResponseForControllerResultEvent;
 use Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent;
 use Symfony\Component\HttpKernel\Event\FilterControllerEvent;
@@ -112,21 +111,38 @@ class Application extends \Pimple implements HttpKernelInterface, EventSubscribe
             return new RedirectableUrlMatcher($app['routes'], $app['request_context']);
         });
 
-        $this['route_middlewares_trigger'] = $this->protect(function (KernelEvent $event) use ($app) {
+        $this['route_before_middlewares_trigger'] = $this->protect(function (GetResponseEvent $event) use ($app) {
             $request = $event->getRequest();
             $routeName = $request->attributes->get('_route');
             if (!$route = $app['routes']->get($routeName)) {
                 return;
             }
 
-            foreach ((array) $route->getOption('_middlewares') as $callback) {
+            foreach ((array) $route->getOption('_before_middlewares') as $callback) {
                 $ret = call_user_func($callback, $request);
                 if ($ret instanceof Response) {
                     $event->setResponse($ret);
 
                     return;
                 } elseif (null !== $ret) {
-                    throw new \RuntimeException(sprintf('Middleware for route "%s" returned an invalid response value. Must return null or an instance of Response.', $routeName));
+                    throw new \RuntimeException(sprintf('A before middleware for route "%s" returned an invalid response value. Must return null or an instance of Response.', $routeName));
+                }
+            }
+        });
+
+        $this['route_after_middlewares_trigger'] = $this->protect(function (FilterResponseEvent $event) use ($app) {
+            $request = $event->getRequest();
+            $routeName = $request->attributes->get('_route');
+            if (!$route = $app['routes']->get($routeName)) {
+                return;
+            }
+
+            foreach ((array) $route->getOption('_after_middlewares') as $callback) {
+                $response = call_user_func($callback, $request, $event->getResponse());
+                if ($response instanceof Response) {
+                    $event->setResponse($response);
+                } elseif (null !== $response) {
+                    throw new \RuntimeException(sprintf('An after middleware for route "%s" returned an invalid response value. Must return null or an instance of Response.', $routeName));
                 }
             }
         });
@@ -516,8 +532,9 @@ class Application extends \Pimple implements HttpKernelInterface, EventSubscribe
         if (HttpKernelInterface::MASTER_REQUEST === $event->getRequestType()) {
             $this->beforeDispatched = true;
             $this['dispatcher']->dispatch(SilexEvents::BEFORE, $event);
-            $this['route_middlewares_trigger']($event);
         }
+
+        $this['route_before_middlewares_trigger']($event);
     }
 
     /**
@@ -557,6 +574,8 @@ class Application extends \Pimple implements HttpKernelInterface, EventSubscribe
      */
     public function onKernelResponse(FilterResponseEvent $event)
     {
+        $this['route_after_middlewares_trigger']($event);
+
         if (HttpKernelInterface::MASTER_REQUEST === $event->getRequestType()) {
             $this['dispatcher']->dispatch(SilexEvents::AFTER, $event);
         }

src/Silex/Controller.php

@@ -157,15 +157,28 @@ class Controller
 
     /**
      * Sets a callback to handle before triggering the route callback.
-     * (a.k.a. "Route Middleware")
      *
      * @param mixed $callback A PHP callback to be triggered when the Route is matched, just before the route callback
      *
      * @return Controller $this The current Controller instance
      */
-    public function middleware($callback)
+    public function before($callback)
     {
-        $this->route->middleware($callback);
+        $this->route->before($callback);
+
+        return $this;
+    }
+
+    /**
+     * Sets a callback to handle after the route callback.
+     *
+     * @param mixed $callback A PHP callback to be triggered after the route callback
+     *
+     * @return Controller $this The current Controller instance
+     */
+    public function after($callback)
+    {
+        $this->route->after($callback);
 
         return $this;
     }

src/Silex/ControllerCollection.php

@@ -219,18 +219,35 @@ class ControllerCollection
 
     /**
      * Sets a callback to handle before triggering the route callback.
-     * (a.k.a. "Route Middleware")
      *
      * @param mixed $callback A PHP callback to be triggered when the Route is matched, just before the route callback
      *
-     * @return ControllerCollection $this The current Controller instance
+     * @return ControllerCollection $this The current ControllerCollection instance
+     */
+    public function before($callback)
+    {
+        $this->defaultRoute->before($callback);
+
+        foreach ($this->controllers as $controller) {
+            $controller->before($callback);
+        }
+
+        return $this;
+    }
+
+    /**
+     * Sets a callback to handle after the route callback.
+     *
+     * @param mixed $callback A PHP callback to be triggered after the route callback
+     *
+     * @return ControllerCollection $this The current ControllerCollection instance
      */
-    public function middleware($callback)
+    public function after($callback)
     {
-        $this->defaultRoute->middleware($callback);
+        $this->defaultRoute->after($callback);
 
         foreach ($this->controllers as $controller) {
-            $controller->middleware($callback);
+            $controller->after($callback);
         }
 
         return $this;

src/Silex/Route.php

@@ -107,17 +107,32 @@ class Route extends BaseRoute
 
     /**
      * Sets a callback to handle before triggering the route callback.
-     * (a.k.a. "Route Middleware")
      *
      * @param mixed $callback A PHP callback to be triggered when the Route is matched, just before the route callback
      *
      * @return Controller $this The current Controller instance
      */
-    public function middleware($callback)
+    public function before($callback)
     {
-        $middlewareCallbacks = $this->getOption('_middlewares');
-        $middlewareCallbacks[] = $callback;
-        $this->setOption('_middlewares', $middlewareCallbacks);
+        $callbacks = $this->getOption('_before_middlewares');
+        $callbacks[] = $callback;
+        $this->setOption('_before_middlewares', $callbacks);
+
+        return $this;
+    }
+
+    /**
+     * Sets a callback to handle after the route callback.
+     *
+     * @param mixed $callback A PHP callback to be triggered after the route callback
+     *
+     * @return Controller $this The current Controller instance
+     */
+    public function after($callback)
+    {
+        $callbacks = $this->getOption('_after_middlewares');
+        $callbacks[] = $callback;
+        $this->setOption('_after_middlewares', $callbacks);
 
         return $this;
     }

tests/Silex/Tests/ApplicationTest.php

@@ -174,15 +174,27 @@ class ApplicationTest extends \PHPUnit_Framework_TestCase
         $test = $this;
 
         $middlewareTarget = array();
-        $middleware1 = function (Request $request) use (&$middlewareTarget, $test) {
+        $beforeMiddleware1 = function (Request $request) use (&$middlewareTarget, $test) {
             $test->assertEquals('/reached', $request->getRequestUri());
-            $middlewareTarget[] = 'middleware1_triggered';
+            $middlewareTarget[] = 'before_middleware1_triggered';
         };
-        $middleware2 = function (Request $request) use (&$middlewareTarget, $test) {
+        $beforeMiddleware2 = function (Request $request) use (&$middlewareTarget, $test) {
             $test->assertEquals('/reached', $request->getRequestUri());
-            $middlewareTarget[] = 'middleware2_triggered';
+            $middlewareTarget[] = 'before_middleware2_triggered';
         };
-        $middleware3 = function (Request $request) use (&$middlewareTarget, $test) {
+        $beforeMiddleware3 = function (Request $request) use (&$middlewareTarget, $test) {
+            throw new \Exception('This middleware shouldn\'t run!');
+        };
+
+        $afterMiddleware1 = function (Request $request, Response $response) use (&$middlewareTarget, $test) {
+            $test->assertEquals('/reached', $request->getRequestUri());
+            $middlewareTarget[] = 'after_middleware1_triggered';
+        };
+        $afterMiddleware2 = function (Request $request, Response $response) use (&$middlewareTarget, $test) {
+            $test->assertEquals('/reached', $request->getRequestUri());
+            $middlewareTarget[] = 'after_middleware2_triggered';
+        };
+        $afterMiddleware3 = function (Request $request, Response $response) use (&$middlewareTarget, $test) {
             throw new \Exception('This middleware shouldn\'t run!');
         };
 
@@ -191,28 +203,31 @@ class ApplicationTest extends \PHPUnit_Framework_TestCase
 
             return 'hello';
         })
-        ->middleware($middleware1)
-        ->middleware($middleware2);
+        ->before($beforeMiddleware1)
+        ->before($beforeMiddleware2)
+        ->after($afterMiddleware1)
+        ->after($afterMiddleware2);
 
         $app->get('/never-reached', function () use (&$middlewareTarget) {
             throw new \Exception('This route shouldn\'t run!');
         })
-        ->middleware($middleware3);
+        ->before($beforeMiddleware3)
+        ->after($afterMiddleware3);
 
         $result = $app->handle(Request::create('/reached'));
 
-        $this->assertSame(array('middleware1_triggered', 'middleware2_triggered', 'route_triggered'), $middlewareTarget);
+        $this->assertSame(array('before_middleware1_triggered', 'before_middleware2_triggered', 'route_triggered', 'after_middleware1_triggered', 'after_middleware2_triggered'), $middlewareTarget);
         $this->assertEquals('hello', $result->getContent());
     }
 
-    public function testRoutesMiddlewaresWithResponseObject()
+    public function testRoutesBeforeMiddlewaresWithResponseObject()
     {
         $app = new Application();
 
         $app->get('/foo', function () {
             throw new \Exception('This route shouldn\'t run!');
         })
-        ->middleware(function () {
+        ->before(function () {
             return new Response('foo');
         });
 
@@ -222,14 +237,31 @@ class ApplicationTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals('foo', $result->getContent());
     }
 
-    public function testRoutesMiddlewaresWithRedirectResponseObject()
+    public function testRoutesAfterMiddlewaresWithResponseObject()
+    {
+        $app = new Application();
+
+        $app->get('/foo', function () {
+            return new Response('foo');
+        })
+        ->after(function () {
+            return new Response('bar');
+        });
+
+        $request = Request::create('/foo');
+        $result = $app->handle($request);
+
+        $this->assertEquals('bar', $result->getContent());
+    }
+
+    public function testRoutesBeforeMiddlewaresWithRedirectResponseObject()
     {
         $app = new Application();
 
         $app->get('/foo', function () {
             throw new \Exception('This route shouldn\'t run!');
         })
-        ->middleware(function () use ($app) {
+        ->before(function () use ($app) {
             return $app->redirect('/bar');
         });
 
@@ -240,7 +272,7 @@ class ApplicationTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals('/bar', $result->getTargetUrl());
     }
 
-    public function testRoutesMiddlewaresTriggeredAfterSilexBeforeFilters()
+    public function testRoutesBeforeMiddlewaresTriggeredAfterSilexBeforeFilters()
     {
         $app = new Application();
 
@@ -252,7 +284,7 @@ class ApplicationTest extends \PHPUnit_Framework_TestCase
         $app->get('/foo', function () use (&$middlewareTarget) {
             $middlewareTarget[] = 'route_triggered';
         })
-        ->middleware($middleware);
+        ->before($middleware);
 
         $app->before(function () use (&$middlewareTarget) {
             $middlewareTarget[] = 'before_triggered';
@@ -263,6 +295,29 @@ class ApplicationTest extends \PHPUnit_Framework_TestCase
         $this->assertSame(array('before_triggered', 'middleware_triggered', 'route_triggered'), $middlewareTarget);
     }
 
+    public function testRoutesAfterMiddlewaresTriggeredBeforeSilexAfterFilters()
+    {
+        $app = new Application();
+
+        $middlewareTarget = array();
+        $middleware = function (Request $request) use (&$middlewareTarget) {
+            $middlewareTarget[] = 'middleware_triggered';
+        };
+
+        $app->get('/foo', function () use (&$middlewareTarget) {
+            $middlewareTarget[] = 'route_triggered';
+        })
+        ->after($middleware);
+
+        $app->after(function () use (&$middlewareTarget) {
+            $middlewareTarget[] = 'after_triggered';
+        });
+
+        $app->handle(Request::create('/foo'));
+
+        $this->assertSame(array('route_triggered', 'middleware_triggered', 'after_triggered'), $middlewareTarget);
+    }
+
     public function testFinishFilter()
     {
         $containerTarget = array();
@@ -293,7 +348,26 @@ class ApplicationTest extends \PHPUnit_Framework_TestCase
     /**
      * @expectedException RuntimeException
      */
-    public function testNonResponseAndNonNullReturnFromRouteMiddlewareShouldThrowRuntimeException()
+    public function testNonResponseAndNonNullReturnFromRouteBeforeMiddlewareShouldThrowRuntimeException()
+    {
+        $app = new Application();
+
+        $middleware = function (Request $request) {
+            return 'string return';
+        };
+
+        $app->get('/', function () {
+            return 'hello';
+        })
+        ->before($middleware);
+
+        $app->handle(Request::create('/'), HttpKernelInterface::MASTER_REQUEST, false);
+    }
+
+    /**
+     * @expectedException RuntimeException
+     */
+    public function testNonResponseAndNonNullReturnFromRouteAfterMiddlewareShouldThrowRuntimeException()
     {
         $app = new Application();
 
@@ -304,7 +378,7 @@ class ApplicationTest extends \PHPUnit_Framework_TestCase
         $app->get('/', function () {
             return 'hello';
         })
-        ->middleware($middleware);
+        ->after($middleware);
 
         $app->handle(Request::create('/'), HttpKernelInterface::MASTER_REQUEST, false);
     }

tests/Silex/Tests/ControllerCollectionTest.php

@@ -136,13 +136,23 @@ class ControllerCollectionTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals('http', $controller->getRoute()->getRequirement('_scheme'));
     }
 
-    public function testMiddleware()
+    public function testBefore()
     {
         $controllers = new ControllerCollection();
-        $controllers->middleware('mid1');
-        $controller = $controllers->match('/{id}/{name}/{extra}', function () {})->middleware('mid2');
-        $controllers->middleware('mid3');
+        $controllers->before('mid1');
+        $controller = $controllers->match('/{id}/{name}/{extra}', function () {})->before('mid2');
+        $controllers->before('mid3');
 
-        $this->assertEquals(array('mid1', 'mid2', 'mid3'), $controller->getRoute()->getOption('_middlewares'));
+        $this->assertEquals(array('mid1', 'mid2', 'mid3'), $controller->getRoute()->getOption('_before_middlewares'));
+    }
+
+    public function testAfter()
+    {
+        $controllers = new ControllerCollection();
+        $controllers->after('mid1');
+        $controller = $controllers->match('/{id}/{name}/{extra}', function () {})->after('mid2');
+        $controllers->after('mid3');
+
+        $this->assertEquals(array('mid1', 'mid2', 'mid3'), $controller->getRoute()->getOption('_after_middlewares'));
     }
 }