ENHANCEMENT Ensure 'trace' is captured for fatal non-exceptions #1080
Conversation
|
Hi @Seldaek I didn't want to bother you over the holiday! :) Would you be able to please have a look at this ticket and give feedback? |
src/Monolog/ErrorHandler.php
Outdated
| // fatal error codes are ignored if a fatal error handler is present as well to avoid duplicate log entries | ||
| if (!$this->hasFatalErrorHandler || !in_array($code, self::$fatalErrors, true)) { | ||
| $level = isset($this->errorLevelMap[$code]) ? $this->errorLevelMap[$code] : LogLevel::CRITICAL; | ||
| $this->logger->log($level, self::codeToString($code).': '.$message, array('code' => $code, 'message' => $message, 'file' => $file, 'line' => $line)); | ||
| $this->logger->log($level, self::codeToString($code).': '.$message, array('code' => $code, 'message' => $message, 'file' => $file, 'line' => $line, 'trace' => $trace)); |
There was a problem hiding this comment.
To add the trace on fatals probably make sense as those are really bad, but I am not sure if it makes sense to add it to every error logged.. this includes notices, warnings etc and will make the logged data grow quite substantially. I don't think I want to do such a big change on 1.x which is very stable at this point.
There was a problem hiding this comment.
Thanks for the feedback; I'll look at perhaps a 2.x level change, and investigate the use of trace in non-error contexts.
tractorcow
force-pushed
the
pulls/1/error-trace
branch
from
d787aed
to
13f5794
Compare
|
Hi @Seldaek I have re-written the PR with a preference for respecting the stability of the current branch, over trying to be precise. In this vein, I've opted to only add traces for fatal errors handled via hasFatalErrorHandler, as this was the least risky enhancement I could make. Handling hasFatalErrorHandler = false but isFatal = true might have added risky complexity. A single additional trace on shutdown is unlikely to greatly bloat any existing log. I will likely follow up with a 2.x suggested alternative, but if you are happy for this to go into 1.x, it would solve the immediate need for providing traces for fatal errors in most applications. :) |
tractorcow
changed the title
tractorcow
force-pushed
the
pulls/1/error-trace
branch
from
13f5794
to
2d9c905
Compare
|
It was necessary to revert the tests, as the only behavioural modifications were on shutdown, and were nigh-untestable (without exposing more API). |
|
Hi @Seldaek just checking back in on you. Do you think the latest update addresses your concerns? I'm happy if the answer is no, I just want to know whether or not we can close this issue. :) Thanks for your help up until now. |
|
Thanks |
|
Thanks very much @Seldaek our project just got easier to debug thanks to you. |
Fixes #693
See discussion on this ticket.