diff --git a/system/Log/Handlers/ChromeLoggerHandler.php b/system/Log/Handlers/ChromeLoggerHandler.php index 7e21926c4e2d..5d15e5c50890 100644 --- a/system/Log/Handlers/ChromeLoggerHandler.php +++ b/system/Log/Handlers/ChromeLoggerHandler.php @@ -96,15 +96,13 @@ public function __construct(array $config = []) /** * Handles logging the message. - * If the handler returns false, then execution of handlers - * will stop. Any handlers that have not run, yet, will not - * be run. + * Always lets the remaining handlers run. * * @param string $level * @param object|string $message * @param array $context */ - public function handle($level, $message, array $context = []): bool + public function handle($level, $message, array $context = []): int { $message = $this->format($message); @@ -129,7 +127,7 @@ public function handle($level, $message, array $context = []): bool $this->sendLogs(); - return true; + return self::RESULT_CONTINUE; } /** diff --git a/system/Log/Handlers/ErrorlogHandler.php b/system/Log/Handlers/ErrorlogHandler.php index 52f9add8cb5d..7a38b57021bb 100644 --- a/system/Log/Handlers/ErrorlogHandler.php +++ b/system/Log/Handlers/ErrorlogHandler.php @@ -62,15 +62,13 @@ public function __construct(array $config = []) /** * Handles logging the message. - * If the handler returns false, then execution of handlers - * will stop. Any handlers that have not run, yet, will not - * be run. + * Always lets the remaining handlers run, even if `error_log()` fails. * * @param string $level * @param string $message * @param array $context */ - public function handle($level, $message, array $context = []): bool + public function handle($level, $message, array $context = []): int { if ($context !== []) { $message .= ' ' . $this->encodeContext($context); @@ -78,7 +76,9 @@ public function handle($level, $message, array $context = []): bool $message = strtoupper($level) . ' --> ' . $message . "\n"; - return $this->errorLog($message, $this->messageType); + $this->errorLog($message, $this->messageType); + + return self::RESULT_CONTINUE; } /** diff --git a/system/Log/Handlers/FileHandler.php b/system/Log/Handlers/FileHandler.php index 99bdc2b20113..fc06c7ac056c 100644 --- a/system/Log/Handlers/FileHandler.php +++ b/system/Log/Handlers/FileHandler.php @@ -65,9 +65,8 @@ public function __construct(array $config = []) /** * Handles logging the message. - * If the handler returns false, then execution of handlers - * will stop. Any handlers that have not run, yet, will not - * be run. + * Always lets the remaining handlers run, even if writing + * the log file fails. * * @param string $level * @param string $message @@ -75,7 +74,7 @@ public function __construct(array $config = []) * * @throws Exception */ - public function handle($level, $message, array $context = []): bool + public function handle($level, $message, array $context = []): int { $filepath = $this->path . 'log-' . date('Y-m-d') . '.' . $this->fileExtension; @@ -92,7 +91,7 @@ public function handle($level, $message, array $context = []): bool } if (! $fp = @fopen($filepath, 'ab')) { - return false; + return self::RESULT_CONTINUE; } // Instantiating DateTime with microseconds appended to initial date is needed for proper support of this format @@ -130,6 +129,6 @@ public function handle($level, $message, array $context = []): bool @chmod($filepath, $this->filePermissions); } - return is_int($result); + return self::RESULT_CONTINUE; } } diff --git a/system/Log/Handlers/HandlerInterface.php b/system/Log/Handlers/HandlerInterface.php index b0f767fd2805..ad8d6b281407 100644 --- a/system/Log/Handlers/HandlerInterface.php +++ b/system/Log/Handlers/HandlerInterface.php @@ -25,18 +25,32 @@ interface HandlerInterface */ public const GLOBAL_CONTEXT_KEY = '_ci_context'; + /** + * Returned by `handle()` to let the remaining handlers run. + */ + public const RESULT_CONTINUE = 1; + + /** + * Returned by `handle()` to stop the chain. Any handlers that + * have not run, yet, will not be run. + */ + public const RESULT_STOP = 2; + /** * Handles logging the message. - * If the handler returns false, then execution of handlers - * will stop. Any handlers that have not run, yet, will not - * be run. + * Must return either RESULT_CONTINUE or RESULT_STOP. When RESULT_STOP + * is returned, execution of handlers will stop and any handlers that + * have not run, yet, will not be run. Any other value lets the + * remaining handlers run. * * @param string $level * @param string $message * @param array $context Full context array; may contain * GLOBAL_CONTEXT_KEY with CI global data + * + * @return int One of the RESULT_* constants */ - public function handle($level, $message, array $context = []): bool; + public function handle($level, $message, array $context = []): int; /** * Checks whether the Handler will handle logging items of this diff --git a/system/Log/Logger.php b/system/Log/Logger.php index 35336fe643d4..c1c11fb4ab2f 100644 --- a/system/Log/Logger.php +++ b/system/Log/Logger.php @@ -324,8 +324,8 @@ public function log($level, string|Stringable $message, array $context = []): vo continue; } - // If the handler returns false, then we don't execute any other handlers. - if (! $handler->setDateFormat($this->dateFormat)->handle($level, $message, $context)) { + // If the handler asks to stop, then we don't execute any other handlers. + if ($handler->setDateFormat($this->dateFormat)->handle($level, $message, $context) === HandlerInterface::RESULT_STOP) { break; } } diff --git a/tests/_support/Log/Handlers/TestHandler.php b/tests/_support/Log/Handlers/TestHandler.php index 9eaa3ef1ab15..6df13b93f044 100644 --- a/tests/_support/Log/Handlers/TestHandler.php +++ b/tests/_support/Log/Handlers/TestHandler.php @@ -58,22 +58,20 @@ public function __construct(array $config) /** * Handles logging the message. - * If the handler returns false, then execution of handlers - * will stop. Any handlers that have not run, yet, will not - * be run. + * Always lets the remaining handlers run. * * @param string $level * @param string $message * @param array $context */ - public function handle($level, $message, array $context = []): bool + public function handle($level, $message, array $context = []): int { $date = Time::now()->format($this->dateFormat); self::$logs[] = strtoupper($level) . ' - ' . $date . ' --> ' . $message; self::$contexts[] = $context; - return true; + return self::RESULT_CONTINUE; } public static function getLogs() diff --git a/tests/system/Log/Handlers/ChromeLoggerHandlerTest.php b/tests/system/Log/Handlers/ChromeLoggerHandlerTest.php index 000bf2ea9c78..62119c9dfe80 100644 --- a/tests/system/Log/Handlers/ChromeLoggerHandlerTest.php +++ b/tests/system/Log/Handlers/ChromeLoggerHandlerTest.php @@ -43,7 +43,7 @@ public function testHandle(): void $config->handlers['CodeIgniter\Log\Handlers\TestHandler']['handles'] = ['critical']; $logger = new ChromeLoggerHandler($config->handlers['CodeIgniter\Log\Handlers\TestHandler']); - $this->assertTrue($logger->handle('warning', 'This a log test')); + $this->assertSame(HandlerInterface::RESULT_CONTINUE, $logger->handle('warning', 'This a log test')); } public function testSendLogs(): void diff --git a/tests/system/Log/Handlers/ErrorlogHandlerTest.php b/tests/system/Log/Handlers/ErrorlogHandlerTest.php index 7aa89ab2a933..51c924c70aac 100644 --- a/tests/system/Log/Handlers/ErrorlogHandlerTest.php +++ b/tests/system/Log/Handlers/ErrorlogHandlerTest.php @@ -35,7 +35,7 @@ public function testErrorLoggingWithErrorLog(): void $logger = $this->getMockedHandler(['handles' => ['critical', 'error']]); $logger->method('errorLog')->willReturn(true); $logger->expects($this->once())->method('errorLog')->with("ERROR --> Test message.\n", 0); - $this->assertTrue($logger->handle('error', 'Test message.')); + $this->assertSame(HandlerInterface::RESULT_CONTINUE, $logger->handle('error', 'Test message.')); } public function testErrorLoggingAppendsContextAsJson(): void @@ -44,7 +44,7 @@ public function testErrorLoggingAppendsContextAsJson(): void $logger->method('errorLog')->willReturn(true); $logger->expects($this->once())->method('errorLog') ->with("ERROR --> Test message. {\"_ci_context\":{\"foo\":\"bar\"}}\n", 0); - $this->assertTrue($logger->handle('error', 'Test message.', [HandlerInterface::GLOBAL_CONTEXT_KEY => ['foo' => 'bar']])); + $this->assertSame(HandlerInterface::RESULT_CONTINUE, $logger->handle('error', 'Test message.', [HandlerInterface::GLOBAL_CONTEXT_KEY => ['foo' => 'bar']])); } /** diff --git a/tests/system/Log/Handlers/FileHandlerTest.php b/tests/system/Log/Handlers/FileHandlerTest.php index ab16dee5615f..b3eadd6f9434 100644 --- a/tests/system/Log/Handlers/FileHandlerTest.php +++ b/tests/system/Log/Handlers/FileHandlerTest.php @@ -43,7 +43,7 @@ public function testHandle(): void $logger = new MockFileLogger($config->handlers[TestHandler::class]); $logger->setDateFormat('Y-m-d H:i:s:u'); - $this->assertTrue($logger->handle('warning', 'This is a test log')); + $this->assertSame(HandlerInterface::RESULT_CONTINUE, $logger->handle('warning', 'This is a test log')); } public function testBasicHandle(): void @@ -56,7 +56,19 @@ public function testBasicHandle(): void $logger->setDateFormat('Y-m-d H:i:s:u'); $expected = 'log-' . date('Y-m-d') . '.log'; vfsStream::newFile($expected)->at(vfsStream::setup('root/charlie'))->withContent('This is a test log'); - $this->assertTrue($logger->handle('warning', 'This is a test log')); + $this->assertSame(HandlerInterface::RESULT_CONTINUE, $logger->handle('warning', 'This is a test log')); + } + + public function testHandleContinuesWhenFileCannotBeOpened(): void + { + $config = new LoggerConfig(); + $config->handlers[TestHandler::class]['path'] = $this->start . 'does-not-exist/'; + $config->handlers[TestHandler::class]['handles'] = ['critical']; + + $logger = new MockFileLogger($config->handlers[TestHandler::class]); + $logger->setDateFormat('Y-m-d H:i:s:u'); + + $this->assertSame(HandlerInterface::RESULT_CONTINUE, $logger->handle('warning', 'This is a test log')); } public function testHandleCreateFile(): void diff --git a/tests/system/Log/LoggerTest.php b/tests/system/Log/LoggerTest.php index 487a4d4b9b9e..7baf83b0750d 100644 --- a/tests/system/Log/LoggerTest.php +++ b/tests/system/Log/LoggerTest.php @@ -18,6 +18,8 @@ use CodeIgniter\Exceptions\RuntimeException; use CodeIgniter\I18n\Time; use CodeIgniter\Log\Exceptions\LogException; +use CodeIgniter\Log\Handlers\BaseHandler; +use CodeIgniter\Log\Handlers\HandlerInterface; use CodeIgniter\Test\CIUnitTestCase; use CodeIgniter\Test\Mock\MockLogger as LoggerConfig; use PHPUnit\Framework\Attributes\Group; @@ -95,6 +97,67 @@ public function testLogActuallyLogs(): void $this->assertSame($expected, $logs[0]); } + public function testLogRunsRemainingHandlersWhenAHandlerReturnsContinue(): void + { + $config = new LoggerConfig(); + $config->handlers = [ + $this->getResultHandlerClass() => ['handles' => ['debug'], 'result' => HandlerInterface::RESULT_CONTINUE], + TestHandler::class => ['handles' => ['debug']], + ]; + + $logger = new Logger($config); + $logger->log('debug', 'Test message'); + + $this->assertCount(1, TestHandler::getLogs()); + } + + public function testLogStopsRunningHandlersWhenAHandlerReturnsStop(): void + { + $config = new LoggerConfig(); + $config->handlers = [ + $this->getResultHandlerClass() => ['handles' => ['debug'], 'result' => HandlerInterface::RESULT_STOP], + TestHandler::class => ['handles' => ['debug']], + ]; + + // Handlers are created lazily and TestHandler resets its logs when + // created, so reset them here as it will never be reached. + new TestHandler([]); + + $logger = new Logger($config); + $logger->log('debug', 'Test message'); + + $this->assertCount(0, TestHandler::getLogs()); + } + + /** + * Returns the class of a handler that returns the `result` value from its config. + * + * @return class-string + */ + private function getResultHandlerClass(): string + { + $handler = new class ([]) extends BaseHandler { + private int $result; + + /** + * @param array{handles?: list, result?: int} $config + */ + public function __construct(array $config) + { + parent::__construct($config); + + $this->result = $config['result'] ?? HandlerInterface::RESULT_CONTINUE; + } + + public function handle($level, $message, array $context = []): int + { + return $this->result; + } + }; + + return $handler::class; + } + public function testLogDoesnotLogUnhandledLevels(): void { $config = new LoggerConfig(); diff --git a/user_guide_src/source/changelogs/v4.8.0.rst b/user_guide_src/source/changelogs/v4.8.0.rst index 89d0c3c7e154..9665875e10c5 100644 --- a/user_guide_src/source/changelogs/v4.8.0.rst +++ b/user_guide_src/source/changelogs/v4.8.0.rst @@ -68,6 +68,7 @@ update your implementations to include the new methods or method changes to ensu - **HTTP:** ``CodeIgniter\HTTP\ResponseInterface`` now requires the ``stream()`` and ``eventStream()`` methods, which create streaming and SSE responses. See :ref:`streaming-responses`. - **HTTP:** ``CodeIgniter\HTTP\Files\UploadedFileInterface::move()`` now returns ``static`` instead of ``bool``. The previous ``bool`` return was incompatible with ``CodeIgniter\Files\File::move()``, which ``UploadedFile`` extends, so no implementation could satisfy both. See :doc:`../installation/upgrade_480` for the migration. - **Logging:** ``CodeIgniter\Log\Handlers\HandlerInterface::handle()`` now requires a third parameter ``array $context = []``. Any custom log handler that overrides ``handle()`` - whether implementing ``HandlerInterface`` directly or extending a built-in handler class - must add the parameter to its ``handle()`` method signature. +- **Logging:** ``CodeIgniter\Log\Handlers\HandlerInterface::handle()`` now returns ``int`` instead of ``bool``. Return ``HandlerInterface::RESULT_CONTINUE`` to let the remaining handlers run, or ``HandlerInterface::RESULT_STOP`` to stop the chain. Any custom log handler that overrides ``handle()`` must update its return type and return value. See :doc:`../installation/upgrade_480` for the migration. - **Security:** The ``SecurityInterface``'s ``verify()`` method now has a native return type of ``static``. - **Validation:** ``CodeIgniter\Validation\ValidationInterface`` now requires the ``getValidatedInput()`` method, which returns a ``CodeIgniter\Input\ValidatedInput`` instance. @@ -419,6 +420,7 @@ Changes ******* - **Config:** Added the ``md`` key for ``Config\Mimes::$mimes`` for Markdown files. +- **Logging:** The built-in log handlers (``FileHandler``, ``ErrorlogHandler``, ``ChromeLoggerHandler``) no longer stop the remaining handlers from running when they fail to write a log entry. Previously, a failing ``FileHandler`` (e.g. due to file permissions) silently prevented every handler configured after it from logging. ************ Deprecations diff --git a/user_guide_src/source/general/logging.rst b/user_guide_src/source/general/logging.rst index 6d241951adf7..65d7035888ff 100644 --- a/user_guide_src/source/general/logging.rst +++ b/user_guide_src/source/general/logging.rst @@ -81,6 +81,12 @@ Each handler's section will have one property in common: ``handles``, which is a .. literalinclude:: logging/004.php +The handlers run in the order they are listed. A handler's ``handle()`` method returns +``HandlerInterface::RESULT_CONTINUE`` to let the remaining handlers run, or +``HandlerInterface::RESULT_STOP`` to stop the chain. The built-in handlers always +continue, so a handler that fails (for example, a ``FileHandler`` that cannot write to +the log directory) does not prevent the handlers after it from logging. + Modifying the Message with Context ================================== diff --git a/user_guide_src/source/installation/upgrade_480.rst b/user_guide_src/source/installation/upgrade_480.rst index 7aebe713dc65..c2fece31103f 100644 --- a/user_guide_src/source/installation/upgrade_480.rst +++ b/user_guide_src/source/installation/upgrade_480.rst @@ -145,6 +145,43 @@ The context array may contain the CI global context data under the ``HandlerInterface::GLOBAL_CONTEXT_KEY`` (``'_ci_context'``) key when ``$logGlobalContext`` is enabled in ``Config\Logger``. +``HandlerInterface::handle()`` now also returns ``int`` instead of ``bool``. +The return value tells the ``Logger`` whether to run the remaining handlers: + +- ``HandlerInterface::RESULT_CONTINUE`` (``1``): the remaining handlers run. +- ``HandlerInterface::RESULT_STOP`` (``2``): the chain stops and the handlers + that have not run yet are skipped. + +Previously, returning ``false`` stopped the chain and ``true`` continued it. The +built-in handlers returned ``false`` when they failed to write, so a failing +``FileHandler`` prevented the handlers after it from logging. They now always +return ``RESULT_CONTINUE``. + +If you have a custom log handler that overrides ``handle()``, you must update the +return type, and return the new constants: + +.. code-block:: php + + // Before + public function handle($level, $message, array $context = []): bool + { + // ... + return true; // continue with the next handler + // return false; // stop the chain + } + + // After + public function handle($level, $message, array $context = []): int + { + // ... + return self::RESULT_CONTINUE; // continue with the next handler + // return self::RESULT_STOP; // stop the chain + } + +.. note:: A ``handle()`` method that still declares a ``bool`` return type is + incompatible with the interface and will cause a fatal error when the class + is loaded. + ************* Project Files *************