clue · SimonFrings · Jul 22, 2022 · Jul 6, 2022
diff --git a/docs/api/app.md b/docs/api/app.md
@@ -163,3 +163,58 @@ One of the main features of the `App` is middleware support.
 Middleware allows you to extract common functionality such as HTTP login, session handling or logging into reusable components.
 These middleware components can be added to both individual routes or globally to all registered routes.
 See [middleware documentation](middleware.md) for more details.
+
+## Error handling
+
+Each controller function needs to return a response object in order to send
+an HTTP response message. If the controller function throws an `Exception` (or
+`Throwable`) or returns any invalid type, the HTTP request will automatically be
+rejected with a `500 Internal Server Error` HTTP error response:
+
+```php
+<?php
+
+// …
+
+$app->get('/user', function () {
+    throw new BadMethodCallException();
+});
+```
+
+You can try out this example by sending an HTTP request like this:
+
+```bash hl_lines="2"
+$ curl -I http://localhost:8080/user
+HTTP/1.1 500 Internal Server Error
+…
+```
+
+Internally, the `App` will automatically add a default error handler by adding
+the [`ErrorHandler`](middleware.md#errorhandler) to the list of middleware used.
+You may also explicitly pass an [`ErrorHandler`](middleware.md#errorhandler)
+middleware to the `App` like this:
+
+```php title="public/index.php"
+<?php
+
+require __DIR__ . '/../vendor/autoload.php';
+
+$app = new FrameworkX\App(
+    new FrameworkX\ErrorHandler()
+);
+
+// Register routes here, see routing…
+
+$app->run();
+```
+
+> ⚠️ **Feature preview**
+>
+> Note that the [`ErrorHandler`](middleware.md#errorhandler) may currently only
+> be passed as a middleware instance and not as a middleware name to the `App`.
+
+By default, this error message contains only few details to the client to avoid
+leaking too much internal information.
+If you want to implement custom error handling, you're recommended to either
+catch any exceptions your own or use a custom [middleware handler](middleware.md)
+to catch any exceptions in your application.
diff --git a/docs/api/middleware.md b/docs/api/middleware.md
@@ -553,3 +553,18 @@ and also any requests that can not be routed.
 You can also combine global middleware handlers (think logging) with additional
 middleware handlers for individual routes (think authentication).
 Global middleware handlers will always be called before route middleware handlers.
+
+## Built-in middleware
+
+### ErrorHandler
+
+> ⚠️ **Feature preview**
+>
+> This is a feature preview, i.e. it might not have made it into the current beta.
+> Give feedback to help us prioritize.
+> We also welcome [contributors](../getting-started/community.md) to help out!
+
+X ships with a built-in `ErrorHandler` middleware that is responsible for handling
+errors and exceptions returned from following middleware and controllers.
+This default error handling can be configured through the [`App`](app.md).
+See [error handling](app.md#error-handling) for more details.
diff --git a/docs/api/response.md b/docs/api/response.md
@@ -553,10 +553,9 @@ int(42)
 ## Internal Server Error
 
 Each controller function needs to return a response object in order to send
-an HTTP response message.
-If the controller functions throws an `Exception` (or `Throwable`) or any other type, the
-HTTP request will automatically be rejected with a `500 Internal Server Error`
-HTTP error response:
+an HTTP response message. If the controller function throws an `Exception` (or
+`Throwable`) or returns any invalid type, the HTTP request will automatically be
+rejected with a `500 Internal Server Error` HTTP error response:
 
 ```php
 <?php
@@ -576,8 +575,5 @@ HTTP/1.1 500 Internal Server Error
 …
 ```
 
-This error message contains only few details to the client to avoid leaking
-internal information.
-If you want to implement custom error handling, you're recommended to either
-catch any exceptions your own or use a [middleware handler](middleware.md) to
-catch any exceptions in your application.
+This default error handling can be configured through the [`App`](app.md).
+See [error handling](app.md#error-handling) for more details.
diff --git a/src/App.php b/src/App.php
@@ -38,36 +38,42 @@ class App
      */
     public function __construct(...$middleware)
     {
-        $errorHandler = new ErrorHandler();
+        // new MiddlewareHandler([$fiberHandler, $accessLogHandler, $errorHandler, ...$middleware, $routeHandler])
+        $handlers = [];
 
         $container = new Container();
         if ($middleware) {
-            foreach ($middleware as $i => $handler) {
+            foreach ($middleware as $handler) {
                 if ($handler instanceof Container) {
                     $container = $handler;
-                    unset($middleware[$i]);
+                } elseif ($handler === ErrorHandler::class) {
+                    throw new \TypeError('ErrorHandler may currently only be passed as instance');
                 } elseif (!\is_callable($handler)) {
-                    $middleware[$i] = $container->callable($handler);
+                    $handlers[] = $container->callable($handler);
+                } else {
+                    $handlers[] = $handler;
                 }
             }
         }
 
-        // new MiddlewareHandler([$fiberHandler, $accessLogHandler, $errorHandler, ...$middleware, $routeHandler])
-        \array_unshift($middleware, $errorHandler);
+        // add default ErrorHandler as first handler unless it is already added explicitly
+        if (!($handlers[0] ?? null) instanceof ErrorHandler) {
+            \array_unshift($handlers, new ErrorHandler());
+        }
 
         // only log for built-in webserver and PHP development webserver by default, others have their own access log
         if (\PHP_SAPI === 'cli' || \PHP_SAPI === 'cli-server') {
-            \array_unshift($middleware, new AccessLogHandler());
+            \array_unshift($handlers, new AccessLogHandler());
         }
 
         // automatically start new fiber for each request on PHP 8.1+
         if (\PHP_VERSION_ID >= 80100) {
-            \array_unshift($middleware, new FiberHandler()); // @codeCoverageIgnore
+            \array_unshift($handlers, new FiberHandler()); // @codeCoverageIgnore
         }
 
         $this->router = new RouteHandler($container);
-        $middleware[] = $this->router;
-        $this->handler = new MiddlewareHandler($middleware);
+        $handlers[] = $this->router;
+        $this->handler = new MiddlewareHandler($handlers);
         $this->sapi = new SapiHandler();
     }
 

diff --git a/src/ErrorHandler.php b/src/ErrorHandler.php
@@ -8,7 +8,7 @@
 use React\Promise\PromiseInterface;
 
 /**
- * @internal
+ * @final
  */
 class ErrorHandler
 {
@@ -27,6 +27,7 @@ public function __construct()
      *     method never throws or resolves a rejected promise. If the next
      *     handler fails to return a valid response, it will be turned into a
      *     valid error response before returning.
+     * @throws void
      */
     public function __invoke(ServerRequestInterface $request, callable $next)
     {
@@ -105,6 +106,7 @@ private function coroutine(\Generator $generator): \Generator
         } while (true);
     } // @codeCoverageIgnore
 
+    /** @internal */
     public function requestNotFound(): ResponseInterface
     {
         return $this->htmlResponse(
@@ -114,6 +116,7 @@ public function requestNotFound(): ResponseInterface
         );
     }
 
+    /** @internal */
     public function requestMethodNotAllowed(array $allowedMethods): ResponseInterface
     {
         $methods = \implode('/', \array_map(function (string $method) { return '<code>' . $method . '</code>'; }, $allowedMethods));
@@ -125,6 +128,7 @@ public function requestMethodNotAllowed(array $allowedMethods): ResponseInterfac
         )->withHeader('Allow', \implode(', ', $allowedMethods));
     }
 
+    /** @internal */
     public function requestProxyUnsupported(): ResponseInterface
     {
         return $this->htmlResponse(
@@ -134,7 +138,7 @@ public function requestProxyUnsupported(): ResponseInterface
         );
     }
 
-    public function errorInvalidException(\Throwable $e): ResponseInterface
+    private function errorInvalidException(\Throwable $e): ResponseInterface
     {
         $where = ' in ' . $this->where($e->getFile(), $e->getLine());
         $message = '<code>' . $this->html->escape($e->getMessage()) . '</code>';
@@ -147,7 +151,7 @@ public function errorInvalidException(\Throwable $e): ResponseInterface
         );
     }
 
-    public function errorInvalidResponse($value): ResponseInterface
+    private function errorInvalidResponse($value): ResponseInterface
     {
         return $this->htmlResponse(
             Response::STATUS_INTERNAL_SERVER_ERROR,
@@ -157,7 +161,7 @@ public function errorInvalidResponse($value): ResponseInterface
         );
     }
 
-    public function errorInvalidCoroutine($value, string $file, int $line): ResponseInterface
+    private function errorInvalidCoroutine($value, string $file, int $line): ResponseInterface
     {
         $where = ' near or before '. $this->where($file, $line) . '.';
 
@@ -184,7 +188,7 @@ private function htmlResponse(int $statusCode, string $title, string ...$info):
         );
     }
 
-    public function describeType($value): string
+    private function describeType($value): string
     {
         if ($value === null) {
             return 'null';

diff --git a/tests/AppTest.php b/tests/AppTest.php
@@ -152,6 +152,95 @@ public function testConstructWithContainerAndMiddlewareClassNameAssignsCallableF
         $this->assertSame($container, $ref->getValue($routeHandler));
     }
 
+    public function testConstructWithErrorHandlerOnlyAssignsErrorHandlerAfterDefaultAccessLogHandler()
+    {
+        $errorHandler = new ErrorHandler();
+
+        $app = new App($errorHandler);
+
+        $ref = new ReflectionProperty($app, 'handler');
+        $ref->setAccessible(true);
+        $handler = $ref->getValue($app);
+
+        $this->assertInstanceOf(MiddlewareHandler::class, $handler);
+        $ref = new ReflectionProperty($handler, 'handlers');
+        $ref->setAccessible(true);
+        $handlers = $ref->getValue($handler);
+
+        if (PHP_VERSION_ID >= 80100) {
+            $first = array_shift($handlers);
+            $this->assertInstanceOf(FiberHandler::class, $first);
+        }
+
+        $this->assertCount(3, $handlers);
+        $this->assertInstanceOf(AccessLogHandler::class, $handlers[0]);
+        $this->assertSame($errorHandler, $handlers[1]);
+        $this->assertInstanceOf(RouteHandler::class, $handlers[2]);
+    }
+
+    public function testConstructWithErrorHandlerClassThrows()
+    {
+        $this->expectException(\TypeError::class);
+        $this->expectExceptionMessage('ErrorHandler may currently only be passed as instance');
+        new App(ErrorHandler::class);
+    }
+
+    public function testConstructWithContainerAndErrorHandlerAssignsErrorHandlerAfterDefaultAccessLogHandler()
+    {
+        $errorHandler = new ErrorHandler();
+
+        $app = new App(new Container(), $errorHandler);
+
+        $ref = new ReflectionProperty($app, 'handler');
+        $ref->setAccessible(true);
+        $handler = $ref->getValue($app);
+
+        $this->assertInstanceOf(MiddlewareHandler::class, $handler);
+        $ref = new ReflectionProperty($handler, 'handlers');
+        $ref->setAccessible(true);
+        $handlers = $ref->getValue($handler);
+
+        if (PHP_VERSION_ID >= 80100) {
+            $first = array_shift($handlers);
+            $this->assertInstanceOf(FiberHandler::class, $first);
+        }
+
+        $this->assertCount(3, $handlers);
+        $this->assertInstanceOf(AccessLogHandler::class, $handlers[0]);
+        $this->assertSame($errorHandler, $handlers[1]);
+        $this->assertInstanceOf(RouteHandler::class, $handlers[2]);
+    }
+
+    public function testConstructWithMiddlewareAndErrorHandlerAssignsGivenErrorHandlerAfterMiddlewareAndDefaultAccessLogHandlerAndErrorHandlerFirst()
+    {
+        $middleware = function (ServerRequestInterface $request, callable $next) { };
+        $errorHandler = new ErrorHandler();
+
+        $app = new App($middleware, $errorHandler);
+
+        $ref = new ReflectionProperty($app, 'handler');
+        $ref->setAccessible(true);
+        $handler = $ref->getValue($app);
+
+        $this->assertInstanceOf(MiddlewareHandler::class, $handler);
+        $ref = new ReflectionProperty($handler, 'handlers');
+        $ref->setAccessible(true);
+        $handlers = $ref->getValue($handler);
+
+        if (PHP_VERSION_ID >= 80100) {
+            $first = array_shift($handlers);
+            $this->assertInstanceOf(FiberHandler::class, $first);
+        }
+
+        $this->assertCount(5, $handlers);
+        $this->assertInstanceOf(AccessLogHandler::class, $handlers[0]);
+        $this->assertInstanceOf(ErrorHandler::class, $handlers[1]);
+        $this->assertNotSame($errorHandler, $handlers[1]);
+        $this->assertSame($middleware, $handlers[2]);
+        $this->assertSame($errorHandler, $handlers[3]);
+        $this->assertInstanceOf(RouteHandler::class, $handlers[4]);
+    }
+
     public function testRunWillReportListeningAddressAndRunLoopWithSocketServer()
     {
         $socket = @stream_socket_server('127.0.0.1:8080');
@@ -244,7 +333,7 @@ public function testRunWillRestartLoopUntilSocketIsClosed()
         $this->expectOutputRegex('/' . preg_quote('Warning: Loop restarted. Upgrade to react/async v4 recommended for production use.' . PHP_EOL, '/') . '$/');
         $app->run();
     }
-    
+
     /**
      * @requires function pcntl_signal
      * @requires function posix_kill
@@ -261,7 +350,7 @@ public function testRunWillStopWhenReceivingSigint()
         $this->expectOutputRegex('/' . preg_quote('Received SIGINT, stopping loop' . PHP_EOL, '/') . '$/');
         $app->run();
     }
-    
+
     /**
      * @requires function pcntl_signal
      * @requires function posix_kill

diff --git a/tests/ErrorHandlerTest.php b/tests/ErrorHandlerTest.php
@@ -441,7 +441,11 @@ public function testErrorInvalidExceptionReturnsError500(string $in, string $exp
 
         $line = __LINE__ + 1;
         $e = new \RuntimeException($in);
-        $response = $handler->errorInvalidException($e);
+
+        // $response = $handler->errorInvalidException($e);
+        $ref = new \ReflectionMethod($handler, 'errorInvalidException');
+        $ref->setAccessible(true);
+        $response = $ref->invoke($handler, $e);
 
         $this->assertStringContainsString("<title>Error 500: Internal Server Error</title>\n", (string) $response->getBody());
         $this->assertStringContainsString("<p>The requested page failed to load, please try again later.</p>\n", (string) $response->getBody());
@@ -494,7 +498,10 @@ public function testErrorInvalidResponseReturnsError500($value, string $name)
     {
         $handler = new ErrorHandler();
 
-        $response = $handler->errorInvalidResponse($value);
+        // $response = $handler->errorInvalidResponse($value);
+        $ref = new \ReflectionMethod($handler, 'errorInvalidResponse');
+        $ref->setAccessible(true);
+        $response = $ref->invoke($handler, $value);
 
         $this->assertStringContainsString("<title>Error 500: Internal Server Error</title>\n", (string) $response->getBody());
         $this->assertStringContainsString("<p>The requested page failed to load, please try again later.</p>\n", (string) $response->getBody());
@@ -511,7 +518,11 @@ public function testErrorInvalidCoroutineReturnsError500($value, string $name)
 
         $file = __FILE__;
         $line = __LINE__;
-        $response = $handler->errorInvalidCoroutine($value, $file, $line);
+
+        // $response = $handler->errorInvalidCoroutine($value, $file, $line);
+        $ref = new \ReflectionMethod($handler, 'errorInvalidCoroutine');
+        $ref->setAccessible(true);
+        $response = $ref->invoke($handler, $value, $file, $line);
 
         $this->assertStringContainsString("<title>Error 500: Internal Server Error</title>\n", (string) $response->getBody());
         $this->assertStringContainsString("<p>The requested page failed to load, please try again later.</p>\n", (string) $response->getBody());