From de997fe98e093104b2b6ff62eeceae0958584bdf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mat=C4=9Bj=20Such=C3=A1nek?= Date: Wed, 2 Dec 2020 19:08:37 +0100 Subject: [PATCH] Improve type safety of filter ids Also fix a bug in FilterProfiler. It would attempt to reset stats for global filters but we do not record them (yet?). Change-Id: I0228d8c85dab146deb877dfce506f1e8e7711a9f --- includes/AbuseFilter.php | 1 + includes/FilterProfiler.php | 19 +++++++++++-------- includes/View/AbuseFilterViewEdit.php | 4 ++-- includes/Watcher/EmergencyWatcher.php | 6 +++--- includes/special/SpecialAbuseLog.php | 2 +- tests/phpunit/unit/EmergencyWatcherTest.php | 4 ++-- 6 files changed, 20 insertions(+), 16 deletions(-) diff --git a/includes/AbuseFilter.php b/includes/AbuseFilter.php index 3f418c342..59b62313f 100644 --- a/includes/AbuseFilter.php +++ b/includes/AbuseFilter.php @@ -138,6 +138,7 @@ class AbuseFilter { * * @param string|int $filter * @return array + * @phan-return array{0:int,1:bool} * @throws InvalidArgumentException */ public static function splitGlobalName( $filter ) { diff --git a/includes/FilterProfiler.php b/includes/FilterProfiler.php index d86400b7d..3abcb52b3 100644 --- a/includes/FilterProfiler.php +++ b/includes/FilterProfiler.php @@ -91,9 +91,9 @@ class FilterProfiler { } /** - * @param int|string $filter + * @param int $filter */ - public function resetFilterProfile( $filter ) : void { + public function resetFilterProfile( int $filter ) : void { $profileKey = $this->filterProfileKey( $filter ); $this->objectStash->delete( $profileKey ); } @@ -101,11 +101,11 @@ class FilterProfiler { /** * Retrieve per-filter statistics. * - * @param string|int $filter + * @param int $filter * @return array See self::NULL_FILTER_PROFILE for the returned array structure * @phan-return array{count:int,matches:int,total-time:float,total-cond:int} */ - public function getFilterProfile( $filter ) : array { + public function getFilterProfile( int $filter ) : array { return $this->objectStash->get( $this->filterProfileKey( $filter ) ) ?: self::NULL_FILTER_PROFILE; } @@ -164,7 +164,7 @@ class FilterProfiler { * disabled filters too, as their profiling data will be reset upon re-enabling anyway. * * @param string $group - * @param array $allFilters + * @param string[] $allFilters */ public function checkResetProfiling( string $group, array $allFilters ) : void { $profile = $this->getGroupProfile( $group ); @@ -174,7 +174,10 @@ class FilterProfiler { $profileKey = $this->filterProfileGroupKey( $group ); $this->objectStash->delete( $profileKey ); foreach ( $allFilters as $filter ) { - $this->resetFilterProfile( $filter ); + list( $filterID, $global ) = AbuseFilter::splitGlobalName( $filter ); + if ( $global === false ) { + $this->resetFilterProfile( $filterID ); + } } } } @@ -304,10 +307,10 @@ class FilterProfiler { /** * Get the memcache access key used to store per-filter profiling data. * - * @param string|int $filter + * @param int $filter * @return string */ - private function filterProfileKey( $filter ) : string { + private function filterProfileKey( int $filter ) : string { return $this->objectStash->makeKey( 'abusefilter-profile', 'v3', $filter ); } diff --git a/includes/View/AbuseFilterViewEdit.php b/includes/View/AbuseFilterViewEdit.php index 71816b2ad..7479c3a48 100644 --- a/includes/View/AbuseFilterViewEdit.php +++ b/includes/View/AbuseFilterViewEdit.php @@ -101,11 +101,11 @@ class AbuseFilterViewEdit extends AbuseFilterView { $out->setPageTitle( $this->msg( 'abusefilter-edit' ) ); $out->addHelpLink( 'Extension:AbuseFilter/Rules format' ); - $filter = $this->filter; - if ( !is_numeric( $filter ) && $filter !== null ) { + if ( !is_numeric( $this->filter ) && $this->filter !== null ) { $this->showUnrecoverableError( 'abusefilter-edit-badfilter' ); return; } + $filter = $this->filter ? (int)$this->filter : null; $history_id = $this->historyID; if ( $this->historyID ) { $dbr = wfGetDB( DB_REPLICA ); diff --git a/includes/Watcher/EmergencyWatcher.php b/includes/Watcher/EmergencyWatcher.php index 16faad88e..f97b3f99f 100644 --- a/includes/Watcher/EmergencyWatcher.php +++ b/includes/Watcher/EmergencyWatcher.php @@ -61,9 +61,9 @@ class EmergencyWatcher implements Watcher { * Determine which filters must be throttled, i.e. their potentially dangerous * actions must be disabled. * - * @param string[] $filters The filters to check + * @param int[] $filters The filters to check * @param string $group Group the filters belong to - * @return string[] Array of filters to be throttled + * @return int[] Array of filters to be throttled */ public function getFiltersToThrottle( array $filters, string $group ) : array { $groupProfile = $this->profiler->getGroupProfile( $group ); @@ -83,7 +83,7 @@ class EmergencyWatcher implements Watcher { $throttleFilters = []; foreach ( $filters as $filter ) { - $filterObj = $this->filterLookup->getFilter( (int)$filter, false ); + $filterObj = $this->filterLookup->getFilter( $filter, false ); if ( $filterObj->isThrottled() ) { continue; } diff --git a/includes/special/SpecialAbuseLog.php b/includes/special/SpecialAbuseLog.php index f5fb02b9e..f06d27f13 100644 --- a/includes/special/SpecialAbuseLog.php +++ b/includes/special/SpecialAbuseLog.php @@ -1239,7 +1239,7 @@ class SpecialAbuseLog extends AbuseFilterSpecialPage { $filterLink = $linkMsg->escaped(); } } else { - $title = SpecialPage::getTitleFor( 'AbuseFilter', $filterID ); + $title = SpecialPage::getTitleFor( 'AbuseFilter', (string)$filterID ); $linkText = $this->msg( 'abusefilter-log-detailedentry-local' ) ->numParams( $filterID )->text(); $filterLink = $linkRenderer->makeKnownLink( $title, $linkText ); diff --git a/tests/phpunit/unit/EmergencyWatcherTest.php b/tests/phpunit/unit/EmergencyWatcherTest.php index 64d5da007..75358109d 100644 --- a/tests/phpunit/unit/EmergencyWatcherTest.php +++ b/tests/phpunit/unit/EmergencyWatcherTest.php @@ -173,11 +173,11 @@ class EmergencyWatcherTest extends MediaWikiUnitTestCase { $this->getOptions() ); $toThrottle = $watcher->getFiltersToThrottle( - [ '1' ], + [ 1 ], $group ); $this->assertSame( - $willThrottle ? [ '1' ] : [], + $willThrottle ? [ 1 ] : [], $toThrottle ); }