diff --git a/includes/AbuseFilterHooks.php b/includes/AbuseFilterHooks.php index af050d163..7be051878 100644 --- a/includes/AbuseFilterHooks.php +++ b/includes/AbuseFilterHooks.php @@ -105,11 +105,12 @@ class AbuseFilterHooks { // @todo is there any real point in passing this in? $text = AbuseFilter::contentToString( $content ); - $filterStatus = self::filterEdit( $context, $content, $text, $status, $summary, $slot ); + $filterResult = self::filterEdit( $context, $content, $text, $summary, $slot ); - if ( !$filterStatus->isOK() ) { + if ( !$filterResult->isOK() ) { // Produce a useful error message for API edits - $status->apiHookResult = self::getApiResult( $filterStatus ); + $filterResultApi = self::getApiStatus( $filterResult ); + $status->merge( $filterResultApi ); } } @@ -184,13 +185,12 @@ class AbuseFilterHooks { * @param IContextSource $context the context of the edit * @param Content $content the new Content generated by the edit * @param string $text new page content (subject of filtering) - * @param Status $status Error message to return * @param string $summary Edit summary for page * @param string $slot slot role for the content * @return Status */ public static function filterEdit( IContextSource $context, Content $content, $text, - Status $status, $summary, $slot = SlotRecord::MAIN + $summary, $slot = SlotRecord::MAIN ) { self::$lastEditPage = null; @@ -218,8 +218,6 @@ class AbuseFilterHooks { $runner = new AbuseFilterRunner( $user, $title, $vars, 'default' ); $filterResult = $runner->run(); if ( !$filterResult->isOK() ) { - $status->merge( $filterResult ); - return $filterResult; } @@ -268,45 +266,30 @@ class AbuseFilterHooks { /** * @param Status $status Error message details - * @return array API result + * @return Status Status containing the same error messages with extra data for the API */ - private static function getApiResult( Status $status ) { - global $wgFullyInitialised; + private static function getApiStatus( Status $status ) { + $allActionsTaken = $status->getValue(); + $statusForApi = Status::newGood(); - $params = $status->getErrorsArray()[0]; - $key = array_shift( $params ); + foreach ( $status->getErrors() as $error ) { + list( $filterDescription, $filter ) = $error['params']; + $actionsTaken = $allActionsTaken[ $filter ]; - $warning = wfMessage( $key )->params( $params ); - if ( !$wgFullyInitialised ) { - // This could happen for account autocreation checks - $warning = $warning->inContentLanguage(); + $code = ( $actionsTaken === [ 'warn' ] ) ? 'abusefilter-warning' : 'abusefilter-disallowed'; + $data = [ + 'abusefilter' => [ + 'id' => $filter, + 'description' => $filterDescription, + 'actions' => $actionsTaken, + ], + ]; + + $message = ApiMessage::create( $error, $code, $data ); + $statusForApi->fatal( $message ); } - list( $filterDescription, $filter ) = $params; - - // The value is a nested structure keyed by filter id, which doesn't make sense when we only - // return the result from one filter. Flatten it to a plain array of actions. - $actionsTaken = array_values( array_unique( - array_merge( ...array_values( $status->getValue() ) ) - ) ); - $code = ( $actionsTaken === [ 'warn' ] ) ? 'abusefilter-warning' : 'abusefilter-disallowed'; - - ApiResult::setIndexedTagName( $params, 'param' ); - return [ - 'code' => $code, - 'message' => [ - 'key' => $key, - 'params' => $params, - ], - 'abusefilter' => [ - 'id' => $filter, - 'description' => $filterDescription, - 'actions' => $actionsTaken, - ], - // For backwards-compatibility - 'info' => 'Hit AbuseFilter: ' . $filterDescription, - 'warning' => $warning->parse(), - ]; + return $statusForApi; } /** @@ -986,13 +969,10 @@ class AbuseFilterHooks { $filterResult = $runner->run(); if ( !$filterResult->isOK() ) { - $messageAndParams = $filterResult->getErrorsArray()[0]; - $apiResult = self::getApiResult( $filterResult ); - $error = ApiMessage::create( - $messageAndParams, - $apiResult['code'], - $apiResult - ); + // Produce a useful error message for API edits + $filterResultApi = self::getApiStatus( $filterResult ); + // @todo Return all errors instead of only the first one + $error = $filterResultApi->getErrors()[0]['message']; } return $filterResult->isOK(); diff --git a/modules/ve-abusefilter/ve.init.mw.AbuseFilterSaveErrorHandler.js b/modules/ve-abusefilter/ve.init.mw.AbuseFilterSaveErrorHandler.js index 4ce60dae8..68c8a1bc6 100644 --- a/modules/ve-abusefilter/ve.init.mw.AbuseFilterSaveErrorHandler.js +++ b/modules/ve-abusefilter/ve.init.mw.AbuseFilterSaveErrorHandler.js @@ -7,15 +7,17 @@ mw.libs.ve.targetLoader.addPlugin( function () { ve.init.mw.AbuseFilterSaveErrorHandler.static.name = 'abuseFilter'; ve.init.mw.AbuseFilterSaveErrorHandler.static.matchFunction = function ( data ) { - return !!ve.getProp( data, 'visualeditoredit', 'edit', 'abusefilter' ); + return data.errors && data.errors.some( function ( err ) { + return err.code === 'abusefilter-disallowed' || err.code === 'abusefilter-warning'; + } ); }; ve.init.mw.AbuseFilterSaveErrorHandler.static.process = function ( data, target ) { - var - $message = $( $.parseHTML( ve.getProp( data, 'visualeditoredit', 'edit', 'warning' ) ) ), - isWarning = ve.getProp( data, 'visualeditoredit', 'edit', 'code' ) !== 'abusefilter-disallowed'; + var isWarning = data.errors.every( function ( err ) { + return err.code === 'abusefilter-warning'; + } ); // Handle warnings/errors from Extension:AbuseFilter - target.showSaveError( $message, isWarning, isWarning ); + target.showSaveError( target.extractErrorMessages( data ), isWarning, isWarning ); // Emit event for tracking. TODO: This is a bad design target.emit( 'saveErrorAbuseFilter' ); }; diff --git a/tests/phpunit/AbuseFilterConsequencesTest.php b/tests/phpunit/AbuseFilterConsequencesTest.php index 756efd793..49f8afa47 100644 --- a/tests/phpunit/AbuseFilterConsequencesTest.php +++ b/tests/phpunit/AbuseFilterConsequencesTest.php @@ -752,7 +752,9 @@ class AbuseFilterConsequencesTest extends MediaWikiTestCase { $actual = []; foreach ( $errors as $error ) { - $msg = $error['message']; + // We don't use any of the "API" stuff in ApiMessage here, but this is the most + // convenient way to get a Message from a StatusValue error structure. + $msg = ApiMessage::create( $error )->getKey(); if ( strpos( $msg, 'abusefilter' ) !== false ) { $actual[] = $msg; }