Allow top-level error handler within dispatcher (#37)

src/Dispatcher.php

@@ -6,11 +6,14 @@
 
 use BadMethodCallException;
 use DomainException;
+use Firehed\API\Interfaces\ErrorHandlerInterface;
 use Firehed\Common\ClassMapper;
 use Firehed\Input\Containers\ParsedInput;
 use Psr\Container\ContainerInterface;
 use Psr\Http\Message\RequestInterface;
 use Psr\Http\Message\ResponseInterface;
+use Psr\Http\Message\ServerRequestInterface;
+use Throwable;
 use OutOfBoundsException;
 use UnexpectedValueException;
 
@@ -64,6 +67,19 @@ public function setContainer(ContainerInterface $container = null): self
         return $this;
     }
 
+    /**
+     * Provide a default error handler. This will be used in the event that an
+     * endpoint does not define its own handler.
+     *
+     * @param ErrorHandlerInterface $handler
+     * @return self
+     */
+    public function setErrorHandler(ErrorHandlerInterface $handler): self
+    {
+        $this->error_handler = $handler;
+        return $this;
+    }
+
     /**
      * Inject the request
      *
@@ -135,8 +151,19 @@ public function dispatch(): ResponseInterface
                 ->validate($endpoint);
 
             $response = $endpoint->execute($safe_input);
-        } catch (\Throwable $e) {
-            $response = $endpoint->handleException($e);
+        } catch (Throwable $e) {
+            try {
+                $response = $endpoint->handleException($e);
+            } catch (Throwable $e) {
+                // If an application-wide handler has been defined, use the
+                // response that it generates. If not, just rethrow the
+                // exception for the system default (if defined) to handle.
+                if ($this->error_handler && $this->request instanceof ServerRequestInterface) {
+                    $response = $this->error_handler->handle($this->request, $e);
+                } else {
+                    throw $e;
+                }
+            }
         }
         return $this->executeResponseMiddleware($response);
     }

src/ErrorHandler.php

@@ -7,6 +7,11 @@
 use Psr\Log\LoggerInterface;
 use Throwable;
 
+/**
+ * This will be deprecated in the next minor release and removed in the next
+ * major release. Instead, prefer to provide an ErrorHandlerInterface to the
+ * Dispatcher.
+ */
 class ErrorHandler
 {
     /** @var LoggerInterface */

src/Interfaces/ErrorHandlerInterface.php

@@ -0,0 +1,18 @@
+<?php
+declare(strict_types=1);
+
+namespace Firehed\API\Interfaces;
+
+use Psr\Http\Message\ResponseInterface;
+use Psr\Http\Message\ServerRequestInterface;
+use Throwable;
+
+interface ErrorHandlerInterface
+{
+    /**
+     * Handle an exception for the given request. It is RECOMMENDED that
+     * implementations use the $request's accept header to determine how best
+     * to format the response.
+     */
+    public function handle(ServerRequestInterface $request, Throwable $t): ResponseInterface;
+}

tests/DispatcherTest.php

@@ -5,11 +5,14 @@
 namespace Firehed\API;
 
 use Exception;
+use Firehed\API\Interfaces\EndpointInterface;
+use Firehed\API\Interfaces\ErrorHandlerInterface;
 use Psr\Container\ContainerInterface;
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\RequestInterface;
+use Psr\Http\Message\ServerRequestInterface;
 use Psr\Http\Message\UriInterface;
-use Firehed\API\Interfaces\EndpointInterface;
+use Throwable;
 
 /**
  * @coversDefaultClass Firehed\API\Dispatcher
@@ -335,7 +338,7 @@ public function testErrorInResponseHandler()
                 "The exception thrown from the error handler's failure should ".
                 "have made it through"
             );
-        } catch (\Throwable $e) {
+        } catch (Throwable $e) {
             $this->assertSame(
                 $error,
                 $e,
@@ -450,7 +453,7 @@ public function testFailedAuthenticationReachesErrorHandler()
     }
 
     /** @covers ::dispatch */
-    public function testFailedEndpointExecutionReachesErrorHandler()
+    public function testFailedEndpointExecutionReachesEndpointErrorHandler()
     {
         $e = new Exception('This should reach the error handler');
         $endpoint = $this->getMockEndpoint();
@@ -487,6 +490,102 @@ public function testInvalidTypeResponseFromEndpointReachesErrorHandler()
         $this->executeMockRequestOnEndpoint($endpoint);
     }
 
+    /**
+     * @covers ::dispatch
+     * @covers ::setErrorHandler
+     */
+    public function testExceptionsReachDefaultErrorHandlerWhenSet()
+    {
+        $e = new Exception('This should reach the main error handler');
+        $res = $this->createMock(ResponseInterface::class);
+        $cb = function ($req, $ex) use ($e, $res) {
+            $this->assertSame($e, $ex, 'A different exception reached the handler');
+
+            return $res;
+        };
+
+        $handler = $this->createMock(ErrorHandlerInterface::class);
+        $handler->expects($this->once())
+            ->method('handle')
+            ->will($this->returnCallback($cb));
+
+        $dispatcher = new Dispatcher();
+        $this->assertSame(
+            $dispatcher,
+            $dispatcher->setErrorHandler($handler),
+            'setErrorHandler should return $this'
+        );
+
+        $endpoint = $this->getMockEndpoint();
+        $endpoint->method('execute')
+            ->will($this->throwException($e));
+        $endpoint->expects($this->once())
+            ->method('handleException')
+            ->with($e)
+            ->will($this->throwException($e));
+        $this->executeMockRequestOnEndpoint($endpoint, $dispatcher, ServerRequestInterface::class);
+    }
+
+    /**
+     * This is a sort of BC-prevention test: in v4, the Dispatcher will only
+     * accept a ServerRequestInterface instead of the base-level
+     * RequestInterface. The new error handler is typehinted as such. This
+     * checks that if the base class was provided to the dispatcher, it
+     * shouldn't attempt to use the error handler since it would just result in
+     * a TypeError. This will be removed in v4.
+     *
+     * @covers ::dispatch
+     */
+    public function testExceptionsLeakWhenRequestIsBaseClass()
+    {
+        $e = new Exception('This should reach the top level');
+
+        $handler = $this->createMock(ErrorHandlerInterface::class);
+        $handler->expects($this->never())
+            ->method('handle');
+
+        $dispatcher = new Dispatcher();
+        $dispatcher->setErrorHandler($handler);
+
+        $endpoint = $this->getMockEndpoint();
+        $endpoint->method('execute')
+            ->will($this->throwException($e));
+        $endpoint->expects($this->once())
+            ->method('handleException')
+            ->with($e)
+            ->will($this->throwException($e));
+        try {
+            $this->executeMockRequestOnEndpoint($endpoint, $dispatcher);
+            $this->fail('An exception should have been thrown');
+        } catch (Throwable $t) {
+            $this->assertSame($e, $t);
+        }
+    }
+
+    /** @covers ::dispatch */
+    public function testExceptionsLeakWhenNoErrorHandler()
+    {
+        $e = new Exception('This should reach the top level');
+
+        $endpoint = $this->getMockEndpoint();
+        $endpoint->method('execute')
+            ->will($this->throwException($e));
+        // This is a quasi-v4 endpoint: one where the endpoint's exception
+        // handler just rethrows the exception. This should be the same as not
+        // choosing to have an endpoint handle exeptions directly in v4.
+        $endpoint->expects($this->once())
+            ->method('handleException')
+            ->with($e)
+            ->will($this->throwException($e));
+
+        try {
+            $this->executeMockRequestOnEndpoint($endpoint);
+            $this->fail('An exception should have been thrown');
+        } catch (Throwable $t) {
+            $this->assertSame($e, $t, 'A different exception was thrown');
+        }
+    }
+
     // ----(Helper methods)----------------------------------------------------
 
     /**
@@ -510,12 +609,14 @@ private function checkResponse(ResponseInterface $response, int $expected_code)
      * @param string $uri path component of URI
      * @param string $method optional HTTP method
      * @param array $query_data optional raw, unescaped query string data
+     * @param string $requestClass What RequestInterface to mock
      * @return RequestInterface | \PHPUnit\Framework\MockObject\MockObject
      */
     private function getMockRequestWithUriPath(
         string $uri,
         string $method = 'GET',
-        array $query_data = []
+        array $query_data = [],
+        string $requestClass = RequestInterface::class
     ): RequestInterface {
         $mock_uri = $this->createMock(UriInterface::class);
         $mock_uri->expects($this->any())
@@ -524,7 +625,8 @@ private function getMockRequestWithUriPath(
         $mock_uri->method('getQuery')
             ->will($this->returnValue(http_build_query($query_data)));
 
-        $req = $this->createMock(RequestInterface::class);
+        /** @var RequestInterface | \PHPUnit\Framework\MockObject\MockObject */
+        $req = $this->createMock($requestClass);
 
         $req->expects($this->any())
             ->method('getUri')
@@ -557,17 +659,24 @@ private function getMockEndpoint(): EndpointInterface
      * Run the endpointwith an empty request
      *
      * @param EndpointInterface $endpoint the endpoint to test
+     * @param ?Dispatcher $dispatcher a configured dispatcher
      * @return ResponseInterface the endpoint response
      */
-    private function executeMockRequestOnEndpoint(EndpointInterface $endpoint): ResponseInterface
-    {
-        $req = $this->getMockRequestWithUriPath('/container', 'GET');
+    private function executeMockRequestOnEndpoint(
+        EndpointInterface $endpoint,
+        Dispatcher $dispatcher = null,
+        string $requestClass = RequestInterface::class
+    ): ResponseInterface {
+        $req = $this->getMockRequestWithUriPath('/container', 'GET', [], $requestClass);
         $list = [
             'GET' => [
                 '/container' => 'ClassThatDoesNotExist',
             ],
         ];
-        $response = (new Dispatcher())
+        if (!$dispatcher) {
+            $dispatcher = new Dispatcher();
+        }
+        $response = $dispatcher
             ->setContainer($this->getMockContainer(['ClassThatDoesNotExist' => $endpoint]))
             ->setEndpointList($list)
             ->setParserList($this->getDefaultParserList())