Improve logging #1633
Improve logging #1633
Conversation
Having a custom view implies that a friendly message is displayed to the user, in which case we can bet that the exception won't need to be "debugged" per se.
This takes place only in the FallbackExceptionHandler. Having a custom exception handler implies that a friendly message is displayed in the API response, in which case we can bet that the exception won't need to be "debugged" per se.
|
|
||
| if (! $this->debug) { | ||
| $this->logger->error($e); | ||
| } |
There was a problem hiding this comment.
Why only log in debug mode?
There was a problem hiding this comment.
My thought was that in debug mode you can see the error displayed when you encounter it, and therefore no need to log it. I suppose it'd be simpler to just log everything regardless though.
There was a problem hiding this comment.
On second thought, errors that occur in the forum stack don't get logged when debug mode is on, because they go through the HandleErrorsWithWhoops middleware. Whatever we do, it should be consistent between the forum/API stacks. Should we add logging to the Whoops middleware or keep things as-is?
There was a problem hiding this comment.
Let's always use the log, it makes it easier when errors aren't clearly visible when developing. Although this does not always happen in our own code, it might still happen in third party extensions and without logs the cause of white pages etc might be less clear. Let's stick to always logging.
There was a problem hiding this comment.
Yes, I agree. Always logging means a) no surprises and b) persistence.
There was a problem hiding this comment.
I am fine with this.
What I would like to see in beta.9:
- One middleware (always enabled) that converts all known exception types with custom views to pretty error pages, always.
- Another middleware (either Whoops or pretty pages) that a) displays the exceptions and b) logs them. (The logging could even be a separate middleware that re-throws the exceptions, unless that messes with the stack traces.)
IMO, there is no need for NotFound and similar exceptions to ever be handled with the fallback error handling (Whoops or pretty pages) - we are just using the exception mechanism to handle known error cases.
Fixes #1619
Fixes #991
Changes proposed in this pull request:
See commit messages for more details.
This is not a definitive implementation for optimal error handling and logging. Ideally we should have a more advanced error handling solution like the one referenced in #1619. But I thought it would be good to implement a least-resistance stopgap in the meantime.
Reviewers should focus on:
Is this acceptable for the purposes of beta 8.
Confirmed
Frontend changes: tested on a local Flarum installation.php vendor/bin/phpunit).