Merge pull request #10 from Minstel/middleware-errorsorigin/router-cleanup

Middleware errors

7 files changed, 379 insertions, 12 deletions

diff --git a/src/Router.php b/src/Router.php
index 955e1e1..18c40be 100644
--- a/src/Router.php
+++ b/src/Router.php
@@ -78,7 +78,7 @@ class Router
      * @param ResponseInterface      $response
      * @return ResponseInterface
      */
-    final public function run(ServerRequestInterface $request, ResponseInterface $response)
+    final public function handle(ServerRequestInterface $request, ResponseInterface $response)
     {
         return $this->__invoke($request, $response);
     }
@@ -93,11 +93,11 @@ class Router
      */
     public function __invoke(ServerRequestInterface $request, ResponseInterface $response, $next = null)
     {
-        $handle = [$this, 'handle'];
+        $run = [$this, 'run'];
 
-        #Call to $this->handle will be executed last in the chain of middlewares
-        $next = function(ServerRequestInterface $request, ResponseInterface $response) use ($next, $handle) {
-            return call_user_func($handle, $request, $response, $next);
+        #Call to $this->run will be executed last in the chain of middlewares
+        $next = function(ServerRequestInterface $request, ResponseInterface $response) use ($next, $run) {
+            return call_user_func($run, $request, $response, $next);
         };
 
         #Build middlewares call chain, so that the last added was executed in first place
@@ -118,7 +118,7 @@ class Router
      * @param callback      $next
      * @return ResponseInterface
      */
-    protected function handle(ServerRequestInterface $request, ResponseInterface $response, $next = null)
+    public function run(ServerRequestInterface $request, ResponseInterface $response, $next = null)
     {
         $glob = new Glob($this->routes);
         $route = $glob->getRoute($request);
diff --git a/src/Router/Middleware/ErrorHandler.php b/src/Router/Middleware/ErrorHandler.php
new file mode 100644
index 0000000..789c455
--- /dev/null
+++ b/src/Router/Middleware/ErrorHandler.php
@@ -0,0 +1,54 @@
+<?php
+
+namespace Jasny\Router\Middleware;
+
+use Psr\Http\Message\ServerRequestInterface;
+use Psr\Http\Message\ResponseInterface;
+
+/**
+ * Handle error in following middlewares/app actions
+ */
+class ErrorHandler
+{    
+    /**
+     * Run middleware action
+     *
+     * @param ServerRequestInterface $request
+     * @param ResponseInterface      $response
+     * @param callback               $next
+     * @return ResponseInterface
+     */
+    public function __invoke(ServerRequestInterface $request, ResponseInterface $response, $next = null)
+    {
+        if ($next && !is_callable($next)) {
+            throw new \InvalidArgumentException("'next' should be a callback");            
+        }
+
+        $error = false;
+
+        try {
+            $response = $next ? call_user_func($next, $request, $response) : $response;
+        } catch(\Throwable $e) {
+            $error = true;
+        } catch(\Exception $e) { #This block can be removed when migrating to PHP7, because Throwable represents both Exception and Error
+            $error = true;
+        }
+
+        return $error ? $this->handleError($response) : $response;
+    }
+
+    /**
+     * Handle caught error
+     *
+     * @param ResponseInterface $response
+     * @return ResponseInterface
+     */
+    protected function handleError($response)
+    {
+        $body = $response->getBody();        
+        $body->rewind();
+        $body->write('Unexpected error');
+
+        return $response->withStatus(500, 'Internal Server Error')->withBody($body);
+    }
+}
diff --git a/src/Router/Middleware/ErrorPage.php b/src/Router/Middleware/ErrorPage.php
new file mode 100644
index 0000000..7064b1e
--- /dev/null
+++ b/src/Router/Middleware/ErrorPage.php
@@ -0,0 +1,75 @@
+<?php
+
+namespace Jasny\Router\Middleware;
+
+use Jasny\Router;
+use Psr\Http\Message\ServerRequestInterface;
+use Psr\Http\Message\ResponseInterface;
+
+/**
+ * Route to error page on error
+ */
+class ErrorPage
+{    
+    /**
+     * Router
+     * @var Router
+     */
+    protected $router = null;
+
+    /**
+     * Class constructor
+     * 
+     * @param Router $routes
+     */
+    public function __construct(Router $router)
+    {
+        $this->router = $router;
+    }
+
+    /**
+     * Get router connected to middleware
+     *
+     * @return Router
+     */
+    public function getRouter()
+    {
+        return $this->router;
+    }
+
+    /**
+     * Run middleware action
+     *
+     * @param ServerRequestInterface $request
+     * @param ResponseInterface      $response
+     * @param callback               $next
+     * @return ResponseInterface
+     */
+    public function __invoke(ServerRequestInterface $request, ResponseInterface $response, $next = null)
+    {
+        if ($next && !is_callable($next)) {
+            throw new \InvalidArgumentException("'next' should be a callback");            
+        }
+
+        $response = $next ? call_user_func($next, $request, $response) : $response;    
+        $status = $response->getStatusCode();
+
+        if (!$this->isErrorStatus($status)) return $response;
+
+        $uri = $request->getUri()->withPath("/$status");
+        $request = $request->withUri($uri, true);
+
+        return $this->getRouter()->run($request, $response);
+    }
+
+    /**
+     * Detect if response has error status code
+     *
+     * @param int $status
+     * @return boolean
+     */
+    protected function isErrorStatus($status)
+    {
+        return $status >= 400 && $status < 600;
+    }
+}
diff --git a/tests/Router/Middleware/ErrorHandlerTest.php b/tests/Router/Middleware/ErrorHandlerTest.php
new file mode 100644
index 0000000..c4b6313
--- /dev/null
+++ b/tests/Router/Middleware/ErrorHandlerTest.php
@@ -0,0 +1,86 @@
+<?php
+
+use Jasny\Router\Middleware\ErrorHandler;
+use Psr\Http\Message\ServerRequestInterface;
+use Psr\Http\Message\ResponseInterface;
+use Psr\Http\Message\StreamInterface;
+
+class ErrorHandlerTest extends PHPUnit_Framework_TestCase
+{
+    /**
+     * Test invoke with invalid 'next' param
+     */
+    public function testInvokeInvalidNext()
+    {
+        $middleware = new ErrorHandler();
+        list($request, $response) = $this->getRequests();
+
+        $this->expectException(\InvalidArgumentException::class);
+
+        $result = $middleware($request, $response, 'not_callable');
+    }
+
+    /**
+     * Test that exception in 'next' callback is caught
+     */
+    public function testInvokeCatchError()
+    {
+        $middleware = new ErrorHandler();
+        list($request, $response) = $this->getRequests();
+
+        $this->expectCatchError($response);
+
+        $result = $middleware($request, $response, function($request, $response) {
+            throw new Exception('Test exception'); 
+        });
+
+        $this->assertEquals(get_class($response), get_class($result), "Middleware should return response object");        
+    }
+
+    /**
+     * Test case when there is no error
+     */
+    public function testInvokeNoError()
+    {
+        $middleware = new ErrorHandler();
+        list($request, $response) = $this->getRequests();
+
+        $result = $middleware($request, $response, function($request, $response) {
+            $response->nextCalled = true;
+
+            return $response;
+        });        
+
+        $this->assertEquals(get_class($response), get_class($result), "Middleware should return response object");        
+        $this->assertTrue($result->nextCalled, "'next' was not called");   
+    }
+
+    /**
+     * Get requests for testing
+     *
+     * @return array
+     */
+    public function getRequests()
+    {
+        $request = $this->createMock(ServerRequestInterface::class);
+        $response = $this->createMock(ResponseInterface::class);
+
+        return [$request, $response];
+    }
+
+    /**
+     * Expect for error
+     *
+     * @param ResponseInterface $response 
+     */
+    public function expectCatchError($response)
+    {
+        $stream = $this->createMock(StreamInterface::class);
+        $stream->expects($this->once())->method('rewind');
+        $stream->expects($this->once())->method('write')->with($this->equalTo('Unexpected error'));
+
+        $response->method('getBody')->will($this->returnValue($stream));
+        $response->expects($this->once())->method('withBody')->with($this->equalTo($stream))->will($this->returnSelf());
+        $response->expects($this->once())->method('withStatus')->with($this->equalTo(500), $this->equalTo('Internal Server Error'))->will($this->returnSelf());
+    }
+}
diff --git a/tests/Router/Middleware/ErrorPageTest.php b/tests/Router/Middleware/ErrorPageTest.php
new file mode 100644
index 0000000..da87cd0
--- /dev/null
+++ b/tests/Router/Middleware/ErrorPageTest.php
@@ -0,0 +1,152 @@
+<?php
+
+use Jasny\Router;
+use Jasny\Router\Routes\Glob;
+use Jasny\Router\Middleware\ErrorPage;
+use Psr\Http\Message\ServerRequestInterface;
+use Psr\Http\Message\ResponseInterface;
+use Psr\Http\Message\UriInterface;
+
+class ErrorPageTest extends PHPUnit_Framework_TestCase
+{
+    /**
+     * Test invoke with invalid 'next' param
+     */
+    public function testInvokeInvalidNext()
+    {
+        $middleware = new ErrorPage($this->getRouter());
+        list($request, $response) = $this->getRequests();
+
+        $this->expectException(\InvalidArgumentException::class);
+
+        $result = $middleware($request, $response, 'not_callable');
+    }
+
+    /**
+     * Test error and not-error cases with calling 'next' callback
+     *
+     * @dataProvider invokeProvider
+     * @param int $statusCode
+     */
+    public function testInvokeNext($statusCode)
+    {
+        $isError = $statusCode >= 400;
+        $router = $this->getRouter();
+        $middleware = new ErrorPage($router);
+        list($request, $response) = $this->getRequests($statusCode);
+
+        $isError ?
+            $this->expectSetError($router, $request, $response, $statusCode) :
+            $this->notExpectSetError($router, $request, $response, $statusCode);
+
+        $result = $middleware($request, $response, function($request, $response) {
+            $response->nextCalled = true;
+
+            return $response;
+        });
+
+        $this->assertEquals(get_class($response), get_class($result), "Middleware should return response object");
+        $this->assertTrue($result->nextCalled, "'next' was not called");        
+    }
+
+    /**
+     * Test error and not-error cases without calling 'next' callback
+     *
+     * @dataProvider invokeProvider
+     * @param int $statusCode
+     */
+    public function testInvokeNoNext($statusCode)
+    {
+        $isError = $statusCode >= 400;
+        $router = $this->getRouter();
+        $middleware = new ErrorPage($router);
+        list($request, $response) = $this->getRequests($statusCode);
+
+        $isError ?
+            $this->expectSetError($router, $request, $response, $statusCode) :
+            $this->notExpectSetError($router, $request, $response, $statusCode);
+
+        $result = $middleware($request, $response);
+
+        $this->assertEquals($response, $result, "Middleware should return response object");     
+    }
+
+    /**
+     * Provide data for testing '__invoke' method
+     *
+     * @return array
+     */
+    public function invokeProvider()
+    {
+        return [
+            [200],
+            [300],
+            [400],
+            [404],
+            [500],
+            [503]
+        ];
+    }
+
+    /**
+     * Get requests for testing
+     *
+     * @param int $statusCode 
+     * @return array
+     */
+    public function getRequests($statusCode = null)
+    {
+        $request = $this->createMock(ServerRequestInterface::class);
+        $response = $this->createMock(ResponseInterface::class);
+
+        if ($statusCode) {
+            $response->method('getStatusCode')->will($this->returnValue($statusCode));            
+        }
+
+        return [$request, $response];
+    }
+
+    /**
+     * @return Router
+     */
+    public function getRouter()
+    {
+        return $this->getMockBuilder(Router::class)->disableOriginalConstructor()->getMock();
+    }
+
+    /**
+     * Expect for error
+     *
+     * @param Router $router
+     * @param ServerRequestInterface $request 
+     * @param ResponseInterface $response 
+     * @param int $statusCode 
+     */
+    public function expectSetError($router, $request, $response, $statusCode)
+    {
+        $uri = $this->createMock(UriInterface::class);
+
+        $uri->expects($this->once())->method('withPath')->with($this->equalTo("/$statusCode"))->will($this->returnSelf());
+        $request->expects($this->once())->method('getUri')->will($this->returnValue($uri));
+        $request->expects($this->once())->method('withUri')->with($this->equalTo($uri), $this->equalTo(true))->will($this->returnSelf());
+        $router->expects($this->once())->method('run')->with($this->equalTo($request), $this->equalTo($response))->will($this->returnValue($response));
+    }
+
+    /**
+     * Not expect for error
+     *
+     * @param Router $router
+     * @param ServerRequestInterface $request 
+     * @param ResponseInterface $response 
+     * @param int $statusCode 
+     */
+    public function notExpectSetError($router, $request, $response, $statusCode)
+    {
+        $uri = $this->createMock(UriInterface::class);
+
+        $uri->expects($this->never())->method('withPath');
+        $request->expects($this->never())->method('getUri');
+        $request->expects($this->never())->method('withUri');
+        $router->expects($this->never())->method('run');
+    }
+}
diff --git a/tests/Router/RunnerTest.php b/tests/Router/RunnerTest.php
index 60b80b9..1a2f571 100644
--- a/tests/Router/RunnerTest.php
+++ b/tests/Router/RunnerTest.php
@@ -48,7 +48,7 @@ class RunnerTest extends PHPUnit_Framework_TestCase
      */
     public function testInvoke()
     {
-        $runner = $this->getMockBuilder('Jasny\Router\Runner')->disableOriginalConstructor()->getMockForAbstractClass();
+        $runner = $this->getMockBuilder(Runner::class)->disableOriginalConstructor()->getMockForAbstractClass();
         $queries = [
             'request' => $this->createMock(ServerRequestInterface::class),
             'response' => $this->createMock(ResponseInterface::class)
diff --git a/tests/RouterTest.php b/tests/RouterTest.php
index 9b63360..ef19458 100644
--- a/tests/RouterTest.php
+++ b/tests/RouterTest.php
@@ -23,9 +23,9 @@ class RouterTest extends PHPUnit_Framework_TestCase
     }
 
     /**
-     * Test that on router 'run', method '__invoke' is called
+     * Test that on router 'handle', method '__invoke' is called
      */
-    public function testRun()
+    public function testHandle()
     {
         $router = $this->createMock(Router::class, ['__invoke']);
         list($request, $response) = $this->getRequests();
@@ -34,7 +34,7 @@ class RouterTest extends PHPUnit_Framework_TestCase
             return ['request' => $arg1, 'response' => $arg2];
         }));
 
-        $result = $router->run($request, $response);
+        $result = $router->handle($request, $response);
 
         $this->assertEquals($request, $result['request'], "Request was not processed correctly");
         $this->assertEquals($response, $result['response'], "Response was not processed correctly");
@@ -148,7 +148,7 @@ class RouterTest extends PHPUnit_Framework_TestCase
     {
         $routes = [
             '/foo' => Route::create(['fn' => function($request, $response) {
-                $response->testMiddlewareCalls[] = 'handle';
+                $response->testMiddlewareCalls[] = 'run';
                 return $response;
             }])
         ];
@@ -162,7 +162,7 @@ class RouterTest extends PHPUnit_Framework_TestCase
             return $response;
         });
 
-        $this->assertEquals(['first','last','handle','outer'], $response->testMiddlewareCalls, "Actions were executed in wrong order");
+        $this->assertEquals(['first','last','run','outer'], $response->testMiddlewareCalls, "Actions were executed in wrong order");
     }
 
     /**