Further improve the ErrorLoggerInterface

Provide better method names, and better parameters. This should also
help reduce the need for error handling configuration to be used in
Loggers which feels like a win as userland code doesn't need to read
configuration data to make decisions on whether or not to include
a trace.

The new methods can't be added to the interface because of backwards
compatibility so I've annotated them and used `method_exists()`. This
isn't ideal but it does allow us to be backwards compatible.

src/Error/ErrorLogger.php

@@ -52,7 +52,62 @@ class ErrorLogger implements ErrorLoggerInterface
     }
 
     /**
-     * @inheritDoc
+     * Log an error to Cake's Log subsystem
+     *
+     * @param \Cake\Error\PhpError $error The error to log
+     * @param bool $includeTrace Should the log message include a stacktrace
+     * @param ?\Psr\Http\Message\ServerRequestInterface $request The request if in an HTTP context.
+     * @return bool Logging success
+     */
+    public function logError(PhpError $error, bool $includeTrace = false, ?ServerRequestInterface $request = null): bool
+    {
+        $message = $error->getMessage();
+        if ($request) {
+            $message .= $this->getRequestContext($request);
+        }
+        if ($includeTrace) {
+            $message .= "\nTrace:\n" . $error->getTraceAsString() . "\n";
+        }
+        $logMap = [
+            'strict' => LOG_NOTICE,
+            'deprecated' => LOG_NOTICE,
+        ];
+        $level = $error->getLabel();
+        $level = $logMap[$level] ?? $level;
+
+        return Log::write($level, $message);
+    }
+
+    /**
+     * Log an exception to Cake's Log subsystem
+     *
+     * @param \Throwable $exception The exception to log a message for.
+     * @param bool $includeTrace Whether or not a stack trace should be logged.
+     * @param \Psr\Http\Message\ServerRequestInterface|null $request The current request if available.
+     * @return bool
+     */
+    public function logException(
+        Throwable $exception,
+        bool $includeTrace = false,
+        ?ServerRequestInterface $request = null
+    ): bool {
+        $message = $this->getMessage($exception, false, $includeTrace);
+
+        if ($request !== null) {
+            $message .= $this->getRequestContext($request);
+        }
+
+        $message .= "\n\n";
+
+        return Log::error($message);
+    }
+
+    /**
+     * @param string|int $level The logging level
+     * @param string $message The message to be logged.
+     * @param array $context Context.
+     * @return bool
+     * @deprecated 4.4.0 Use logError instead.
      */
     public function logMessage($level, string $message, array $context = []): bool
     {
@@ -72,11 +127,14 @@ class ErrorLogger implements ErrorLoggerInterface
     }
 
     /**
-     * @inheritDoc
+     * @param \Throwable $exception The exception to log a message for.
+     * @param \Psr\Http\Message\ServerRequestInterface|null $request The current request if available.
+     * @return bool
+     * @deprecated 4.4.0 Use logException instead.
      */
     public function log(Throwable $exception, ?ServerRequestInterface $request = null): bool
     {
-        $message = $this->getMessage($exception);
+        $message = $this->getMessage($exception, false, $this->getConfig('trace'));
 
         if ($request !== null) {
             $message .= $this->getRequestContext($request);
@@ -92,9 +150,10 @@ class ErrorLogger implements ErrorLoggerInterface
      *
      * @param \Throwable $exception The exception to log a message for.
      * @param bool $isPrevious False for original exception, true for previous
+     * @param bool $includeTrace Whether or not to include a stack trace.
      * @return string Error message
      */
-    protected function getMessage(Throwable $exception, bool $isPrevious = false): string
+    protected function getMessage(Throwable $exception, bool $isPrevious = false, bool $includeTrace = false): string
     {
         $message = sprintf(
             '%s[%s] %s in %s on line %s',
@@ -113,7 +172,7 @@ class ErrorLogger implements ErrorLoggerInterface
             }
         }
 
-        if ($this->getConfig('trace')) {
+        if ($includeTrace) {
             /** @var array $trace */
             $trace = Debugger::formatTrace($exception, ['format' => 'points']);
             $message .= "\nStack Trace:\n";
@@ -128,7 +187,7 @@ class ErrorLogger implements ErrorLoggerInterface
 
         $previous = $exception->getPrevious();
         if ($previous) {
-            $message .= $this->getMessage($previous, true);
+            $message .= $this->getMessage($previous, true, $includeTrace);
         }
 
         return $message;

src/Error/ErrorLoggerInterface.php

@@ -24,6 +24,11 @@ use Throwable;
  *
  * Used by the ErrorHandlerMiddleware and global
  * error handlers to log exceptions and errors.
+ *
+ * @method bool logException(\Throwable $exception, bool $includeTrace = false, ?\Psr\Http\Message\ServerRequestInterface $request = null)
+ *   Log an exception with an optional HTTP request.
+ * @method bool logError(\Cake\Error\PhpError $error, bool $includeTrace = false, ?\Psr\Http\Message\ServerRequestInterface $request = null)
+ *   Log an error with an optional HTTP request.
  */
 interface ErrorLoggerInterface
 {

src/Error/ErrorTrap.php

@@ -127,20 +127,10 @@ class ErrorTrap
 
         $debug = Configure::read('debug');
         $renderer = $this->renderer();
-        $logger = $this->logger();
 
         try {
             // Log first incase rendering or event listeners fail
-            if ($this->_config['log']) {
-                $context = [];
-                if ($this->_config['trace']) {
-                    $context = [
-                        'trace' => $error->getTraceAsString(),
-                        'request' => Router::getRequest(),
-                    ];
-                }
-                $logger->logMessage($error->getLabel(), $error->getMessage(), $context);
-            }
+            $this->logError($error);
             $event = $this->dispatchEvent('Error.beforeRender', ['error' => $error]);
             if ($event->isStopped()) {
                 return true;
@@ -148,7 +138,7 @@ class ErrorTrap
             $renderer->write($renderer->render($error, $debug));
         } catch (Exception $e) {
             // Fatal errors always log.
-            $logger->logMessage('error', 'Could not render error. Got: ' . $e->getMessage());
+            $this->logger()->logMessage('error', 'Could not render error. Got: ' . $e->getMessage());
 
             return false;
         }
@@ -157,6 +147,37 @@ class ErrorTrap
     }
 
     /**
+     * Logging helper method.
+     *
+     * @param \Cake\Error\PhpError $error The error object to log.
+     * @return void
+     */
+    protected function logError(PhpError $error): void
+    {
+        if (!$this->_config['log']) {
+            return;
+        }
+        $logger = $this->logger();
+        if (method_exists($logger, 'logError')) {
+            $logger->logError($error, $this->_config['trace'], Router::getRequest());
+        } else {
+            $loggerClass = get_class($logger);
+            deprecationWarning(
+                "The configured logger `{$loggerClass}` does not implement `logError` " .
+                'which will be required in future versions of CakePHP.'
+            );
+            $context = [];
+            if ($this->_config['trace']) {
+                $context = [
+                    'trace' => $error->getTraceAsString(),
+                    'request' => Router::getRequest(),
+                ];
+            }
+            $logger->logMessage($error->getLabel(), $error->getMessage(), $context);
+        }
+    }
+
+    /**
      * Get an instance of the renderer.
      *
      * @return \Cake\Error\ErrorRendererInterface

src/Error/ExceptionTrap.php

@@ -355,7 +355,17 @@ class ExceptionTrap
             }
         }
         if ($shouldLog) {
-            $this->logger()->log($exception, $request);
+            $logger = $this->logger();
+            if (method_exists($logger, 'logException')) {
+                $logger->logException($exception, $this->_config['trace'], $request);
+            } else {
+                $loggerClass = get_class($logger);
+                deprecationWarning(
+                    "The configured logger `{$loggerClass}` should implement `logException()` " .
+                    'to be compatible with future versions of CakePHP.'
+                );
+                $this->logger()->log($exception, $request);
+            }
         }
         $this->dispatchEvent('Exception.beforeRender', ['exception' => $exception]);
     }

tests/TestCase/Error/ErrorTrapTest.php

@@ -24,10 +24,13 @@ use Cake\Error\PhpError;
 use Cake\Error\Renderer\ConsoleErrorRenderer;
 use Cake\Error\Renderer\HtmlErrorRenderer;
 use Cake\Error\Renderer\TextErrorRenderer;
+use Cake\Http\ServerRequest;
 use Cake\Log\Log;
+use Cake\Routing\Router;
 use Cake\TestSuite\TestCase;
 use InvalidArgumentException;
 use stdClass;
+use TestApp\Error\LegacyErrorLogger;
 
 class ErrorTrapTest extends TestCase
 {
@@ -36,6 +39,7 @@ class ErrorTrapTest extends TestCase
         parent::setUp();
 
         Log::drop('test_error');
+        Router::reload();
     }
 
     public function testConfigRendererInvalid()
@@ -186,6 +190,37 @@ class ErrorTrapTest extends TestCase
         $this->assertEmpty($logs);
     }
 
+    public function testHandleErrorLogDeprecatedLoggerMethods()
+    {
+        $request = new ServerRequest([
+            'url' => '/articles/view/1',
+        ]);
+        Router::setRequest($request);
+
+        Log::setConfig('test_error', [
+            'className' => 'Array',
+        ]);
+        $trap = new ErrorTrap([
+            'errorRenderer' => TextErrorRenderer::class,
+            'logger' => LegacyErrorLogger::class,
+            'log' => true,
+            'trace' => true,
+        ]);
+
+        $this->deprecated(function () use ($trap) {
+            // Calling this method directly as deprecated() interferes with registering
+            // an error handler.
+            ob_start();
+            $trap->handleError(E_USER_WARNING, 'Oh no it was bad', __FILE__, __LINE__);
+            ob_get_clean();
+        });
+
+        $logs = Log::engine('test_error')->read();
+        $this->assertStringContainsString('Oh no it was bad', $logs[0]);
+        $this->assertStringContainsString('IncludeTrace', $logs[0]);
+        $this->assertStringContainsString('URL=http://localhost/articles/view/1', $logs[0]);
+    }
+
     public function testRegisterNoOutputDebug()
     {
         Log::setConfig('test_error', [

tests/TestCase/Error/ExceptionTrapTest.php

@@ -23,12 +23,14 @@ use Cake\Error\Renderer\ConsoleExceptionRenderer;
 use Cake\Error\Renderer\TextExceptionRenderer;
 use Cake\Error\Renderer\WebExceptionRenderer;
 use Cake\Http\Exception\MissingControllerException;
+use Cake\Http\ServerRequest;
 use Cake\Log\Log;
 use Cake\TestSuite\Stub\ConsoleOutput;
 use Cake\TestSuite\TestCase;
 use Cake\Utility\Text;
 use InvalidArgumentException;
 use stdClass;
+use TestApp\Error\LegacyErrorLogger;
 use Throwable;
 
 class ExceptionTrapTest extends TestCase
@@ -243,6 +245,28 @@ class ExceptionTrapTest extends TestCase
         $this->assertEmpty($logs);
     }
 
+    public function testLogExceptionDeprecatedLoggerMethods()
+    {
+        Log::setConfig('test_error', [
+            'className' => 'Array',
+        ]);
+        $trap = new ExceptionTrap([
+            'log' => true,
+            'logger' => LegacyErrorLogger::class,
+            'trace' => true,
+        ]);
+        $error = new InvalidArgumentException('nope');
+        $request = new ServerRequest(['url' => '/articles/view/1']);
+        $this->deprecated(function () use ($trap, $error, $request) {
+            $trap->logException($error, $request);
+        });
+
+        $logs = Log::engine('test_error')->read();
+        $this->assertStringContainsString('nope', $logs[0]);
+        $this->assertStringContainsString('IncludeTrace', $logs[0]);
+        $this->assertStringContainsString('URL=http://localhost/articles/view/1', $logs[0]);
+    }
+
     /**
      * @preserveGlobalState disabled
      * @runInSeparateProcess

tests/test_app/TestApp/Error/LegacyErrorLogger.php

@@ -0,0 +1,94 @@
+<?php
+declare(strict_types=1);
+
+/**
+ * CakePHP(tm) : Rapid Development Framework (https://cakephp.org)
+ * Copyright (c) Cake Software Foundation, Inc. (https://cakefoundation.org)
+ *
+ * Licensed under The MIT License
+ * For full copyright and license information, please see the LICENSE.txt
+ * Redistributions of files must retain the above copyright notice.
+ *
+ * @copyright     Copyright (c) Cake Software Foundation, Inc. (https://cakefoundation.org)
+ * @link          https://cakephp.org CakePHP(tm) Project
+ * @since         4.4.0
+ * @license       https://opensource.org/licenses/mit-license.php MIT License
+ */
+namespace TestApp\Error;
+
+use Cake\Core\InstanceConfigTrait;
+use Cake\Error\ErrorLoggerInterface;
+use Cake\Log\Log;
+use Psr\Http\Message\ServerRequestInterface;
+use Throwable;
+
+/**
+ * Test stub that only implements the interface as declared.
+ * Necessary to test the deprecated logging paths in ErrorTrap and ExceptionTrap
+ */
+class LegacyErrorLogger implements ErrorLoggerInterface
+{
+    use InstanceConfigTrait;
+
+    /**
+     * Default configuration values.
+     *
+     * - `trace` Should error logs include stack traces?
+     *
+     * @var array<string, mixed>
+     */
+    protected $_defaultConfig = [
+        'trace' => false,
+    ];
+
+    /**
+     * Constructor
+     *
+     * @param array<string, mixed> $config Config array.
+     */
+    public function __construct(array $config = [])
+    {
+        $this->setConfig($config);
+    }
+
+    /**
+     * @param string|int $level The logging level
+     * @param string $message The message to be logged.
+     * @param array $context Context.
+     * @return bool
+     */
+    public function logMessage($level, string $message, array $context = []): bool
+    {
+        if (!empty($context['request'])) {
+            $message .= ' URL=' . $context['request']->getUri();
+        }
+        if (!empty($context['trace'])) {
+            $message .= ' IncludeTrace';
+        }
+        $logMap = [
+            'strict' => LOG_NOTICE,
+            'deprecated' => LOG_NOTICE,
+        ];
+        $level = $logMap[$level] ?? $level;
+
+        return Log::write($level, $message);
+    }
+
+    /**
+     * @param \Throwable $exception The exception to log a message for.
+     * @param \Psr\Http\Message\ServerRequestInterface|null $request The current request if available.
+     * @return bool
+     */
+    public function log(Throwable $exception, ?ServerRequestInterface $request = null): bool
+    {
+        $message = $exception->getMessage();
+        if ($request !== null) {
+            $message .= 'URL=' . $request->getUri();
+        }
+        if ($this->getConfig('trace')) {
+            $message .= 'IncludeTrace';
+        }
+
+        return Log::error($message);
+    }
+}