-
Notifications
You must be signed in to change notification settings - Fork 54
Add a PSR-3 logger, and support any PSR-3 logger #265
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 3.x
Are you sure you want to change the base?
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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… 🤔
| if (version_compare(phpversion(), '5.4.0', '>=')) { | ||
| $backtrace = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 3); | ||
| } elseif (version_compare(phpversion(), '5.3.6', '>=')) { | ||
| $backtrace = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS); | ||
| } else { | ||
| $backtrace = debug_backtrace(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove this conditional logic as 5.4 is well below our support matrix.
| if (version_compare(phpversion(), '5.4.0', '>=')) { | ||
| $backtrace = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 3); | ||
| } elseif (version_compare(phpversion(), '5.3.6', '>=')) { | ||
| $backtrace = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS); | ||
| } else { | ||
| $backtrace = debug_backtrace(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I don't think we need this.
|
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. |
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