Remove access to config globals from includes/exception

Allow callers of MWExceptionHandler::getStructuredExceptionData() and
jsonSerializeException() to explicitly control whether a backtrace is
included in the return value. This avoids the need to rely on the
LogExceptionBacktrace setting in static methods.

Bug: T294739
Change-Id: Ib941c22d6b7ec5f1b984bf5ded90652e42ad7b67

RELEASE-NOTES-1.39

@@ -166,6 +166,12 @@ because of Phabricator reports.
   to load configuration for. Using the WIKI_NAME environment variable for this
   purpose is deprecated. This is only relevant if you have been using
   $wgWikiFarmSettingsDirectory to load wiki farm config.
+* MWExceptionHandler::installHandler was marked @internal and had required
+  arguments added. This method is intended for use in bootstrap code and is
+  unused in known extensions.
+* MWException::useOutputPage was made private without deprecation.
+  This method was apparently only public for testing and is unused in known
+  extensions.
 * Calling getId() on a User or UserIdentityValue from the wrong wiki, deprecated
   since 1.36, now throws an exception.
 * IResultWrapper::next() now returns void, to match the Iterator interface that

includes/Setup.php

@@ -274,7 +274,7 @@ MediaWikiServices::allowGlobalInstance();
 // is complete.
 define( 'MW_SERVICE_BOOTSTRAP_COMPLETE', 1 );
 
-MWExceptionHandler::installHandler();
+MWExceptionHandler::installHandler( $wgLogExceptionBacktrace, $wgPropagateErrors );
 
 // Non-trivial validation of: $wgServer
 // The FatalError page only renders cleanly after MWExceptionHandler is installed.

includes/exception/MWException.php

@@ -17,6 +17,8 @@
  *
  * @file
  */
+use MediaWiki\MainConfigNames;
+use MediaWiki\MediaWikiServices;
 
 /**
  * MediaWiki exception
@@ -32,7 +34,8 @@ class MWException extends Exception {
 	 *
 	 * @return bool
 	 */
