MWException: Log stack traces for php errors (not exceptions)

* Remove use of 'error' where it's redundant.
* Remove call to logException from responsibility of MWException.
  Call from exception handler instead.

Change-Id: I8764cf5df87b226813c9b9cf99f9b4f3fa4b7c92

includes/MediaWiki.php

@@ -446,7 +446,7 @@ class MediaWiki {
 			$this->triggerJobs();
 			$this->restInPeace();
 		} catch ( Exception $e ) {
-			MWExceptionHandler::handle( $e );
+			MWExceptionHandler::handleException( $e );
 		}
 	}
 

includes/exception/MWException.php

@@ -222,8 +222,6 @@ class MWException extends Exception {
 	public function report() {
 		global $wgMimeType;
 
-		MWExceptionHandler::logException( $this );
-
 		if ( defined( 'MW_API' ) ) {
 			// Unhandled API exception, we can't be sure that format printer is alive
 			self::header( 'MediaWiki-API-Error: internal_api_error_' . get_class( $this ) );

includes/exception/MWExceptionHandler.php

@@ -23,11 +23,13 @@
  * @ingroup Exception
  */
 class MWExceptionHandler {
+
 	/**
-	 * Install an exception handler for MediaWiki exception types.
+	 * Install handlers with PHP.
 	 */
 	public static function installHandler() {
-		set_exception_handler( array( 'MWExceptionHandler', 'handle' ) );
+		set_exception_handler( array( 'MWExceptionHandler', 'handleException' ) );
+		set_error_handler( array( 'MWExceptionHandler', 'handleError' ) );
 	}
 
 	/**
@@ -45,7 +47,7 @@ class MWExceptionHandler {
 				$e->report();
 			} catch ( Exception $e2 ) {
 				// Exception occurred from within exception handler
-				// Show a simpler error message for the original exception,
+				// Show a simpler message for the original exception,
 				// don't try to invoke report()
 				$message = "MediaWiki internal error.\n\n";
 
@@ -83,7 +85,6 @@ class MWExceptionHandler {
 				echo nl2br( htmlspecialchars( $message ) ) . "\n";
 			}
 
-			self::logException( $e );
 		}
 	}
 
@@ -108,6 +109,7 @@ class MWExceptionHandler {
 	 * If there are any open database transactions, roll them back and log
 	 * the stack trace of the exception that should have been caught so the
 	 * transaction could be aborted properly.
+	 *
 	 * @since 1.23
 	 * @param Exception $e
 	 */
@@ -133,13 +135,15 @@ class MWExceptionHandler {
 	 *   } catch ( Exception $e ) {
 	 *       echo $e->__toString();
 	 *   }
+	 *
+	 * @since 1.25
 	 * @param Exception $e
 	 */
-	public static function handle( $e ) {
+	public static function handleException( $e ) {
 		global $wgFullyInitialised;
 
 		self::rollbackMasterChangesAndLog( $e );
-
+		self::logException( $e );
 		self::report( $e );
 
 		// Final cleanup
@@ -155,6 +159,22 @@ class MWExceptionHandler {
 		exit( 1 );
 	}
 
+	/**
+	 * @since 1.25
+	 * @param int $level Error level raised
+	 * @param string $message
+	 * @param string $file
+	 * @param int $line
+	 */
+	public static function handleError( $level, $message, $file = null, $line = null ) {
+		$e = new ErrorException( $message, 0, $level, $file, $line );
+		self::logError( $e );
+
+		// This handler is for logging only. Return false will instruct PHP
+		// to continue regular handling.
+		return false;
+	}
+
 	/**
 	 * Generate a string representation of an exception's stack trace
 	 *
@@ -219,7 +239,7 @@ class MWExceptionHandler {
 	}
 
 	/**
-	 * Get the ID for this error.
+	 * Get the ID for this exception.
 	 *
 	 * The ID is saved so that one can match the one output to the user (when
 	 * $wgShowExceptionDetails is set to false), to the entry in the debug log.
@@ -251,8 +271,7 @@ class MWExceptionHandler {
 	}
 
 	/**
-	 * Return the requested URL and point to file and line number from which the
-	 * exception occurred.
+	 * Get a message formatting the exception message and its origin.
 	 *
 	 * @since 1.22
 	 * @param Exception $e
@@ -260,12 +279,13 @@ class MWExceptionHandler {
 	 */
 	public static function getLogMessage( Exception $e ) {
 		$id = self::getLogId( $e );
+		$type = get_class( $e );
 		$file = $e->getFile();
 		$line = $e->getLine();
 		$message = $e->getMessage();
 		$url = self::getURL() ?: '[no req]';
 
-		return "[$id] $url   Exception from line $line of $file: $message";
+		return "[$id] $url   $type from line $line of $file: $message";
 	}
 
 	/**
@@ -287,6 +307,7 @@ class MWExceptionHandler {
 	 * @code
 	 *  {
 	 *    "id": "c41fb419",
+	 *    "type": "MWException",
 	 *    "file": "/var/www/mediawiki/includes/cache/MessageCache.php",
 	 *    "line": 704,
 	 *    "message": "Non-string key given",
@@ -298,6 +319,7 @@ class MWExceptionHandler {
 	 * @code
 	 *  {
 	 *    "id": "dc457938",
+	 *    "type": "MWException",
 	 *    "file": "/vagrant/mediawiki/includes/cache/MessageCache.php",
 	 *    "line": 704,
 	 *    "message": "Non-string key given",
@@ -324,6 +346,7 @@ class MWExceptionHandler {
 
 		$exceptionData = array(
 			'id' => self::getLogId( $e ),
+			'type' => get_class( $e ),
 			'file' => $e->getFile(),
 			'line' => $e->getLine(),
 			'message' => $e->getMessage(),
@@ -347,7 +370,7 @@ class MWExceptionHandler {
 	 * Log an exception to the exception log (if enabled).
 	 *
 	 * This method must not assume the exception is an MWException,
-	 * it is also used to handle PHP errors or errors from other libraries.
+	 * it is also used to handle PHP exceptions or exceptions from other libraries.
 	 *
 	 * @since 1.22
 	 * @param Exception $e
@@ -368,7 +391,22 @@ class MWExceptionHandler {
 				wfDebugLog( 'exception-json', $json, 'private' );
 			}
 		}
-
 	}
 
+	/**
+	 * Log an exception that wasn't thrown but made to wrap an error.
+	 *
+	 * @since 1.25
+	 * @param Exception $e
+	*/
+	protected static function logError( Exception $e ) {
+		global $wgLogExceptionBacktrace;
+
+		$log = self::getLogMessage( $e );
+		if ( $wgLogExceptionBacktrace ) {
+			wfDebugLog( 'error', $log . "\n" . $e->getTraceAsString() );
+		} else {
+			wfDebugLog( 'error', $log );
+		}
+	}
 }

tests/phpunit/phpunit.php

@@ -93,6 +93,12 @@ class PHPUnitMaintClass extends Maintenance {
 	public function execute() {
 		global $IP;
 
+		// Deregister handler from MWExceptionHandler::installHandle so that PHPUnit's own handler
+		// stays in tact.
+		// Has to in execute() instead of finalSetup(), because finalSetup() runs before
+		// doMaintenance.php includes Setup.php, which calls MWExceptionHandler::installHandle().
+		restore_error_handler();
+
 		$this->forceFormatServerArgv();
 
 		# Make sure we have --configuration or PHPUnit might complain