Changes for phpcs

Author: pbowyer

_build/test/Tests/Model/TestLogger.php

@@ -0,0 +1,30 @@
+<?php
+/*
+ * This file is part of the MODX Revolution package.
+ *
+ * Copyright (c) MODX, LLC
+ *
+ * For complete copyright and license information, see the COPYRIGHT and LICENSE
+ * files found in the top-level directory of this distribution.
+ *
+ * @package modx-test
+ */
+
+namespace MODX\Revolution\Tests\Model;
+
+use Psr\Log\AbstractLogger;
+
+class TestLogger extends AbstractLogger
+{
+    /** @var array */
+    public $records = [];
+
+    public function log($level, $message, array $context = []): void
+    {
+        $this->records[] = [
+            'level' => $level,
+            'message' => $message,
+            'context' => $context,
+        ];
+    }
+}

_build/test/Tests/Model/modXLoggingTest.php

@@ -9,11 +9,11 @@
  *
  * @package modx-test
  */
+
 namespace MODX\Revolution\Tests\Model;
 
 use MODX\Revolution\modX;
 use MODX\Revolution\MODxTestCase;
-use Psr\Log\AbstractLogger;
 use Psr\Log\LoggerInterface;
 use Psr\Log\LogLevel;
 use xPDO\xPDO;
@@ -427,18 +427,3 @@ public function testPsrLoggerStringifiesNonStringMessages()
         $this->assertStringContainsString('value', $logger->records[0]['message']);
     }
 }
-
-class TestLogger extends AbstractLogger
-{
-    /** @var array */
-    public $records = [];
-
-    public function log($level, $message, array $context = []): void
-    {
-        $this->records[] = [
-            'level' => $level,
-            'message' => $message,
-            'context' => $context,
-        ];
-    }
-}

draft-pr.md

@@ -0,0 +1,82 @@
+# PSR-3 logging support in MODX
+
+## What this does
+
+MODX currently can't use standard PHP logging tools like Monolog. The `modX::_log()` override bypasses xPDO's new PSR-3 support (commit `55fd34318a`) in several code paths, so a logger registered on xPDO never sees all of MODX's log output.
+
+This PR adds `setLogger(LoggerInterface $logger)` to modX, restructures `_log()` to dispatch to a PSR-3 logger when one is set, and adds 20 new tests (11 baseline, 9 PSR-3).
+
+```php
+use Monolog\Logger;
+use Monolog\Handler\StreamHandler;
+
+$logger = new Logger('modx');
+$logger->pushHandler(new StreamHandler('/path/to/modx.log', Logger::DEBUG));
+$logger->pushProcessor(new Monolog\Processor\IntrospectionProcessor());
+
+$modx->setLogger($logger);
+$modx->setLogLevel(modX::LOG_LEVEL_DEBUG); // control what reaches the logger
+```
+
+## Backward compatibility
+
+When no PSR-3 logger is registered (`$this->logger === null`, the default), all logging behavior is identical to the previous implementation. 11 baseline tests verify this.
+
+## Design decisions and alternatives
+
+Three questions came up during implementation that have meaningful trade-offs. The current implementation reflects one set of choices, but these could reasonably go a different way.
+
+### 1. Should the PSR-3 logger respect modX's `logLevel`, or receive everything?
+
+**Options considered:**
+
+- **A. Respect `logLevel` (chosen):** The PSR-3 logger only sees messages that pass the `logLevel` threshold. Users set `$modx->setLogLevel(modX::LOG_LEVEL_DEBUG)` when they want full visibility, and can filter further in their logger config (e.g. Monolog handler levels).
+
+- **B. Send everything to the logger:** The PSR-3 logger receives all messages regardless of `logLevel`. The logger's own filtering decides what to keep.
+
+**Why A was chosen:** Performance. Every `log()` call that isn't filtered out early triggers `debug_backtrace()` in the legacy path to resolve the calling file and line. The early `logLevel` check (an integer comparison) skips all that work for messages below threshold. With option B and the default `LOG_LEVEL_ERROR`, every DEBUG/INFO/WARN call would still build context and dispatch to the logger. The trade-off is one extra line of config (`setLogLevel`) when setting up a logger.
+
+**If you'd prefer B:** We remove the early return at the top of `_log()` for the PSR-3 path, or add a separate threshold property for the PSR-3 logger. Note that this also interacts with decision 2 below; if the backtrace is skipped for the PSR-3 path (as it is now), the performance cost of option B is much lower.
+
+### 2. Should `debug_backtrace()` be called for the PSR-3 path?
+
+**Options considered:**
+
+- **A. Skip backtrace for PSR-3 (chosen):** The `file` and `line` context fields are only populated if the caller explicitly passed them (e.g. `__FILE__`, `__LINE__`). When not provided, they're empty strings. Monolog users add `IntrospectionProcessor` for automatic caller info.
+
+- **B. Always resolve backtrace:** Every log call resolves the backtrace so that `file`/`line` are always present in the PSR-3 context, matching legacy behavior.
+
+**Why A was chosen:** `debug_backtrace()` is the most expensive operation in the log path. Monolog's `IntrospectionProcessor` does the same thing but only for messages that survive handler-level filtering — so a DEBUG message that gets discarded by the handler never triggers a backtrace. This gives users better control over the performance/detail trade-off.
+
+**If you'd prefer B:** Move the backtrace resolution to before the `_dispatchToPsrLogger()` call, so the PSR-3 context always has file/line. Simpler for users who don't want to configure Monolog processors, but costs a backtrace on every dispatched log call.
+
+### 3. Should the PSR-3 logger replace legacy output, or be additive?
+
+**Options considered:**
+
+- **A. Replace legacy output (chosen):** When a PSR-3 logger is set, legacy FILE/ECHO/HTML/ARRAY output is suppressed. Only modRegister logging is preserved alongside the PSR-3 logger.
+
+- **B. Additive (both):** Both the PSR-3 logger and legacy targets receive messages. The existing `error.log` continues alongside the custom logger.
+
+- **C. Configurable:** Add an option (e.g. `psr_logger_exclusive`) to let users choose at runtime.
+
+**Why A was chosen:** If you've set up Monolog, you're taking control of where logs go. Continuing to write to the legacy `error.log` is confusing and potentially wasteful. modRegister is preserved because it's a message queue mechanism, not traditional logging.
+
+**If you'd prefer B:** In the restructured `_log()`, remove the early return after the PSR-3 dispatch (lines 2989-2995 in the current code) and let execution fall through to the legacy path. The `$this->logger = null` / `try/finally` trick already exists for the legacy `parent::_log()` call and would prevent double-dispatch.
+
+**If you'd prefer C:** Add a config option check before the early return, e.g.:
+```php
+if ($hasPsrLogger && $this->getOption('psr_logger_exclusive', null, true)) {
+    // skip legacy
+    return;
+}
+// fall through to legacy path
+```
+
+### Other notes
+
+- **xPDOLogger exclusion:** The bundled `xPDOLogger` (standalone xPDO's default logger) is explicitly excluded from modX's PSR-3 dispatch. Its `handleXpdo()` method calls `exit()` for FATAL, while modX uses `sendError('fatal')` which renders a proper error page. xPDOLogger is intended for standalone xPDO only.
+
+- **FATAL handling:** modX continues to use `sendError('fatal')` for fatal log messages, not `exit()`. The PSR-3 logger receives the FATAL message (mapped to `LogLevel::CRITICAL`) before `sendError()` is called.
+
+- **Backward compatibility:** When no PSR-3 logger is set (`$this->logger === null`, the default), all behavior is identical to the previous implementation. The baseline tests verify this.