-	public function useOutputPage() {
+	private function useOutputPage() {
+		// NOTE: keep in sync with MWExceptionRenderer::useOutputPage
 		return $this->useMessageCache() &&
 		!empty( $GLOBALS['wgFullyInitialised'] ) &&
 		!empty( $GLOBALS['wgOut'] ) &&
@@ -79,7 +82,7 @@ class MWException extends Exception {
 	 * @return string Message with arguments replaced
 	 */
 	public function msg( $key, $fallback, ...$params ) {
-		// FIXME: Keep logic in sync with MWExceptionRenderer::msg.
+		// NOTE: Keep logic in sync with MWExceptionRenderer::msg.
 		$res = false;
 		if ( $this->useMessageCache() ) {
 			try {
@@ -98,6 +101,18 @@ class MWException extends Exception {
 		return $res;
 	}
 
+	private function shouldShowExceptionDetails(): bool {
+		// NOTE: keep in sync with MWExceptionRenderer::shouldShowExceptionDetails
+		if ( MediaWikiServices::hasInstance() ) {
+			$services = MediaWikiServices::getInstance();
+			if ( $services->hasService( 'MainConfig' ) ) {
+				return $services->getMainConfig()->get( MainConfigNames::ShowExceptionDetails );
+			}
+		}
+		global $wgShowExceptionDetails;
+		return $wgShowExceptionDetails ?? false;
+	}
+
 	/**
 	 * If $wgShowExceptionDetails is true, return a HTML message with a
 	 * backtrace to the error, otherwise show a message to ask to set it to true
@@ -108,9 +123,7 @@ class MWException extends Exception {
 	 * @return string Html to output
 	 */
 	public function getHTML() {
-		global $wgShowExceptionDetails;
-
-		if ( $wgShowExceptionDetails ) {
+		if ( self::shouldShowExceptionDetails() ) {
 			return '<p>' . nl2br( htmlspecialchars( MWExceptionHandler::getLogMessage( $this ) ) ) .
 			'</p><p>Backtrace:</p><p>' .
 			nl2br( htmlspecialchars( MWExceptionHandler::getRedactedTraceAsString( $this ) ) ) .
@@ -145,9 +158,7 @@ class MWException extends Exception {
 	 * @return string
 	 */
 	public function getText() {
-		global $wgShowExceptionDetails;
-
-		if ( $wgShowExceptionDetails ) {
+		if ( self::shouldShowExceptionDetails() ) {
 			return MWExceptionHandler::getLogMessage( $this ) .
 			"\nBacktrace:\n" . MWExceptionHandler::getRedactedTraceAsString( $this ) . "\n";
 		} else {

includes/exception/MWExceptionHandler.php

@@ -62,9 +62,33 @@ class MWExceptionHandler {
 	];
 
 	/**
-	 * Install handlers with PHP.
+	 * Whether exception data should include a backtrace.
+	 *
+	 * @var bool
 	 */
-	public static function installHandler() {
+	private static $logExceptionBacktrace = true;
+
+	/**
+	 * Whether to propagate errors to PHP's built-in handler.
+	 *
+	 * @var bool
+	 */
+	private static $propagateErrors;
+
+	/**
+	 * Install handlers with PHP.
+	 * @internal
+	 * @param bool $logExceptionBacktrace Whether error handlers should include a backtrace
+	 *        in the log.
+	 * @param bool $propagateErrors Whether errors should be propagated to PHP's built-in handler.
+	 */
+	public static function installHandler(
+		bool $logExceptionBacktrace = true,
+		bool $propagateErrors = true
+	) {
+		self::$logExceptionBacktrace = $logExceptionBacktrace;
+		self::$propagateErrors = $propagateErrors;
+
 		// This catches:
 		// * Exception objects that were explicitly thrown but not
 		//   caught anywhere in the application. This is rare given those
@@ -224,8 +248,6 @@ class MWExceptionHandler {
 		$file = null,
 		$line = null
 	) {
-		global $wgPropagateErrors;
-
 		// Map PHP error constant to a PSR-3 severity level.
 		// Avoid use of "DEBUG" or "INFO" levels, unless the
 		// error should evade error monitoring and alerts.
@@ -293,10 +315,10 @@ class MWExceptionHandler {
 		$e = new ErrorException( $prefix . $message, 0, $level, $file, $line );
 		self::logError( $e, 'error', $severity, self::CAUGHT_BY_HANDLER );
 
-		// If $wgPropagateErrors is true return false so PHP shows/logs the error normally.
-		// Ignore $wgPropagateErrors if track_errors is set
+		// If $propagateErrors is true return false so PHP shows/logs the error normally.
+		// Ignore $propagateErrors if track_errors is set
 		// (which means someone is counting on regular PHP error handling behavior).
-		return !( $wgPropagateErrors || ini_get( 'track_errors' ) );
+		return !( self::$propagateErrors || ini_get( 'track_errors' ) );
 	}
 
 	/**
@@ -581,16 +603,23 @@ TXT;
 	 * backtrace) derived from the given throwable. The backtrace information
 	 * will be redacted as per getRedactedTraceAsArray().
 	 *
-	 * @since 1.26
 	 * @param Throwable $e
 	 * @param string $catcher CAUGHT_BY_* class constant indicating what caught the error
+	 * @param bool|null $includeBacktrace Whether to include a backtrace. If null,
+	 *        the value of $includeBacktrace that was provided to installHandler() will be
+	 *        used.
+	 *
 	 * @return array
+	 * @since 1.26
 	 */
 	public static function getStructuredExceptionData(
 		Throwable $e,
-		$catcher = self::CAUGHT_BY_OTHER
+		$catcher = self::CAUGHT_BY_OTHER,
+		?bool $includeBacktrace = null
 	) {
-		global $wgLogExceptionBacktrace;
+		if ( $includeBacktrace === null ) {
+			$includeBacktrace = self::$logExceptionBacktrace;
+		}
 
 		$data = [
 			'id' => WebRequest::getRequestId(),
@@ -610,7 +639,7 @@ TXT;
 			$data['suppressed'] = true;
 		}
 
-		if ( $wgLogExceptionBacktrace ) {
+		if ( $includeBacktrace ) {
 			$data['backtrace'] = self::getRedactedTrace( $e );
 		}
 
@@ -674,16 +703,20 @@ TXT;
 	 * @param bool $pretty Add non-significant whitespace to improve readability (default: false).
 	 * @param int $escaping Bitfield consisting of FormatJson::.*_OK class constants.
 	 * @param string $catcher CAUGHT_BY_* class constant indicating what caught the error
+	 * @param bool|null $includeBacktrace Whether to include a backtrace. If null,
+	 *        the value of $includeBacktrace that was provided to installHandler() will be
+	 *        used.
 	 * @return string|false JSON string if successful; false upon failure
 	 */
 	public static function jsonSerializeException(
 		Throwable $e,
 		$pretty = false,
 		$escaping = 0,
-		$catcher = self::CAUGHT_BY_OTHER
+		$catcher = self::CAUGHT_BY_OTHER,
+		?bool $includeBacktrace = null
 	) {
 		return FormatJson::encode(
-			self::getStructuredExceptionData( $e, $catcher ),
+			self::getStructuredExceptionData( $e, $catcher, $includeBacktrace ),
 			$pretty,
 			$escaping
 		);

includes/exception/MWExceptionRenderer.php

@@ -18,6 +18,7 @@
  * @file
  */
 
+use MediaWiki\MainConfigNames;
 use MediaWiki\MediaWikiServices;
 use Wikimedia\AtEase;
 use Wikimedia\Rdbms\DBConnectionError;
@@ -33,14 +34,28 @@ class MWExceptionRenderer {
 	public const AS_RAW = 1; // show as text
 	public const AS_PRETTY = 2; // show as HTML
 
+	private static function shouldShowExceptionDetails(): bool {
+		// NOTE: keep in sync with MWException::shouldShowExceptionDetails
+		if ( MediaWikiServices::hasInstance() ) {
+			$services = MediaWikiServices::getInstance();
+			if ( $services->hasService( 'MainConfig' ) ) {
+				return $services->getMainConfig()->get( MainConfigNames::ShowExceptionDetails );
+			}
+		}
+
+		// Shouldn't happen, since Setup.php calls MediaWikiServices::allowGlobalInstance() before
+		// MWExceptionHandler::installHandler(). But we shouldn't just crash and if it does,
+		// we should return nicely and continue to report the original error.
+		return false;
+	}
+
 	/**
 	 * @param Throwable $e Original exception
 	 * @param int $mode MWExceptionExposer::AS_* constant
 	 * @param Throwable|null $eNew New throwable from attempting to show the first
 	 */
 	public static function output( Throwable $e, $mode, Throwable $eNew = null ) {
-		global $wgShowExceptionDetails;
-
+		$showExceptionDetails = self::shouldShowExceptionDetails();
 		if ( $e instanceof RequestTimeoutException && headers_sent() ) {
 			// Excimer's flag check happens on function return, so, a timeout
 			// can be thrown after exiting, say, `doPostOutputShutdown`, where
@@ -83,7 +98,7 @@ class MWExceptionRenderer {
 			self::header( 'Content-Type: text/html; charset=UTF-8' );
 			if ( $eNew ) {
 				$message = "MediaWiki internal error.\n\n";
-				if ( $wgShowExceptionDetails ) {
+				if ( $showExceptionDetails ) {
 					$message .= 'Original exception: ' .
 						MWExceptionHandler::getLogMessage( $e ) .
 						"\nBacktrace:\n" . MWExceptionHandler::getRedactedTraceAsString( $e ) .
@@ -97,7 +112,7 @@ class MWExceptionRenderer {
 						self::getShowBacktraceError( $e );
 				}
 				$message .= "\n";
-			} elseif ( $wgShowExceptionDetails ) {
+			} elseif ( $showExceptionDetails ) {
 				$message = MWExceptionHandler::getLogMessage( $e ) .
 					"\nBacktrace:\n" .
 					MWExceptionHandler::getRedactedTraceAsString( $e ) . "\n";
@@ -115,7 +130,7 @@ class MWExceptionRenderer {
 	 * @return bool Should the throwable use $wgOut to output the error?
 	 */
 	private static function useOutputPage( Throwable $e ) {
-		// Can the extension use the Message class/wfMessage to get i18n-ed messages?
+		// Can the exception use the Message class/wfMessage to get i18n-ed messages?
 		foreach ( $e->getTrace() as $frame ) {
 			if ( isset( $frame['class'] ) && $frame['class'] === LocalisationCache::class ) {
 				return false;
@@ -126,6 +141,7 @@ class MWExceptionRenderer {
 		// (e.g. we're in RL code on load.php) - the Skin system (and probably
 		// most of MediaWiki) won't work.
 
+		// NOTE: keep in sync with MWException::useOutputPage
 		return (
 			!empty( $GLOBALS['wgFullyInitialised'] ) &&
 			!empty( $GLOBALS['wgOut'] ) &&
@@ -142,19 +158,18 @@ class MWExceptionRenderer {
 	 * @param Throwable $e
 	 */
 	private static function reportHTML( Throwable $e ) {
-		global $wgOut;
-
 		if ( self::useOutputPage( $e ) ) {
-			$wgOut->prepareErrorPage( self::getExceptionTitle( $e ) );
+			$out = RequestContext::getMain()->getOutput();
+			$out->prepareErrorPage( self::getExceptionTitle( $e ) );
 
 			// Show any custom GUI message before the details
 			$customMessage = self::getCustomMessage( $e );
 			if ( $customMessage !== null ) {
-				$wgOut->addHTML( Html::element( 'p', [], $customMessage ) );
+				$out->addHTML( Html::element( 'p', [], $customMessage ) );
 			}
-			$wgOut->addHTML( self::getHTML( $e ) );
+			$out->addHTML( self::getHTML( $e ) );
 			// Content-Type is set by OutputPage::output
-			$wgOut->output();
+			$out->output();
 		} else {
 			self::header( 'Content-Type: text/html; charset=UTF-8' );
 			$pageTitle = self::msg( 'internalerror', 'Internal error' );
@@ -182,9 +197,8 @@ class MWExceptionRenderer {
 	 * @return string Html to output
 	 */
 	public static function getHTML( Throwable $e ) {
-		global $wgShowExceptionDetails;
-
-		if ( $wgShowExceptionDetails ) {
+		// XXX: do we need a parameter to control inclusion of exception details?
+		if ( self::shouldShowExceptionDetails() ) {
 			$html = Html::errorBox( "<p>" .
 				nl2br( htmlspecialchars( MWExceptionHandler::getLogMessage( $e ) ) ) .
 				'</p><p>Backtrace:</p><p>' .
@@ -223,7 +237,7 @@ class MWExceptionRenderer {
 	 * @return string Message with arguments replaced
 	 */
 	private static function msg( $key, $fallback, ...$params ) {
-		// FIXME: Keep logic in sync with MWException::msg.
+		// NOTE: Keep logic in sync with MWException::msg.
 		try {
 			$res = wfMessage( $key, ...$params )->text();
 		} catch ( Exception $e ) {
@@ -242,9 +256,8 @@ class MWExceptionRenderer {
 	 * @return string
 	 */
 	private static function getText( Throwable $e ) {
-		global $wgShowExceptionDetails;
-
-		if ( $wgShowExceptionDetails ) {
+		// XXX: do we need a parameter to control inclusion of exception details?
+		if ( self::shouldShowExceptionDetails() ) {
 			return MWExceptionHandler::getLogMessage( $e ) .
 				"\nBacktrace:\n" .
 				MWExceptionHandler::getRedactedTraceAsString( $e ) . "\n";
@@ -352,8 +365,9 @@ class MWExceptionRenderer {
 	 * @param Throwable $e
 	 */
 	private static function reportOutageHTML( Throwable $e ) {
-		global $wgShowExceptionDetails, $wgShowHostnames;
-
+		$mainConfig = MediaWikiServices::getInstance()->getMainConfig();
+		$showExceptionDetails = $mainConfig->get( 'ShowExceptionDetails' );
+		$showHostnames = $mainConfig->get( 'ShowHostnames' );
 		$sorry = htmlspecialchars( self::msg(
 			'dberr-problems',
 			'Sorry! This site is experiencing technical difficulties.'
@@ -363,7 +377,7 @@ class MWExceptionRenderer {
 			'Try waiting a few minutes and reloading.'
 		) );
 
-		if ( $wgShowHostnames ) {
+		if ( $showHostnames ) {
 			$info = str_replace(
 				'$1',
 				Html::element( 'span', [ 'dir' => 'ltr' ], $e->getMessage() ),
@@ -383,7 +397,7 @@ class MWExceptionRenderer {
 				'<style>body { font-family: sans-serif; margin: 0; padding: 0.5em 2em; }</style>' .
 				"</head><body><h1>$sorry</h1><p>$again</p><p><small>$info</small></p>";
 
-		if ( $wgShowExceptionDetails ) {
+		if ( $showExceptionDetails ) {
 			$html .= '<p>Backtrace:</p><pre>' .
 				htmlspecialchars( $e->getTraceAsString() ) . '</pre>';
 		}

tests/phpunit/includes/exception/MWExceptionTest.php

@@ -1,4 +1,7 @@
 <?php
+
+use Wikimedia\TestingAccessWrapper;
+
 /**
  * @author Antoine Musso
  * @copyright Copyright © 2013, Antoine Musso
@@ -27,7 +30,7 @@ class MWExceptionTest extends MediaWikiIntegrationTestCase {
 			'wgOut' => $outputPage,
 		] );
 
-		$e = new MWException();
+		$e = TestingAccessWrapper::newFromObject( new MWException() );
 		$this->assertEquals( $expected, $e->useOutputPage() );
 	}
 

tests/phpunit/unit/includes/exception/MWExceptionHandlerTest.php

@@ -150,11 +150,14 @@ TEXT;
 	 * @param string $key Name of the key to validate in the serialized JSON
 	 */
 	public function testJsonserializeexceptionKeys( $expectedKeyType, $exClass, $key ) {
-		// Make sure we log a backtrace:
-		$GLOBALS['wgLogExceptionBacktrace'] = true;
-
 		$json = json_decode(
-			MWExceptionHandler::jsonSerializeException( new $exClass() )
+			MWExceptionHandler::jsonSerializeException(
+				new $exClass(),
+				true,
+				0,
+				MWExceptionHandler::CAUGHT_BY_OTHER,
+				true
+			)
 		);
 		$this->assertObjectHasAttribute( $key, $json );
 		$this->assertSame( $expectedKeyType, gettype( $json->$key ), "Type of the '$key' key" );
@@ -182,9 +185,14 @@ TEXT;
 	 * @covers MWExceptionHandler::jsonSerializeException
 	 */
 	public function testJsonserializeexceptionBacktracingEnabled() {
-		$GLOBALS['wgLogExceptionBacktrace'] = true;
 		$json = json_decode(
-			MWExceptionHandler::jsonSerializeException( new Exception() )
+			MWExceptionHandler::jsonSerializeException(
+				new Exception(),
+				true,
+				0,
+				MWExceptionHandler::CAUGHT_BY_OTHER,
+				true
+			)
 		);
 		$this->assertObjectHasAttribute( 'backtrace', $json );
 	}
@@ -196,9 +204,14 @@ TEXT;
 	 * @covers MWExceptionHandler::jsonSerializeException
 	 */
 	public function testJsonserializeexceptionBacktracingDisabled() {
-		$GLOBALS['wgLogExceptionBacktrace'] = false;
 		$json = json_decode(
-			MWExceptionHandler::jsonSerializeException( new Exception() )
+			MWExceptionHandler::jsonSerializeException(
+				new Exception(),
+				true,
+				0,
+				MWExceptionHandler::CAUGHT_BY_OTHER,
+				false
+			)
 		);
 		$this->assertObjectNotHasAttribute( 'backtrace', $json );
 	}