Browse Source

Inject ErrorHandler instance into ErrorHandlerMiddleware.

This avoids code duplication and those who want to customize error handler
would require only overriding ErrorHandler.
ADmad 6 years ago
parent
commit
4fe753ceb5

+ 1 - 1
src/Error/BaseErrorHandler.php

@@ -331,7 +331,7 @@ abstract class BaseErrorHandler
      *
      * @return \Cake\Error\ErrorLogger
      */
-    protected function getLogger()
+    public function getLogger()
     {
         if ($this->logger === null) {
             /** @var \Cake\Error\ErrorLogger $logger */

+ 30 - 4
src/Error/ErrorHandler.php

@@ -131,7 +131,6 @@ class ErrorHandler extends BaseErrorHandler
     {
         try {
             $renderer = $this->getRenderer(
-                $this->_options['exceptionRenderer'],
                 $exception,
                 Router::getRequest()
             );
@@ -144,20 +143,47 @@ class ErrorHandler extends BaseErrorHandler
     }
 
     /**
+     * Handles exception logging
+     *
+     * @param \Throwable $exception Exception instance.
+     * @return bool
+     */
+    protected function _logException(Throwable $exception): bool
+    {
+        return $this->logException($exception);
+    }
+
+    /**
+     * Log an error for the exception if applicable.
+     *
+     * @param \Throwable $exception The exception to log a message for.
+     * @param \Psr\Http\Message\ServerRequestInterface $request The current request.
+     * @return bool
+     */
+    public function logException(Throwable $exception, ?ServerRequestInterface $request = null): bool
+    {
+        $config = $this->_options;
+        if (empty($config['log'])) {
+            return false;
+        }
+
+        return $this->getLogger()->log($exception, $request ?? Router::getRequest());
+    }
+
+    /**
      * Get a renderer instance.
      *
-     * @param string|callable $renderer The renderer or class name
-     *   to use or a callable factory.
      * @param \Throwable $exception The exception being rendered.
      * @param \Psr\Http\Message\ServerRequestInterface|null $request The request.
      * @return \Cake\Error\ExceptionRendererInterface The exception renderer.
      * @throws \Exception When the renderer class cannot be found.
      */
     public function getRenderer(
-        $renderer,
         Throwable $exception,
         ?ServerRequestInterface $request = null
     ): ExceptionRendererInterface {
+        $renderer = $this->_options['exceptionRenderer'];
+
         if (is_string($renderer)) {
             $class = App::className($renderer, 'Error');
             if (!$class) {

+ 29 - 63
src/Error/Middleware/ErrorHandlerMiddleware.php

@@ -18,11 +18,9 @@ namespace Cake\Error\Middleware;
 
 use Cake\Core\App;
 use Cake\Core\InstanceConfigTrait;
-use Cake\Error\ErrorLogger;
+use Cake\Error\ErrorHandler;
 use Cake\Error\ExceptionRenderer;
-use Cake\Error\ExceptionRendererInterface;
 use Cake\Http\Response;
-use Exception;
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
 use Psr\Http\Server\MiddlewareInterface;
@@ -51,7 +49,9 @@ class ErrorHandlerMiddleware implements MiddlewareInterface
      *   ```
      *
      * - `trace` Should error logs include stack traces?
-     * - `errorLogger` The error logger class. Defaults to \Cake\Error\ErrorLogger
+     * - `exceptionRenderer` The renderer instance or class name to use or a callable factory
+     *   which return \Cake\Error\ExceptionRendererInterface instance.
+     *   Defaults to \Cake\Error\ExceptionRenderer
      *
      * @var array
      */
@@ -59,36 +59,31 @@ class ErrorHandlerMiddleware implements MiddlewareInterface
         'skipLog' => [],
         'log' => true,
         'trace' => false,
-        'errorLogger' => ErrorLogger::class,
+        'exceptionRenderer' => ExceptionRenderer::class,
     ];
 
     /**
-     * Exception render.
+     * Error handler instance.
      *
-     * @var \Cake\Error\ExceptionRendererInterface|callable|string
+     * @var \Cake\Error\ErrorHandler|null
      */
-    protected $exceptionRenderer;
+    protected $errorHandler;
 
     /**
      * Constructor
      *
-     * @param string|callable|null $exceptionRenderer The renderer or class name
-     *   to use or a callable factory. If null, value of 'exceptionRenderer' key
-     *   from $config will be used with fallback to Cake\Error\ExceptionRenderer::class.
-     * @param array $config Configuration options to use.
-     *   will be used.
+     * @param \Cake\Error\ErrorHandler|null $errorHandler The error handler.
+     * @param array $config Configuration options for error handler.
+     *   These options will be ignored if an ErrorHandler instance is passed as
+     *   first argument.
      */
-    public function __construct($exceptionRenderer = null, array $config = [])
+    public function __construct($errorHandler = null, array $config = [])
     {
         $this->setConfig($config);
 
-        if ($exceptionRenderer) {
-            $this->exceptionRenderer = $exceptionRenderer;
-
-            return;
+        if ($errorHandler !== null) {
+            $this->errorHandler = $errorHandler;
         }
-
-        $this->exceptionRenderer = $this->getConfig('exceptionRenderer', ExceptionRenderer::class);
     }
 
     /**
@@ -116,12 +111,13 @@ class ErrorHandlerMiddleware implements MiddlewareInterface
      */
     public function handleException(Throwable $exception, ServerRequestInterface $request): ResponseInterface
     {
-        $renderer = $this->getRenderer($exception, $request);
+        $errorHandler = $this->getErrorHandler();
+
         try {
-            $response = $renderer->render();
-            $this->logException($request, $exception);
+            $response = $errorHandler->getRenderer($exception, $request)->render();
+            $errorHandler->logException($exception, $request);
         } catch (Throwable $internalException) {
-            $this->logException($request, $internalException);
+            $errorHandler->logException($internalException, $request);
             $response = $this->handleInternalError();
         }
 
@@ -141,49 +137,19 @@ class ErrorHandlerMiddleware implements MiddlewareInterface
     }
 
     /**
-     * Get a renderer instance
-     *
-     * @param \Throwable $exception The exception being rendered.
-     * @param \Psr\Http\Message\ServerRequestInterface $request The request.
-     * @return \Cake\Error\ExceptionRendererInterface The exception renderer.
-     * @throws \Exception When the renderer class cannot be found.
-     */
-    protected function getRenderer(Throwable $exception, ServerRequestInterface $request): ExceptionRendererInterface
-    {
-        if (is_string($this->exceptionRenderer)) {
-            $class = App::className($this->exceptionRenderer, 'Error');
-            if (!$class) {
-                throw new Exception(sprintf(
-                    "The '%s' renderer class could not be found.",
-                    $this->exceptionRenderer
-                ));
-            }
-
-            /** @var \Cake\Error\ExceptionRendererInterface */
-            return new $class($exception, $request);
-        }
-
-        /** @var callable $factory */
-        $factory = $this->exceptionRenderer;
-
-        return $factory($exception, $request);
-    }
-
-    /**
-     * Log an error for the exception if applicable.
+     * Get a error handler instance
      *
-     * @param \Psr\Http\Message\ServerRequestInterface $request The current request.
-     * @param \Throwable $exception The exception to log a message for.
-     * @return void
+     * @return \Cake\Error\ErrorHandler The error handler.
      */
-    protected function logException(ServerRequestInterface $request, Throwable $exception): void
+    protected function getErrorHandler(): ErrorHandler
     {
-        if (!$this->getConfig('log')) {
-            return;
+        if ($this->errorHandler === null) {
+            /** @var string $className */
+            $className = App::className('ErrorHandler', 'Error');
+            /** @var \Cake\Error\ErrorHandler $this->errorHandler */
+            $this->errorHandler = new $className($this->getConfig());
         }
 
-        $loggerClass = $this->getConfig('errorLogger');
-        $logger = new $loggerClass($this->getConfig());
-        $logger->log($exception, $request);
+        return $this->errorHandler;
     }
 }

+ 14 - 0
tests/TestCase/Error/ErrorHandlerTest.php

@@ -104,6 +104,20 @@ class ErrorHandlerTest extends TestCase
     }
 
     /**
+     * Test an invalid rendering class.
+     *
+     * @return void
+     */
+    public function testInvalidRenderer()
+    {
+        $this->expectException(\Exception::class);
+        $this->expectExceptionMessage('The \'TotallyInvalid\' renderer class could not be found');
+
+        $errorHandler = new ErrorHandler(['exceptionRenderer' => 'TotallyInvalid']);
+        $errorHandler->getRenderer(new \Exception('Something bad'));
+    }
+
+    /**
      * test error handling when debug is on, an error should be printed from Debugger.
      *
      * @return void

+ 7 - 19
tests/TestCase/Error/Middleware/ErrorHandlerMiddlewareTest.php

@@ -16,6 +16,7 @@ declare(strict_types=1);
  */
 namespace Cake\Test\TestCase\Error\Middleware;
 
+use Cake\Error\ErrorHandler;
 use Cake\Error\ExceptionRendererInterface;
 use Cake\Error\Middleware\ErrorHandlerMiddleware;
 use Cake\Http\Response;
@@ -80,23 +81,6 @@ class ErrorHandlerMiddlewareTest extends TestCase
     }
 
     /**
-     * Test an invalid rendering class.
-     *
-     */
-    public function testInvalidRenderer()
-    {
-        $this->expectException(\Exception::class);
-        $this->expectExceptionMessage('The \'TotallyInvalid\' renderer class could not be found');
-        $request = ServerRequestFactory::fromGlobals();
-
-        $middleware = new ErrorHandlerMiddleware('TotallyInvalid');
-        $handler = new TestRequestHandler(function ($req) {
-            throw new \Exception('Something bad');
-        });
-        $middleware->process($request, $handler);
-    }
-
-    /**
      * Test using a factory method to make a renderer.
      *
      * @return void
@@ -117,7 +101,9 @@ class ErrorHandlerMiddlewareTest extends TestCase
 
             return $mock;
         };
-        $middleware = new ErrorHandlerMiddleware($factory);
+        $middleware = new ErrorHandlerMiddleware(new ErrorHandler([
+            'exceptionRenderer' => $factory,
+        ]));
         $handler = new TestRequestHandler(function ($req) {
             throw new LogicException('Something bad');
         });
@@ -310,7 +296,9 @@ class ErrorHandlerMiddlewareTest extends TestCase
 
             return $mock;
         };
-        $middleware = new ErrorHandlerMiddleware($factory);
+        $middleware = new ErrorHandlerMiddleware(new ErrorHandler([
+            'exceptionRenderer' => $factory,
+        ]));
         $handler = new TestRequestHandler(function ($req) {
             throw new \Cake\Http\Exception\ServiceUnavailableException('whoops');
         });