API: Catch Errors as well as Exceptions
authorBrad Jorsch <bjorsch@wikimedia.org>
Tue, 21 Aug 2018 15:19:15 +0000 (11:19 -0400)
committerBrad Jorsch <bjorsch@wikimedia.org>
Wed, 22 Aug 2018 13:30:06 +0000 (09:30 -0400)
ApiMain (and also api.php) tries to catch any Exception so as to provide
a properly-formatted error message to the client instead of an HTML
error page.

With PHP 7.0, some cases that produce an Exception in HHVM instead
produce an Error. The API code should catch these too.

Fortunately neither Zend PHP nor HHVM care if you try to catch a class
that doesn't exist, so we can just add catch blocks for Throwable and
not worry about it.

Bug: T202416
Change-Id: I189eee466bd09870bc172f2420be393a7c0b1900

api.php
includes/api/ApiMain.php

diff --git a/api.php b/api.php
index 9c5ac95..9cf7578 100644 (file)
--- a/api.php
+++ b/api.php
@@ -72,7 +72,11 @@ try {
        if ( !$processor instanceof ApiMain ) {
                throw new MWException( 'ApiBeforeMain hook set $processor to a non-ApiMain class' );
        }
        if ( !$processor instanceof ApiMain ) {
                throw new MWException( 'ApiBeforeMain hook set $processor to a non-ApiMain class' );
        }
-} catch ( Exception $e ) {
+} catch ( Exception $e ) { // @todo Remove this block when HHVM is no longer supported
+       // Crap. Try to report the exception in API format to be friendly to clients.
+       ApiMain::handleApiBeforeMainException( $e );
+       $processor = false;
+} catch ( Throwable $e ) {
        // Crap. Try to report the exception in API format to be friendly to clients.
        ApiMain::handleApiBeforeMainException( $e );
        $processor = false;
        // Crap. Try to report the exception in API format to be friendly to clients.
        ApiMain::handleApiBeforeMainException( $e );
        $processor = false;
@@ -99,7 +103,9 @@ if ( $wgAPIRequestLog ) {
                try {
                        $manager = $processor->getModuleManager();
                        $module = $manager->getModule( $wgRequest->getVal( 'action' ), 'action' );
                try {
                        $manager = $processor->getModuleManager();
                        $module = $manager->getModule( $wgRequest->getVal( 'action' ), 'action' );
-               } catch ( Exception $ex ) {
+               } catch ( Exception $ex ) { // @todo Remove this block when HHVM is no longer supported
+                       $module = null;
+               } catch ( Throwable $ex ) {
                        $module = null;
                }
                if ( !$module || $module->mustBePosted() ) {
                        $module = null;
                }
                if ( !$module || $module->mustBePosted() ) {
index 03d2952..3b305f9 100644 (file)
@@ -534,7 +534,11 @@ class ApiMain extends ApiBase {
                        MediaWikiServices::getInstance()->getStatsdDataFactory()->timing(
                                'api.' . $this->mModule->getModuleName() . '.executeTiming', 1000 * $runTime
                        );
                        MediaWikiServices::getInstance()->getStatsdDataFactory()->timing(
                                'api.' . $this->mModule->getModuleName() . '.executeTiming', 1000 * $runTime
                        );
-               } catch ( Exception $e ) {
+               } catch ( Exception $e ) { // @todo Remove this block when HHVM is no longer supported
+                       $this->handleException( $e );
+                       $this->logRequest( microtime( true ) - $t, $e );
+                       $isError = true;
+               } catch ( Throwable $e ) {
                        $this->handleException( $e );
                        $this->logRequest( microtime( true ) - $t, $e );
                        $isError = true;
                        $this->handleException( $e );
                        $this->logRequest( microtime( true ) - $t, $e );
                        $isError = true;
@@ -558,9 +562,9 @@ class ApiMain extends ApiBase {
         * Handle an exception as an API response
         *
         * @since 1.23
         * Handle an exception as an API response
         *
         * @since 1.23
-        * @param Exception $e
+        * @param Exception|Throwable $e
         */
         */
-       protected function handleException( Exception $e ) {
+       protected function handleException( $e ) {
                // T65145: Rollback any open database transactions
                if ( !( $e instanceof ApiUsageException || $e instanceof UsageException ) ) {
                        // UsageExceptions are intentional, so don't rollback if that's the case
                // T65145: Rollback any open database transactions
                if ( !( $e instanceof ApiUsageException || $e instanceof UsageException ) ) {
                        // UsageExceptions are intentional, so don't rollback if that's the case
@@ -600,7 +604,10 @@ class ApiMain extends ApiBase {
                        foreach ( $ex->getStatusValue()->getErrors() as $error ) {
                                try {
                                        $this->mPrinter->addWarning( $error );
                        foreach ( $ex->getStatusValue()->getErrors() as $error ) {
                                try {
                                        $this->mPrinter->addWarning( $error );
-                               } catch ( Exception $ex2 ) {
+                               } catch ( Exception $ex2 ) { // @todo Remove this block when HHVM is no longer supported
+                                       // WTF?
+                                       $this->addWarning( $error );
+                               } catch ( Throwable $ex2 ) {
                                        // WTF?
                                        $this->addWarning( $error );
                                }
                                        // WTF?
                                        $this->addWarning( $error );
                                }
@@ -631,17 +638,20 @@ class ApiMain extends ApiBase {
         * friendly to clients. If it fails, it will rethrow the exception.
         *
         * @since 1.23
         * friendly to clients. If it fails, it will rethrow the exception.
         *
         * @since 1.23
-        * @param Exception $e
-        * @throws Exception
+        * @param Exception|Throwable $e
+        * @throws Exception|Throwable
         */
         */
-       public static function handleApiBeforeMainException( Exception $e ) {
+       public static function handleApiBeforeMainException( $e ) {
                ob_start();
 
                try {
                        $main = new self( RequestContext::getMain(), false );
                        $main->handleException( $e );
                        $main->logRequest( 0, $e );
                ob_start();
 
                try {
                        $main = new self( RequestContext::getMain(), false );
                        $main->handleException( $e );
                        $main->logRequest( 0, $e );
-               } catch ( Exception $e2 ) {
+               } catch ( Exception $e2 ) { // @todo Remove this block when HHVM is no longer supported
+                       // Nope, even that didn't work. Punt.
+                       throw $e;
+               } catch ( Throwable $e2 ) {
                        // Nope, even that didn't work. Punt.
                        throw $e;
                }
                        // Nope, even that didn't work. Punt.
                        throw $e;
                }
@@ -1009,7 +1019,7 @@ class ApiMain extends ApiBase {
         * text around the exception's (presumably English) message as a single
         * error (no warnings).
         *
         * text around the exception's (presumably English) message as a single
         * error (no warnings).
         *
-        * @param Exception $e
+        * @param Exception|Throwable $e
         * @param string $type 'error' or 'warning'
         * @return ApiMessage[]
         * @since 1.27
         * @param string $type 'error' or 'warning'
         * @return ApiMessage[]
         * @since 1.27
@@ -1054,7 +1064,7 @@ class ApiMain extends ApiBase {
 
        /**
         * Replace the result data with the information about an exception.
 
        /**
         * Replace the result data with the information about an exception.
-        * @param Exception $e
+        * @param Exception|Throwable $e
         * @return string[] Error codes
         */
        protected function substituteResultWithError( $e ) {
         * @return string[] Error codes
         */
        protected function substituteResultWithError( $e ) {
@@ -1609,7 +1619,7 @@ class ApiMain extends ApiBase {
        /**
         * Log the preceding request
         * @param float $time Time in seconds
        /**
         * Log the preceding request
         * @param float $time Time in seconds
-        * @param Exception|null $e Exception caught while processing the request
+        * @param Exception|Throwable|null $e Exception caught while processing the request
         */
        protected function logRequest( $time, $e = null ) {
                $request = $this->getRequest();
         */
        protected function logRequest( $time, $e = null ) {
                $request = $this->getRequest();