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
Admin app - catch exceptions thrown by JLog::add() #13504
Conversation
Can you fix conflicts here please? |
Tests/reviews here please? Logging shouldn't be a blocking operation, this is one step toward that. |
} | ||
catch (RuntimeException $exception) | ||
{ | ||
JFactory::getApplication()->enqueueMessage( |
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.
Is it not better to call setError here? Especially given this is largely being used for non-html dumps and the view can format handle appropriately
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.
Well remember, most views have a if $this->get('Errors')
style check which basically turns the view into an error page if there are errors found. The current handling of this method doesn't seem to indicate it should cause that to happen.
} | ||
catch (RuntimeException $exception) | ||
{ | ||
JFactory::getApplication()->enqueueMessage($error, 'warning'); |
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.
same this file
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.
This one already has an error set, look above; the $error
value comes from $this->getError()
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.
Then do we need to do anything other than pass here?
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.
If this weren't logging to the jerror
category I'd just leave a no-op comment. But since the app bootstrap sets up a logger for that category to throw messages in the message queue, I'm doing the same thing in this (and others doing the same) catch case.
Partial Pull Request for Issue #13357
Summary of Changes
Logging is not an action that should result in an unhandled error and the user seeing an error page, so catch if a
RuntimeException
is thrown through the course of callingJLog::add()
. This exception may be thrown ifJLog::addLogEntry()
cannot create theJLogLogger
object or an implementation ofJLogLogger::addEntry()
throws one on a fail state (i.e. the database logger failing to connect to the database or insert the record, or the formatted text logger failing to write to the filesystem).Testing Instructions
For all cases where logging to a category other than
jerror
is performed, the exception is just silently ignored. As these are informational log messages only (mainly deprecation notices), there is no need to trigger an error to the UI and notify the user of a failed log statement. For messages categorized asjerror
, the catch block will do exactly whatJLogLoggerMessagequeue::addEntry()
does and callJFactory::getApplication()->enqueueMessage()
.This should mean if you make your log directory unwritable, log messages triggered by code in the admin app should not result in the error page being triggered. You might have to programmatically issue a
chmod()
command before one of these log statements and reset it after to adequately test this until all log statements are wrapped in try/catch.Documentation Changes Required
N/A