Merge "Deprecate error printing in MWException"

RELEASE-NOTES-1.42

@@ -369,6 +369,10 @@ because of Phabricator reports.
    - StaticUserOptionsLookup
 * API modules using ApiBase::requirePostedParameters() must also override
   mustBePosted() to return true.
+* Overriding MWException::getHTML(), ::getText(), ::getPageTitle(), and
+  ::reportHTML() in order to display custom exception messages is now
+  deprecated. Provide the error message when constructing the Exception,
+  or if you need a whole custom error page, use ErrorPageError instead.
 * Using `new ImportReporter( … )` or `new ApiImportReporter( … )` without
   the $context parameter is now deprecated.
 * Using `WikiImporterFactory::getWikiImporter()` without the $performer

includes/exception/ErrorPageError.php

@@ -76,7 +76,7 @@ class ErrorPageError extends MWException implements ILocalizedException {
 	 */
 	public function report( $action = self::SEND_OUTPUT ) {
 		if ( self::isCommandLine() || defined( 'MW_API' ) ) {
-			parent::report();
+			MWExceptionRenderer::output( $this, MWExceptionRenderer::AS_PRETTY );
 		} else {
 			global $wgOut;
 			$wgOut->showErrorPage( $this->title, $this->msg, $this->params );

includes/exception/MWException.php

@@ -106,12 +106,12 @@ class MWException extends Exception {
 	/**
 	 * Format an HTML message for the current exception object.
 	 *
-	 *
-	 * @stable to override
-	 * @todo Rarely used, remove in favour of generic MWExceptionRenderer
+	 * @deprecated since 1.42 Provide the error message when constructing the Exception instead.
+	 *   If you need a whole custom error page, use ErrorPageError instead.
 	 * @return string HTML to output
 	 */
 	public function getHTML() {
+		wfDeprecated( __METHOD__, '1.42' );
 		if ( MWExceptionRenderer::shouldShowExceptionDetails() ) {
 			return '<p>' . nl2br( htmlspecialchars( MWExceptionHandler::getLogMessage( $this ) ) ) .
 			'</p><p>Backtrace:</p><p>' .
@@ -140,11 +140,12 @@ class MWException extends Exception {
 	/**
 	 * Format plain text message for the current exception object.
 	 *
-	 * @stable to override
-	 * @todo Rarely used, remove in favour of generic MWExceptionRenderer
+	 * @deprecated since 1.42 Provide the error message when constructing the Exception instead.
+	 *   If you need a whole custom error page, use ErrorPageError instead.
 	 * @return string
 	 */
 	public function getText() {
+		wfDeprecated( __METHOD__, '1.42' );
 		if ( MWExceptionRenderer::shouldShowExceptionDetails() ) {
 			return MWExceptionHandler::getLogMessage( $this ) .
 			"\nBacktrace:\n" . MWExceptionHandler::getRedactedTraceAsString( $this ) . "\n";
@@ -157,20 +158,24 @@ class MWException extends Exception {
 	/**
 	 * Return the title of the page when reporting this error in a HTTP response.
 	 *
-	 * @stable to override
-	 *
+	 * @deprecated since 1.42 Provide the error message when constructing the Exception instead.
+	 *   If you need a whole custom error page, use ErrorPageError instead.
 	 * @return string
 	 */
 	public function getPageTitle() {
+		wfDeprecated( __METHOD__, '1.42' );
 		return $this->msg( 'internalerror', 'Internal error' );
 	}
 
 	/**
 	 * Output the exception report using HTML.
-	 * @stable to override
+	 * @deprecated since 1.42 Provide the error message when constructing the Exception instead.
+	 *   If you need a whole custom error page, use ErrorPageError instead.
 	 */
 	public function reportHTML() {
+		wfDeprecated( __METHOD__, '1.42' );
 		global $wgOut;
+
 		if ( $this->useOutputPage() ) {
 			$wgOut->prepareErrorPage();
 			$wgOut->setPageTitle( $this->getPageTitle() );
@@ -221,6 +226,27 @@ class MWException extends Exception {
 		}
 	}
 
+	/**
+	 * @internal
+	 */
+	final public function hasOverriddenHandler(): bool {
+		// No deprecation warning - report() is not deprecated, only the other methods
+		if ( MWDebug::detectDeprecatedOverride( $this, __CLASS__, 'report' ) ) {
+			return true;
+		}
+
+		// Check them all here to avoid short-circuiting and report all deprecations,
+		// even if the function is not called in this request
+		$detectedOverrides = [
+			'getHTML' => MWDebug::detectDeprecatedOverride( $this, __CLASS__, 'getHTML', '1.42' ),
+			'getText' => MWDebug::detectDeprecatedOverride( $this, __CLASS__, 'getText', '1.42' ),
+			'getPageTitle' => MWDebug::detectDeprecatedOverride( $this, __CLASS__, 'getPageTitle', '1.42' ),
+			'reportHTML' => MWDebug::detectDeprecatedOverride( $this, __CLASS__, 'reportHTML', '1.42' ),
+		];
+
+		return (bool)array_filter( $detectedOverrides );
+	}
+
 	/**
 	 * Write a message to stderr falling back to stdout if stderr unavailable
 	 *

includes/exception/MWExceptionHandler.php

@@ -124,7 +124,7 @@ class MWExceptionHandler {
 	protected static function report( Throwable $e ) {
 		try {
 			// Try and show the exception prettily, with the normal skin infrastructure
-			if ( $e instanceof MWException ) {
+			if ( $e instanceof MWException && $e->hasOverriddenHandler() ) {
 				// Delegate to MWException until all subclasses are handled by
 				// MWExceptionRenderer and MWException::report() has been
 				// removed.

includes/exception/MWExceptionRenderer.php

@@ -307,7 +307,10 @@ class MWExceptionRenderer {
 	 * @return Message
 	 */
 	private static function getExceptionTitle( Throwable $e ): Message {
-		if ( $e instanceof MWException ) {
+		if (
+			$e instanceof MWException &&
+			MWDebug::detectDeprecatedOverride( $e, MWException::class, 'getPageTitle', '1.42' )
+		) {
 			return ( new RawMessage( '$1' ) )->plaintextParams(
 				$e->getPageTitle() /* convert string title to Message */
 			);
@@ -381,7 +384,7 @@ class MWExceptionRenderer {
 		// NOTE: STDERR may not be available, especially if php-cgi is used from the
 		// command line (T17602). Try to produce meaningful output anyway. Using
 		// echo may corrupt output to STDOUT though.
-		if ( defined( 'STDERR' ) ) {
+		if ( !defined( 'MW_PHPUNIT_TEST' ) && defined( 'STDERR' ) ) {
 			fwrite( STDERR, $message );
 		} else {
 			echo $message;

tests/phpunit/includes/exception/BadTitleErrorTest.php

@@ -21,6 +21,7 @@ class BadTitleErrorTest extends MediaWikiIntegrationTestCase {
 			ob_start();
 			$e->report();
 			$text = ob_get_clean();
+			$this->expectDeprecationAndContinue( '/MWException::getText was deprecated/' );
 			$this->assertStringContainsString( $e->getText(), $text );
 		}
 	}

tests/phpunit/includes/exception/MWExceptionTest.php

@@ -74,4 +74,57 @@ class MWExceptionTest extends MediaWikiIntegrationTestCase {
 		];
 	}
 
+	/**
+	 * @covers MWException::report
+	 */
+	public function testReport() {
+		// Turn off to keep mw-error.log file empty in CI (and thus avoid build failure)
+		$this->setMwGlobals( 'wgDebugLogGroups', [] );
+
+		global $wgOut;
+		$wgOut->disable();
+
+		$e = new class( 'Uh oh!' ) extends MWException {
+			public function report() {
+				global $wgOut;
+				$wgOut->addHTML( 'Oh no!' );
+			}
+		};
+
+		MWExceptionHandler::handleException( $e );
+
+		$this->assertStringContainsString( 'Oh no!', $wgOut->getHTML() );
+	}
+
+	/**
+	 * @covers MWException::report
+	 */
+	public function testReportDeprecated() {
+		// Turn off to keep mw-error.log file empty in CI (and thus avoid build failure)
+		$this->setMwGlobals( 'wgDebugLogGroups', [] );
+
+		global $wgOut;
+		$wgOut->disable();
+
+		$e = new class( 'Uh oh!' ) extends MWException {
+			public function getHTML() {
+				throw new LogicException( 'This should not be called' );
+			}
+
+			public function getText() {
+				return 'Oh no! ' . $this->getPageTitle();
+			}
+		};
+
+		$this->expectDeprecationAndContinue( '/overrides getHTML which was deprecated/' );
+		$this->expectDeprecationAndContinue( '/overrides getText which was deprecated/' );
+		$this->expectDeprecationAndContinue( '/Use of MWException::getPageTitle was deprecated/' );
+
+		ob_start();
+		MWExceptionHandler::handleException( $e );
+		ob_end_clean();
+
+		$this->assertStringContainsString( 'Oh no!', $wgOut->getHTML() );
+	}
+
 }

tests/phpunit/includes/exception/ThrottledErrorTest.php

@@ -21,6 +21,7 @@ class ThrottledErrorTest extends MediaWikiIntegrationTestCase {
 			ob_start();
 			$e->report();
 			$text = ob_get_clean();
+			$this->expectDeprecationAndContinue( '/MWException::getText was deprecated/' );
 			$this->assertStringContainsString( $e->getText(), $text );
 		}
 	}