Add a PSR-3 logger, and support any PSR-3 logger#265
Add a PSR-3 logger, and support any PSR-3 logger#265pbowyer wants to merge 17 commits intomodxcms:3.xfrom
Conversation
`initConfig()` sorts out the data, so we don't have to look at `$options` when setting up the logger
|
Thanks for this work, Peter! I will check this out as soon as possible and get some feedback to you. |
opengeek
left a comment
opengeek
left a comment
There was a problem hiding this comment.
I'm still considering the necessity of the new methods on the xPDO class. Are these potential overkill? I am wondering if we could simply activate it if a PSR-3 logger was provided in the container… 🤔
|
I agree, how about something like this: 3.x...pbowyer:xpdo:3.x-psr-logger-take2 Or were you thinking more pared back? |
Yeah, that is looking more like what I had in mind. |
resolveLogLocation() in src/xPDO/xPDO.php was added but never used. Keeping it around increases maintenance surface and risk: its logic overwrote any backtrace-derived file with $_SERVER['SCRIPT_NAME'], which would produce misleading log locations if it were ever called. Remove the dead helper to avoid confusion and prevent accidental future use of incorrect location resolution.
When callers omit $file/$line, xPDO::_log() resolves the log location using debug_backtrace(). The default debug_backtrace() captures the full stack (including args), which is unnecessary overhead when we only need the immediate caller's file/line. Use debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 3) to keep the same frame indexing used by the existing logic while avoiding deep/arg-heavy traces. This preserves the resulting file/line values in log output while reducing CPU/memory cost.
|
Thanks @opengeek, I've cleaned it up further and merged it into this branch |
Extend the change in c6ab4d9 to xPDOLogger
|
I've also done another "What if..." branch: 3.x...pbowyer:xpdo:3.x-psr-logger-take3 This lets the I've included an optional BC break in a separate commit which removes |
|
Lovely to see this moving! |
|
Nice work @pbowyer ! |
4 participants
Context and history
This PR builds on previous efforts to finally resolve those constraints while ensuring 100% backward compatibility.
Goals
Implementation
I have integrated psr/log with a simple default implementation. The logic follows a "opt-in" flow:
I’ve included exhaustive tests in xPDOLoggingHistoricTest.php to ensure no regressions from the current behaviour.
I started by looking at @Mark-H's branch but I wanted to preserve full backwards compatibility, so used it as inspiration rather than building directly on it. In my code MODX logging works exactly as it currently does, until a custom PSR logger is provided. Once a custom PSR logger is provided, it's assumed that you want PSR-3 output and you opt out of the traditional MODX logging.
Request for feedback
xPDO::$logger. Is this acceptable for ease of use, or should we strictly require the service container for access?xPDOthan I wished to, and there is some duplication to support the current xPDO logging behaviours. Is this level of duplication acceptable to keep the "legacy" side untouched?xPDOLoggerhas a dependency onxPDO. It works, but it feels unclean. Would you prefer a different approach?Remaining Todos
xPDOLoggingHistoricTest.phpandxPDOLoggerTest.phpneed rationalising. I am satisfied thatxPDOLoggingHistoricTest.phpis exhaustive