From 4468a46af23a9eb86536f6bd5e19c37d1b82ac8f Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Fri, 11 Oct 2013 21:22:40 +0200 Subject: [PATCH] exception: Use MWExceptionHandler::logException in more places Most code replaced wasn't exactly like what logException does but most probably should be. A few implementation differences with the code it replaced in various places: * MWException if-guards Was there only to prevent a crash because getLogMessage is an MWException method. Now that logException is generic, it seems sensible to start logging those as well (follows-up a97f3550a0). * Exception::getTraceAsString Now using MWExceptionHandler::formatRedactedTrace instead. It wasn't using it because that method didn't exist yet. Notes: * DatabaseError::getLogMessage Removed as this override was no longer doing anything (we're using MWExceptionHandler::getLogMessage instead of $e->getLogMessage). Introduced isLoggable() to take over the responsibility of indicating when an exception should not be logged (follows-up bcb9f9e1c0d). * DeferredUpdates and Wiki.php Both specificy MWException. Though ApiMain intends to catch all and only logged MWException because it couldn't otherwise, these actually only catch MWException (as opposed to catching all and having an if-statement inside). Left those as-is to have them continue propagate other exceptions. * JobQueueFederated and JobQueueGroup All specify to catch JobQueueError only. Not sure whether it should catch other exceptions. It now can, but I'll leave it as is in case it intends to have those be handled elsewhere (or fatal). Change-Id: I4578a0fe7d95a080f1a3b292ce7ae73a4d5fcaca --- includes/DeferredUpdates.php | 2 +- includes/Exception.php | 14 ++++++++++++-- includes/Wiki.php | 2 +- includes/api/ApiMain.php | 9 ++------- includes/db/DatabaseError.php | 14 +++++++------- includes/job/JobQueueGroup.php | 2 +- 6 files changed, 24 insertions(+), 19 deletions(-) diff --git a/includes/DeferredUpdates.php b/includes/DeferredUpdates.php index a321414ae5..c385f138e9 100644 --- a/includes/DeferredUpdates.php +++ b/includes/DeferredUpdates.php @@ -109,7 +109,7 @@ class DeferredUpdates { // be reported to the user since the output is already sent. // Instead we just log them. if ( !$e instanceof ErrorPageError ) { - wfDebugLog( 'exception', $e->getLogMessage() ); + MWExceptionHandler::logException( $e ); } } } diff --git a/includes/Exception.php b/includes/Exception.php index 46402f9290..582ad9304a 100644 --- a/includes/Exception.php +++ b/includes/Exception.php @@ -43,6 +43,16 @@ class MWException extends Exception { !empty( $GLOBALS['wgTitle'] ); } + /** + * Whether to log this exception in the exception debug log. + * + * @since 1.23 + * @return boolean + */ + function isLoggable() { + return true; + } + /** * Can the extension use the Message class/wfMessage to get i18n-ed messages? * @@ -801,8 +811,8 @@ class MWExceptionHandler { public static function logException( Exception $e ) { global $wgLogExceptionBacktrace; - $log = self::getLogMessage( $e ); - if ( $log ) { + if ( !( $e instanceof MWException ) || $e->isLoggable() ) { + $log = self::getLogMessage( $e ); if ( $wgLogExceptionBacktrace ) { wfDebugLog( 'exception', $log . "\n" . self::formatRedactedTrace( $e ) . "\n" ); } else { diff --git a/includes/Wiki.php b/includes/Wiki.php index 403ef110c6..08567ee355 100644 --- a/includes/Wiki.php +++ b/includes/Wiki.php @@ -688,7 +688,7 @@ class MediaWiki { // We don't want exceptions thrown during job execution to // be reported to the user since the output is already sent. // Instead we just log them. - wfDebugLog( 'exception', $e->getLogMessage() ); + MWExceptionHandler::logException( $e ); } } } diff --git a/includes/api/ApiMain.php b/includes/api/ApiMain.php index c10d85c44b..c11f16cba4 100644 --- a/includes/api/ApiMain.php +++ b/includes/api/ApiMain.php @@ -383,13 +383,8 @@ class ApiMain extends ApiBase { wfRunHooks( 'ApiMain::onException', array( $this, $e ) ); // Log it - if ( $e instanceof MWException && !( $e instanceof UsageException ) ) { - global $wgLogExceptionBacktrace; - if ( $wgLogExceptionBacktrace ) { - wfDebugLog( 'exception', $e->getLogMessage() . "\n" . $e->getTraceAsString() . "\n" ); - } else { - wfDebugLog( 'exception', $e->getLogMessage() ); - } + if ( !( $e instanceof UsageException ) ) { + MWExceptionHandler::logException( $e ); } // Handle any kind of exception by outputting properly formatted error message. diff --git a/includes/db/DatabaseError.php b/includes/db/DatabaseError.php index 937bea0fed..f14a5025e5 100644 --- a/includes/db/DatabaseError.php +++ b/includes/db/DatabaseError.php @@ -133,10 +133,10 @@ class DBConnectionError extends DBError { } /** - * @return bool + * @return boolean */ - function getLogMessage() { - // Don't send to the exception log + function isLoggable() { + // Don't send to the exception log, already in dberror log return false; } @@ -310,15 +310,15 @@ class DBQueryError extends DBError { } /** - * @return bool + * @return boolean */ - function getLogMessage() { - # Don't send to the exception log + function isLoggable() { + // Don't send to the exception log, already in dberror log return false; } /** - * @return String + * @return string */ function getPageTitle() { return $this->msg( 'databaseerror', 'Database error' ); diff --git a/includes/job/JobQueueGroup.php b/includes/job/JobQueueGroup.php index a20ea4af5d..fa7fee5faf 100644 --- a/includes/job/JobQueueGroup.php +++ b/includes/job/JobQueueGroup.php @@ -376,7 +376,7 @@ class JobQueueGroup { ++$count; } } catch ( JobQueueError $e ) { - wfDebugLog( 'exception', $e->getLogMessage() ); + MWExceptionHandler::logException( $e ); } } } -- 2.20.1