From c458651370e10ddec8a99731d3404784a0289036 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartosz=20Dziewo=C5=84ski?= Date: Sat, 13 Apr 2024 01:04:21 +0200 Subject: [PATCH] Fix Status combining MessageSpecifier and parameters array Constructing a Status like this does not make sense (and I want to deprecate it in I0675e557bb93a1c990fa923c50b9f6ee8a9836c8), because the parameters are ignored by most Status methods: $error = Message::newFromSpecifier( 'abusefilter-blocked-domains-attempted' ); $status = Status::newFatal( $error, 'blockeddomain', 'blockeddomain' ); But it worked here, because FilteredActionsHandler::getApiStatus() used a deprecated method that allowed inspecting them. I feel like this code in BlockedDomainFilter has been added to make the tests pass without thinking about what it actually does, which is to output a bunch of useless incorrect data in API errors. I'm not sure if we can remove that now without breaking API compatibility, so add the useless data in FilteredActionsHandler instead, closer to where it's output. Change-Id: Ic12241bd3029bc1b0e7a0023689a2be35ccd30a8 --- extension.json | 2 +- includes/BlockedDomainFilter.php | 4 +- .../Hooks/Handlers/FilteredActionsHandler.php | 55 ++++++++++++------- 3 files changed, 36 insertions(+), 25 deletions(-) diff --git a/extension.json b/extension.json index c14c2c5c8..7c6ff9cdb 100644 --- a/extension.json +++ b/extension.json @@ -12,7 +12,7 @@ "license-name": "GPL-2.0-or-later", "type": "antispam", "requires": { - "MediaWiki": ">= 1.42.0" + "MediaWiki": ">= 1.43.0" }, "AvailableRights": [ "abusefilter-modify", diff --git a/includes/BlockedDomainFilter.php b/includes/BlockedDomainFilter.php index f1ce1c375..88eb357aa 100644 --- a/includes/BlockedDomainFilter.php +++ b/includes/BlockedDomainFilter.php @@ -106,9 +106,7 @@ class BlockedDomainFilter { $blockedDomainsAdded = array_keys( $blockedDomainsAdded ); $error = Message::newFromSpecifier( 'abusefilter-blocked-domains-attempted' ); $error->params( Message::listParam( $blockedDomainsAdded ) ); - - $status = Status::newFatal( $error, 'blockeddomain', 'blockeddomain' ); - $status->value['blockeddomain'] = [ 'disallow' ]; + $status->fatal( $error ); $this->logFilterHit( $user, $title, diff --git a/includes/Hooks/Handlers/FilteredActionsHandler.php b/includes/Hooks/Handlers/FilteredActionsHandler.php index 757374bc4..02cca7bd2 100644 --- a/includes/Hooks/Handlers/FilteredActionsHandler.php +++ b/includes/Hooks/Handlers/FilteredActionsHandler.php @@ -94,13 +94,8 @@ class FilteredActionsHandler implements ); } - $filterResult = $this->filterEdit( $context, $user, $content, $summary, $slot ); + $this->filterEdit( $context, $user, $content, $summary, $slot, $status ); - if ( !$filterResult->isOK() ) { - // Produce a useful error message for API edits - $filterResultApi = self::getApiStatus( $filterResult ); - $status->merge( $filterResultApi ); - } $this->statsDataFactory->timing( 'timing.editAbuseFilter', microtime( true ) - $startTime ); return $status->isOK(); @@ -114,15 +109,16 @@ class FilteredActionsHandler implements * @param Content $content the new Content generated by the edit * @param string $summary Edit summary for page * @param string $slot slot role for the content - * @return Status + * @param Status $status */ private function filterEdit( IContextSource $context, User $user, Content $content, string $summary, - string $slot = SlotRecord::MAIN - ): Status { + string $slot, + Status $status + ): void { $this->editRevUpdater->clearLastEditPage(); $title = $context->getTitle(); @@ -130,13 +126,13 @@ class FilteredActionsHandler implements if ( $title === null ) { // T144265: This *should* never happen. $logger->warning( __METHOD__ . ' received a null title.' ); - return Status::newGood(); + return; } if ( !$title->canExist() ) { // This also should be handled in EditPage or whoever is calling the hook. $logger->warning( __METHOD__ . ' received a Title that cannot exist.' ); // Note that if the title cannot exist, there's no much point in filtering the edit anyway - return Status::newGood(); + return; } $page = $context->getWikiPage(); @@ -145,25 +141,42 @@ class FilteredActionsHandler implements $vars = $builder->getEditVars( $content, $summary, $slot, $page ); if ( $vars === null ) { // We don't have to filter the edit - return Status::newGood(); + return; } $runner = $this->filterRunnerFactory->newRunner( $user, $title, $vars, 'default' ); $filterResult = $runner->run(); if ( !$filterResult->isOK() ) { - return $filterResult; + // Produce a useful error message for API edits + $filterResultApi = self::getApiStatus( $filterResult ); + $status->merge( $filterResultApi ); + return; } $this->editRevUpdater->setLastEditPage( $page ); if ( $this->permissionManager->userHasRight( $user, 'abusefilter-bypass-blocked-external-domains' ) ) { - return Status::newGood(); + return; } $blockedDomainFilterResult = $this->blockedDomainFilter->filter( $vars, $user, $title ); if ( !$blockedDomainFilterResult->isOK() ) { - return $blockedDomainFilterResult; + // Produce a useful error message for API edits + // FIXME: This is not useful, but it was added by accident :( + foreach ( $blockedDomainFilterResult->getMessages() as $msg ) { + if ( $msg->getKey() === 'abusefilter-blocked-domains-attempted' ) { + $blockedDomainFilterResult->replaceMessage( + 'abusefilter-blocked-domains-attempted', + ApiMessage::create( $msg, 'abusefilter-disallowed', [ + 'abusefilter' => [ + 'id' => 'blockeddomain', + 'description' => 'blockeddomain', + 'actions' => 'disallow', + ], + ] ) + ); + } + } + $status->merge( $blockedDomainFilterResult ); } - - return Status::newGood(); } /** @@ -174,8 +187,8 @@ class FilteredActionsHandler implements $allActionsTaken = $status->getValue(); $statusForApi = Status::newGood(); - foreach ( $status->getErrors() as $error ) { - [ $filterDescription, $filter ] = $error['params']; + foreach ( $status->getMessages() as $msg ) { + [ $filterDescription, $filter ] = $msg->getParams(); $actionsTaken = $allActionsTaken[ $filter ]; $code = ( $actionsTaken === [ 'warn' ] ) ? 'abusefilter-warning' : 'abusefilter-disallowed'; @@ -187,7 +200,7 @@ class FilteredActionsHandler implements ], ]; - $message = ApiMessage::create( $error, $code, $data ); + $message = ApiMessage::create( $msg, $code, $data ); $statusForApi->fatal( $message ); } @@ -298,7 +311,7 @@ class FilteredActionsHandler implements } $blockedDomainFilterResult = $this->blockedDomainFilter->filter( $vars, $user, $title ); if ( !$blockedDomainFilterResult->isOK() ) { - $error = $blockedDomainFilterResult->getErrors()[0]['message']; + $error = $blockedDomainFilterResult->getMessages()[0]; return $blockedDomainFilterResult->isOK(); } }