From b71f841552c1c47b1b6e2e760074567516dadacf Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Sun, 20 May 2018 18:02:24 -0700 Subject: [PATCH] Allow top-level error handler within dispatcher (#37) --- src/Dispatcher.php | 31 +++++- src/ErrorHandler.php | 5 + src/Interfaces/ErrorHandlerInterface.php | 18 ++++ tests/DispatcherTest.php | 127 +++++++++++++++++++++-- 4 files changed, 170 insertions(+), 11 deletions(-) create mode 100644 src/Interfaces/ErrorHandlerInterface.php diff --git a/src/Dispatcher.php b/src/Dispatcher.php index 0012bf3..24c2205 100644 --- a/src/Dispatcher.php +++ b/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); } diff --git a/src/ErrorHandler.php b/src/ErrorHandler.php index 68b8000..9611009 100644 --- a/src/ErrorHandler.php +++ b/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 */ diff --git a/src/Interfaces/ErrorHandlerInterface.php b/src/Interfaces/ErrorHandlerInterface.php new file mode 100644 index 0000000..0086a3e --- /dev/null +++ b/src/Interfaces/ErrorHandlerInterface.php @@ -0,0 +1,18 @@ +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())