Browse Source

4.4 - De-duplicate skipLog logic

By moving skipLog logic into `ExceptionTrap` we don't need to have two
copies of it. I've also added more helpful docs to the default
configuration of ExceptionTrap so that it is present in the API docs.
Mark Story 3 years ago
parent
commit
4825a00097

+ 1 - 0
src/Error/ExceptionRendererInterface.php

@@ -23,6 +23,7 @@ use Psr\Http\Message\ResponseInterface;
  *
  * @method \Psr\Http\Message\ResponseInterface|string render() Render the exception to a string or Http Response.
  * @method void write(\Psr\Http\Message\ResponseInterface|string $output) Write the output to the output stream.
+ *  This method is only called when exceptions are handled by a global default exception handler.
  */
 interface ExceptionRendererInterface
 {

+ 30 - 16
src/Error/ExceptionTrap.php

@@ -34,8 +34,22 @@ class ExceptionTrap
     }
 
     /**
-     * See the `Error` key in you `config/app.php`
-     * for details on the keys and their values.
+     * Configuration options. Generally these will be defined in your config/app.php
+     *
+     * - `trace` - boolean - Whether or not backtraces should be included in
+     *   logged exceptions.
+     * - `logger` - string - The class name of the error logger to use.
+     * - `exceptionRenderer` - string - The class responsible for rendering uncaught exceptions.
+     *   The chosen class will be used for for both CLI and web environments. If  you want different
+     *   classes used in CLI and web environments you'll need to write that conditional logic as well.
+     *   The conventional location for custom renderers is in `src/Error`. Your exception renderer needs to
+     *   implement the `render()` method and return either a string or Http\Response.
+     * - `skipLog` - array - List of exceptions to skip for logging. Exceptions that
+     *   extend one of the listed exceptions will also be skipped for logging.
+     *   E.g.: `'skipLog' => ['Cake\Http\Exception\NotFoundException', 'Cake\Http\Exception\UnauthorizedException']`
+     * - `extraFatalErrorMemory` - int - The number of megabytes to increase the memory limit by
+     *   when a fatal error is encountered. This allows
+     *   breathing room to complete logging or error handling.
      *
      * @var array<string, mixed>
      */
@@ -222,17 +236,7 @@ class ExceptionTrap
         }
         $request = Router::getRequest();
 
-        $shouldLog = true;
-        foreach ($this->_config['skipLog'] as $class) {
-            if ($exception instanceof $class) {
-                $shouldLog = false;
-                break;
-            }
-        }
-
-        if ($shouldLog) {
-            $this->logException($exception, $request);
-        }
+        $this->logException($exception, $request);
 
         try {
             $renderer = $this->renderer($exception);
@@ -326,9 +330,10 @@ class ExceptionTrap
      * Log an exception.
      *
      * Primarily a public function to ensure consistency between global exception handling
-     * and the ErrorHandlerMiddleware.
+     * and the ErrorHandlerMiddleware. This method will apply the `skipLog` filter
+     * skipping logging if the exception should not be logged.
      *
-     * After logging, the `Exception.beforeRender` event is triggered.
+     * After logging is attempted the `Exception.beforeRender` event is triggered.
      *
      * @param \Throwable $exception The exception to log
      * @param \Psr\Http\Message\ServerRequestInterface|null $request The optional request
@@ -337,7 +342,16 @@ class ExceptionTrap
     public function logException(Throwable $exception, ?ServerRequestInterface $request = null): void
     {
         $logger = $this->logger();
-        $logger->log($exception, $request);
+        $shouldLog = true;
+        foreach ($this->_config['skipLog'] as $class) {
+            if ($exception instanceof $class) {
+                $shouldLog = false;
+                break;
+            }
+        }
+        if ($shouldLog) {
+            $logger->log($exception, $request);
+        }
         $this->dispatchEvent('Exception.beforeRender', ['exception' => $exception]);
     }
 

+ 6 - 19
src/Error/Middleware/ErrorHandlerMiddleware.php

@@ -45,27 +45,14 @@ class ErrorHandlerMiddleware implements MiddlewareInterface
     /**
      * Default configuration values.
      *
-     * Ignored if contructor is passed an ErrorHandler instance.
+     * Ignored if contructor is passed an ExceptionTrap instance.
      *
-     * - `log` Enable logging of exceptions.
-     * - `skipLog` List of exceptions to skip logging. Exceptions that
-     *   extend one of the listed exceptions will also not be logged. Example:
-     *
-     *   ```
-     *   'skipLog' => ['Cake\Error\NotFoundException', 'Cake\Error\UnauthorizedException']
-     *   ```
-     *
-     * - `trace` Should error logs include stack traces?
-     * - `exceptionRenderer` The renderer instance or class name to use or a callable factory
-     *   which returns a \Cake\Error\ExceptionRendererInterface instance.
-     *   Defaults to \Cake\Error\ExceptionRenderer
+     * Configuration keys and values are shared with `ExceptionTrap`.
      *
      * @var array<string, mixed>
+     * @see \Cake\Error\ExceptionTrap
      */
     protected $_defaultConfig = [
-        'skipLog' => [],
-        'log' => true,
-        'trace' => false,
         'exceptionRenderer' => WebExceptionRenderer::class,
     ];
 
@@ -154,10 +141,10 @@ class ErrorHandlerMiddleware implements MiddlewareInterface
     public function handleException(Throwable $exception, ServerRequestInterface $request): ResponseInterface
     {
         if ($this->errorHandler === null) {
-            $errorHandler = $this->getExceptionTrap();
-            $errorHandler->logException($exception, $request);
+            $trap = $this->getExceptionTrap();
+            $trap->logException($exception, $request);
 
-            $renderer = $errorHandler->renderer($exception, $request);
+            $renderer = $trap->renderer($exception, $request);
         } else {
             $errorHandler = $this->getErrorHandler();
             $errorHandler->logException($exception, $request);

+ 7 - 0
tests/TestCase/Error/ExceptionTrapTest.php

@@ -38,6 +38,8 @@ class ExceptionTrapTest extends TestCase
      */
     private $memoryLimit;
 
+    private $triggered = false;
+
     public function setUp(): void
     {
         parent::setUp();
@@ -242,12 +244,17 @@ class ExceptionTrapTest extends TestCase
             'skipLog' => [InvalidArgumentException::class],
         ]);
 
+        $trap->getEventManager()->on('Exception.beforeRender', function () use (&$triggered) {
+            $this->triggered = true;
+        });
+
         ob_start();
         $trap->handleException(new InvalidArgumentException('nope'));
         ob_get_clean();
 
         $logs = Log::engine('test_error')->read();
         $this->assertEmpty($logs);
+        $this->assertTrue($this->triggered, 'Should have triggered event when skipping logging.');
     }
 
     public function testEventTriggered()