From cf463a40599948928fec4f01aabff9c0c3a8ec44 Mon Sep 17 00:00:00 2001 From: Matthew Flaschen Date: Fri, 31 Mar 2017 01:07:31 -0400 Subject: [PATCH] RCFilters: Remove isAllowedCallable and isAllowed This is pretty fragile; it's easy to accidentally miss one of the checks (as has already happened in e.g. parseParameters). Although I don't yet know of any bugs as a result of this, it's cleaner to do it at registration time. There are no extensions using this feature. Change-Id: I8547ea6432cae73e1bc272dbe959f2415b8a6d21 --- includes/changes/ChangesListBooleanFilter.php | 14 +- includes/changes/ChangesListFilter.php | 46 +--- includes/changes/ChangesListFilterGroup.php | 5 +- .../ChangesListStringOptionsFilter.php | 2 +- .../ChangesListStringOptionsFilterGroup.php | 8 +- .../specialpage/ChangesListSpecialPage.php | 217 +++++++++++------- includes/specials/SpecialRecentchanges.php | 24 +- includes/specials/SpecialWatchlist.php | 24 +- .../ChangesListBooleanFilterGroupTest.php | 9 +- .../changes/ChangesListBooleanFilterTest.php | 13 +- ...hangesListStringOptionsFilterGroupTest.php | 34 +-- ...AbstractChangesListSpecialPageTestCase.php | 27 +++ tests/phpunit/mocks/MockChangesListFilter.php | 2 +- 13 files changed, 215 insertions(+), 210 deletions(-) diff --git a/includes/changes/ChangesListBooleanFilter.php b/includes/changes/ChangesListBooleanFilter.php index d0c4b77527..4117a110cf 100644 --- a/includes/changes/ChangesListBooleanFilter.php +++ b/includes/changes/ChangesListBooleanFilter.php @@ -97,11 +97,6 @@ class ChangesListBooleanFilter extends ChangesListFilter { * to true. It does not need to be set if the exact same filter is simply visible * on both. * $filterDefinition['default'] bool Default - * $filterDefinition['isAllowedCallable'] callable Callable taking two parameters, - * the class name of the special page and an IContextSource, and returning true - * if and only if the current user is permitted to use this filter on the current - * wiki. If it returns false, it will both hide the UI (in all UIs) and prevent - * the DB query modification from taking effect. (optional, defaults to allowed) * $filterDefinition['priority'] int Priority integer. Higher value means higher * up in the group's filter list. * $filterDefinition['queryCallable'] callable Callable accepting parameters, used @@ -166,17 +161,16 @@ class ChangesListBooleanFilter extends ChangesListFilter { /** * @inheritdoc */ - public function displaysOnUnstructuredUi( ChangesListSpecialPage $specialPage ) { - return $this->showHide && - $this->isAllowed( $specialPage ); + public function displaysOnUnstructuredUi() { + return !!$this->showHide; } /** * @inheritdoc */ - public function isFeatureAvailableOnStructuredUi( ChangesListSpecialPage $specialPage ) { + public function isFeatureAvailableOnStructuredUi() { return $this->isReplacedInStructuredUi || - parent::isFeatureAvailableOnStructuredUi( $specialPage ); + parent::isFeatureAvailableOnStructuredUi(); } /** diff --git a/includes/changes/ChangesListFilter.php b/includes/changes/ChangesListFilter.php index 22e797d1cf..b3a16a81b3 100644 --- a/includes/changes/ChangesListFilter.php +++ b/includes/changes/ChangesListFilter.php @@ -73,13 +73,6 @@ abstract class ChangesListFilter { */ protected $description; - /** - * Callable used to check whether this filter is allowed to take effect - * - * @var callable $isAllowedCallable - */ - protected $isAllowedCallable; - /** * List of conflicting groups * @@ -139,11 +132,6 @@ abstract class ChangesListFilter { * $filterDefinition['label'] string i18n key of label for structured UI. * $filterDefinition['description'] string i18n key of description for structured * UI. - * $filterDefinition['isAllowedCallable'] callable Callable taking two parameters, - * the class name of the special page and an IContextSource, and returning true - * if and only if the current user is permitted to use this filter on the current - * wiki. If it returns false, it will both hide the UI (in all UIs) and prevent - * the DB query modification from taking effect. (optional, defaults to allowed) * $filterDefinition['priority'] int Priority integer. Higher value means higher * up in the group's filter list. */ @@ -179,10 +167,6 @@ abstract class ChangesListFilter { $this->description = $filterDefinition['description']; } - if ( isset( $filterDefinition['isAllowedCallable'] ) ) { - $this->isAllowedCallable = $filterDefinition['isAllowedCallable']; - } - $this->priority = $filterDefinition['priority']; $this->group->registerFilter( $this ); @@ -311,20 +295,18 @@ abstract class ChangesListFilter { /** * Checks whether the filter should display on the unstructured UI * - * @param ChangesListSpecialPage $specialPage Current special page * @return bool Whether to display */ - abstract public function displaysOnUnstructuredUi( ChangesListSpecialPage $specialPage ); + abstract public function displaysOnUnstructuredUi(); /** * Checks whether the filter should display on the structured UI * This refers to the exact filter. See also isFeatureAvailableOnStructuredUi. * - * @param ChangesListSpecialPage $specialPage Current special page * @return bool Whether to display */ - public function displaysOnStructuredUi( ChangesListSpecialPage $specialPage ) { - return $this->label !== null && $this->isAllowed( $specialPage ); + public function displaysOnStructuredUi() { + return $this->label !== null; } /** @@ -333,8 +315,8 @@ abstract class ChangesListFilter { * * This can either be the exact filter, or a new filter that replaces it. */ - public function isFeatureAvailableOnStructuredUi( ChangesListSpecialPage $specialPage ) { - return $this->displaysOnStructuredUi( $specialPage ); + public function isFeatureAvailableOnStructuredUi() { + return $this->displaysOnStructuredUi(); } /** @@ -344,24 +326,6 @@ abstract class ChangesListFilter { return $this->priority; } - /** - * Checks whether the filter is allowed for the current context - * - * @param ChangesListSpecialPage $specialPage Current special page - * @return bool Whether it is allowed - */ - public function isAllowed( ChangesListSpecialPage $specialPage ) { - if ( $this->isAllowedCallable === null ) { - return true; - } else { - return call_user_func( - $this->isAllowedCallable, - get_class( $specialPage ), - $specialPage->getContext() - ); - } - } - /** * Gets the CSS class * diff --git a/includes/changes/ChangesListFilterGroup.php b/includes/changes/ChangesListFilterGroup.php index d2ad204546..4ff55201cc 100644 --- a/includes/changes/ChangesListFilterGroup.php +++ b/includes/changes/ChangesListFilterGroup.php @@ -332,12 +332,11 @@ abstract class ChangesListFilterGroup { /** * Gets the JS data in the format required by the front-end of the structured UI * - * @param ChangesListSpecialPage $specialPage * @return array|null Associative array, or null if there are no filters that * display in the structured UI. messageKeys is a special top-level value, with * the value being an array of the message keys to send to the client. */ - public function getJsData( ChangesListSpecialPage $specialPage ) { + public function getJsData() { $output = [ 'name' => $this->name, 'type' => $this->type, @@ -367,7 +366,7 @@ abstract class ChangesListFilterGroup { } ); foreach ( $this->filters as $filterName => $filter ) { - if ( $filter->displaysOnStructuredUi( $specialPage ) ) { + if ( $filter->displaysOnStructuredUi() ) { $filterData = $filter->getJsData(); $output['messageKeys'] = array_merge( $output['messageKeys'], diff --git a/includes/changes/ChangesListStringOptionsFilter.php b/includes/changes/ChangesListStringOptionsFilter.php index b6a877473c..1c977b9d4e 100644 --- a/includes/changes/ChangesListStringOptionsFilter.php +++ b/includes/changes/ChangesListStringOptionsFilter.php @@ -11,7 +11,7 @@ class ChangesListStringOptionsFilter extends ChangesListFilter { /** * @inheritdoc */ - public function displaysOnUnstructuredUi( ChangesListSpecialPage $specialPage ) { + public function displaysOnUnstructuredUi() { return false; } } diff --git a/includes/changes/ChangesListStringOptionsFilterGroup.php b/includes/changes/ChangesListStringOptionsFilterGroup.php index 86ae33fddc..723ef3986d 100644 --- a/includes/changes/ChangesListStringOptionsFilterGroup.php +++ b/includes/changes/ChangesListStringOptionsFilterGroup.php @@ -187,9 +187,7 @@ class ChangesListStringOptionsFilterGroup extends ChangesListFilterGroup { $allowedFilterNames = []; foreach ( $this->filters as $filter ) { - if ( $filter->isAllowed( $specialPage ) ) { - $allowedFilterNames[] = $filter->getName(); - } + $allowedFilterNames[] = $filter->getName(); } if ( $value === self::ALL ) { @@ -234,8 +232,8 @@ class ChangesListStringOptionsFilterGroup extends ChangesListFilterGroup { /** * @inheritdoc */ - public function getJsData( ChangesListSpecialPage $specialPage ) { - $output = parent::getJsData( $specialPage ); + public function getJsData() { + $output = parent::getJsData(); $output['separator'] = self::SEPARATOR; $output['default'] = $this->getDefault(); diff --git a/includes/specialpage/ChangesListSpecialPage.php b/includes/specialpage/ChangesListSpecialPage.php index 2ece5aa676..ad9a248ec6 100644 --- a/includes/specialpage/ChangesListSpecialPage.php +++ b/includes/specialpage/ChangesListSpecialPage.php @@ -59,6 +59,13 @@ abstract class ChangesListSpecialPage extends SpecialPage { */ private $filterGroupDefinitions; + // Same format as filterGroupDefinitions, but for a single group (reviewStatus) + // that is registered conditionally. + private $reviewStatusFilterGroupDefinition; + + // Single filter registered conditionally + private $hideCategorizationFilterDefinition; + /** * Filter groups, and their contained filters * This is an associative array (with group name as key) of ChangesListFilterGroup objects. @@ -245,57 +252,13 @@ abstract class ChangesListSpecialPage extends SpecialPage { ] ], - [ - 'name' => 'reviewStatus', - 'title' => 'rcfilters-filtergroup-reviewstatus', - 'class' => ChangesListBooleanFilterGroup::class, - 'filters' => [ - [ - 'name' => 'hidepatrolled', - 'label' => 'rcfilters-filter-patrolled-label', - 'description' => 'rcfilters-filter-patrolled-description', - // rcshowhidepatr-show, rcshowhidepatr-hide - // wlshowhidepatr - 'showHideSuffix' => 'showhidepatr', - 'default' => false, - 'isAllowedCallable' => function ( $pageClassName, $context ) { - return $context->getUser()->useRCPatrol(); - }, - 'queryCallable' => function ( $specialClassName, $ctx, $dbr, &$tables, &$fields, &$conds, - &$query_options, &$join_conds ) { - - $conds[] = 'rc_patrolled = 0'; - }, - 'cssClassSuffix' => 'patrolled', - 'isRowApplicableCallable' => function ( $ctx, $rc ) { - return $rc->getAttribute( 'rc_patrolled' ); - }, - ], - [ - 'name' => 'hideunpatrolled', - 'label' => 'rcfilters-filter-unpatrolled-label', - 'description' => 'rcfilters-filter-unpatrolled-description', - 'default' => false, - 'isAllowedCallable' => function ( $pageClassName, $context ) { - return $context->getUser()->useRCPatrol(); - }, - 'queryCallable' => function ( $specialClassName, $ctx, $dbr, &$tables, &$fields, &$conds, - &$query_options, &$join_conds ) { - - $conds[] = 'rc_patrolled = 1'; - }, - 'cssClassSuffix' => 'unpatrolled', - 'isRowApplicableCallable' => function ( $ctx, $rc ) { - return !$rc->getAttribute( 'rc_patrolled' ); - }, - ], - ], - ], + // reviewStatus (conditional) [ 'name' => 'significance', 'title' => 'rcfilters-filtergroup-significance', 'class' => ChangesListBooleanFilterGroup::class, + 'priority' => -6, 'filters' => [ [ 'name' => 'hideminor', @@ -344,6 +307,7 @@ abstract class ChangesListSpecialPage extends SpecialPage { 'label' => 'rcfilters-filter-pageedits-label', 'description' => 'rcfilters-filter-pageedits-description', 'default' => false, + 'priority' => -2, 'queryCallable' => function ( $specialClassName, $ctx, $dbr, &$tables, &$fields, &$conds, &$query_options, &$join_conds ) { @@ -359,6 +323,7 @@ abstract class ChangesListSpecialPage extends SpecialPage { 'label' => 'rcfilters-filter-newpages-label', 'description' => 'rcfilters-filter-newpages-description', 'default' => false, + 'priority' => -3, 'queryCallable' => function ( $specialClassName, $ctx, $dbr, &$tables, &$fields, &$conds, &$query_options, &$join_conds ) { @@ -369,44 +334,91 @@ abstract class ChangesListSpecialPage extends SpecialPage { return $rc->getAttribute( 'rc_source' ) === RecentChange::SRC_NEW; }, ], + + // hidecategorization + [ - 'name' => 'hidecategorization', - 'label' => 'rcfilters-filter-categorization-label', - 'description' => 'rcfilters-filter-categorization-description', - // rcshowhidecategorization-show, rcshowhidecategorization-hide. - // wlshowhidecategorization - 'showHideSuffix' => 'showhidecategorization', - 'isAllowedCallable' => function ( $pageClassName, $context ) { - return $context->getConfig()->get( 'RCWatchCategoryMembership' ); + 'name' => 'hidelog', + 'label' => 'rcfilters-filter-logactions-label', + 'description' => 'rcfilters-filter-logactions-description', + 'default' => false, + 'priority' => -5, + 'queryCallable' => function ( $specialClassName, $ctx, $dbr, &$tables, &$fields, &$conds, + &$query_options, &$join_conds ) { + + $conds[] = 'rc_type != ' . $dbr->addQuotes( RC_LOG ); }, + 'cssClassSuffix' => 'src-mw-log', + 'isRowApplicableCallable' => function ( $ctx, $rc ) { + return $rc->getAttribute( 'rc_source' ) === RecentChange::SRC_LOG; + } + ], + ], + ], + ]; + + $this->reviewStatusFilterGroupDefinition = [ + [ + 'name' => 'reviewStatus', + 'title' => 'rcfilters-filtergroup-reviewstatus', + 'class' => ChangesListBooleanFilterGroup::class, + 'priority' => -5, + 'filters' => [ + [ + 'name' => 'hidepatrolled', + 'label' => 'rcfilters-filter-patrolled-label', + 'description' => 'rcfilters-filter-patrolled-description', + // rcshowhidepatr-show, rcshowhidepatr-hide + // wlshowhidepatr + 'showHideSuffix' => 'showhidepatr', 'default' => false, 'queryCallable' => function ( $specialClassName, $ctx, $dbr, &$tables, &$fields, &$conds, &$query_options, &$join_conds ) { - $conds[] = 'rc_type != ' . $dbr->addQuotes( RC_CATEGORIZE ); + $conds[] = 'rc_patrolled = 0'; }, - 'cssClassSuffix' => 'src-mw-categorize', + 'cssClassSuffix' => 'patrolled', 'isRowApplicableCallable' => function ( $ctx, $rc ) { - return $rc->getAttribute( 'rc_source' ) === RecentChange::SRC_CATEGORIZE; + return $rc->getAttribute( 'rc_patrolled' ); }, ], [ - 'name' => 'hidelog', - 'label' => 'rcfilters-filter-logactions-label', - 'description' => 'rcfilters-filter-logactions-description', + 'name' => 'hideunpatrolled', + 'label' => 'rcfilters-filter-unpatrolled-label', + 'description' => 'rcfilters-filter-unpatrolled-description', 'default' => false, 'queryCallable' => function ( $specialClassName, $ctx, $dbr, &$tables, &$fields, &$conds, &$query_options, &$join_conds ) { - $conds[] = 'rc_type != ' . $dbr->addQuotes( RC_LOG ); + $conds[] = 'rc_patrolled = 1'; }, - 'cssClassSuffix' => 'src-mw-log', + 'cssClassSuffix' => 'unpatrolled', 'isRowApplicableCallable' => function ( $ctx, $rc ) { - return $rc->getAttribute( 'rc_source' ) === RecentChange::SRC_LOG; - } + return !$rc->getAttribute( 'rc_patrolled' ); + }, ], ], - ], + ] + ]; + + $this->hideCategorizationFilterDefinition = [ + 'name' => 'hidecategorization', + 'label' => 'rcfilters-filter-categorization-label', + 'description' => 'rcfilters-filter-categorization-description', + // rcshowhidecategorization-show, rcshowhidecategorization-hide. + // wlshowhidecategorization + 'showHideSuffix' => 'showhidecategorization', + 'default' => false, + 'priority' => -4, + 'queryCallable' => function ( $specialClassName, $ctx, $dbr, &$tables, &$fields, &$conds, + &$query_options, &$join_conds ) { + + $conds[] = 'rc_type != ' . $dbr->addQuotes( RC_CATEGORIZE ); + }, + 'cssClassSuffix' => 'src-mw-categorize', + 'isRowApplicableCallable' => function ( $ctx, $rc ) { + return $rc->getAttribute( 'rc_source' ) === RecentChange::SRC_CATEGORIZE; + }, ]; } @@ -503,7 +515,8 @@ abstract class ChangesListSpecialPage extends SpecialPage { } /** - * Register all filters and their groups, plus conflicts + * Register all filters and their groups (including those from hooks), plus handle + * conflicts and defaults. * * You might want to customize these in the same method, in subclasses. You can * call getFilterGroup to access a group, and (on the group) getFilter to access a @@ -513,6 +526,27 @@ abstract class ChangesListSpecialPage extends SpecialPage { protected function registerFilters() { $this->registerFiltersFromDefinitions( $this->filterGroupDefinitions ); + // Make sure this is not being transcluded (we don't want to show this + // information to all users just because the user that saves the edit can + // patrol) + if ( !$this->including() && $this->getUser()->useRCPatrol() ) { + $this->registerFiltersFromDefinitions( $this->reviewStatusFilterGroupDefinition ); + } + + $changeTypeGroup = $this->getFilterGroup( 'changeType' ); + + if ( $this->getConfig()->get( 'RCWatchCategoryMembership' ) ) { + $transformedHideCategorizationDef = $this->transformFilterDefinition( + $this->hideCategorizationFilterDefinition + ); + + $transformedHideCategorizationDef['group'] = $changeTypeGroup; + + $hideCategorization = new ChangesListBooleanFilter( + $transformedHideCategorizationDef + ); + } + Hooks::run( 'ChangesListSpecialPageStructuredFilters', [ $this ] ); $unstructuredGroupDefinition = @@ -536,7 +570,6 @@ abstract class ChangesListSpecialPage extends SpecialPage { 'rcfilters-filter-unregistered-conflicts-user-experience-level' ); - $changeTypeGroup = $this->getFilterGroup( 'changeType' ); $categoryFilter = $changeTypeGroup->getFilter( 'hidecategorization' ); $logactionsFilter = $changeTypeGroup->getFilter( 'hidelog' ); $pagecreationFilter = $changeTypeGroup->getFilter( 'hidenewpages' ); @@ -544,12 +577,15 @@ abstract class ChangesListSpecialPage extends SpecialPage { $significanceTypeGroup = $this->getFilterGroup( 'significance' ); $hideMinorFilter = $significanceTypeGroup->getFilter( 'hideminor' ); - $hideMinorFilter->conflictsWith( - $categoryFilter, - 'rcfilters-hideminor-conflicts-typeofchange-global', - 'rcfilters-hideminor-conflicts-typeofchange', - 'rcfilters-typeofchange-conflicts-hideminor' - ); + // categoryFilter is conditional; see registerFilters + if ( $categoryFilter !== null ) { + $hideMinorFilter->conflictsWith( + $categoryFilter, + 'rcfilters-hideminor-conflicts-typeofchange-global', + 'rcfilters-hideminor-conflicts-typeofchange', + 'rcfilters-typeofchange-conflicts-hideminor' + ); + } $hideMinorFilter->conflictsWith( $logactionsFilter, 'rcfilters-hideminor-conflicts-typeofchange-global', @@ -564,23 +600,46 @@ abstract class ChangesListSpecialPage extends SpecialPage { ); } + /** + * Transforms filter definition to prepare it for constructor. + * + * See overrides of this method as well. + * + * @param array $filterDefinition Original filter definition + * + * @return array Transformed definition + */ + protected function transformFilterDefinition( array $filterDefinition ) { + return $filterDefinition; + } + /** * Register filters from a definition object * * Array specifying groups and their filters; see Filter and * ChangesListFilterGroup constructors. * - * There is light processing to simplify core maintenance. See overrides - * of this method as well. + * There is light processing to simplify core maintenance. */ protected function registerFiltersFromDefinitions( array $definition ) { - $priority = -1; + $autoFillPriority = -1; foreach ( $definition as $groupDefinition ) { - $groupDefinition['priority'] = $priority; - $priority--; + if ( !isset( $groupDefinition['priority'] ) ) { + $groupDefinition['priority'] = $autoFillPriority; + } else { + // If it's explicitly specified, start over the auto-fill + $autoFillPriority = $groupDefinition['priority']; + } + + $autoFillPriority--; $className = $groupDefinition['class']; unset( $groupDefinition['class'] ); + + foreach ( $groupDefinition['filters'] as &$filterDefinition ) { + $filterDefinition = $this->transformFilterDefinition( $filterDefinition ); + } + $this->registerFilterGroup( new $className( $groupDefinition ) ); } } @@ -846,7 +905,7 @@ abstract class ChangesListSpecialPage extends SpecialPage { $query_options, $join_conds, $opts[$filterGroup->getName()] ); } else { foreach ( $filterGroup->getFilters() as $filter ) { - if ( $opts[$filter->getName()] && $filter->isAllowed( $this ) ) { + if ( $opts[$filter->getName()] ) { $filter->modifyQuery( $dbr, $this, $tables, $fields, $conds, $query_options, $join_conds ); } diff --git a/includes/specials/SpecialRecentchanges.php b/includes/specials/SpecialRecentchanges.php index 3e7971aa40..1f88d61a64 100644 --- a/includes/specials/SpecialRecentchanges.php +++ b/includes/specials/SpecialRecentchanges.php @@ -91,16 +91,12 @@ class SpecialRecentChanges extends ChangesListSpecialPage { /** * @inheritdoc */ - protected function registerFiltersFromDefinitions( array $definition ) { - foreach ( $definition as $groupName => &$groupDefinition ) { - foreach ( $groupDefinition['filters'] as &$filterDefinition ) { - if ( isset( $filterDefinition['showHideSuffix'] ) ) { - $filterDefinition['showHide'] = 'rc' . $filterDefinition['showHideSuffix']; - } - } + protected function transformFilterDefinition( array $filterDefinition ) { + if ( isset( $filterDefinition['showHideSuffix'] ) ) { + $filterDefinition['showHide'] = 'rc' . $filterDefinition['showHideSuffix']; } - parent::registerFiltersFromDefinitions( $definition ); + return $filterDefinition; } /** @@ -120,12 +116,18 @@ class SpecialRecentChanges extends ChangesListSpecialPage { $hideBots->setDefault( true ); $reviewStatus = $this->getFilterGroup( 'reviewStatus' ); - $hidePatrolled = $reviewStatus->getFilter( 'hidepatrolled' ); - $hidePatrolled->setDefault( $user->getBoolOption( 'hidepatrolled' ) ); + if ( $reviewStatus !== null ) { + // Conditional on feature being available and rights + $hidePatrolled = $reviewStatus->getFilter( 'hidepatrolled' ); + $hidePatrolled->setDefault( $user->getBoolOption( 'hidepatrolled' ) ); + } $changeType = $this->getFilterGroup( 'changeType' ); $hideCategorization = $changeType->getFilter( 'hidecategorization' ); - $hideCategorization->setDefault( $user->getBoolOption( 'hidecategorization' ) ); + if ( $hideCategorization !== null ) { + // Conditional on feature being available + $hideCategorization->setDefault( $user->getBoolOption( 'hidecategorization' ) ); + } } /** diff --git a/includes/specials/SpecialWatchlist.php b/includes/specials/SpecialWatchlist.php index 9066148564..365736f52d 100644 --- a/includes/specials/SpecialWatchlist.php +++ b/includes/specials/SpecialWatchlist.php @@ -108,16 +108,12 @@ class SpecialWatchlist extends ChangesListSpecialPage { /** * @inheritdoc */ - protected function registerFiltersFromDefinitions( array $definition ) { - foreach ( $definition as $groupName => &$groupDefinition ) { - foreach ( $groupDefinition['filters'] as &$filterDefinition ) { - if ( isset( $filterDefinition['showHideSuffix'] ) ) { - $filterDefinition['showHide'] = 'wl' . $filterDefinition['showHideSuffix']; - } - } + protected function transformFilterDefinition( array $filterDefinition ) { + if ( isset( $filterDefinition['showHideSuffix'] ) ) { + $filterDefinition['showHide'] = 'wl' . $filterDefinition['showHideSuffix']; } - parent::registerFiltersFromDefinitions( $definition ); + return $filterDefinition; } /** @@ -143,8 +139,11 @@ class SpecialWatchlist extends ChangesListSpecialPage { $hideLiu->setDefault( $user->getBoolOption( 'watchlisthideliu' ) ); $reviewStatus = $this->getFilterGroup( 'reviewStatus' ); - $hidePatrolled = $reviewStatus->getFilter( 'hidepatrolled' ); - $hidePatrolled->setDefault( $user->getBoolOption( 'watchlisthidepatrolled' ) ); + if ( $reviewStatus !== null ) { + // Conditional on feature being available and rights + $hidePatrolled = $reviewStatus->getFilter( 'hidepatrolled' ); + $hidePatrolled->setDefault( $user->getBoolOption( 'watchlisthidepatrolled' ) ); + } $authorship = $this->getFilterGroup( 'authorship' ); $hideMyself = $authorship->getFilter( 'hidemyself' ); @@ -152,7 +151,10 @@ class SpecialWatchlist extends ChangesListSpecialPage { $changeType = $this->getFilterGroup( 'changeType' ); $hideCategorization = $changeType->getFilter( 'hidecategorization' ); - $hideCategorization->setDefault( $user->getBoolOption( 'watchlisthidecategorization' ) ); + if ( $hideCategorization !== null ) { + // Conditional on feature being available + $hideCategorization->setDefault( $user->getBoolOption( 'watchlisthidecategorization' ) ); + } } /** diff --git a/tests/phpunit/includes/changes/ChangesListBooleanFilterGroupTest.php b/tests/phpunit/includes/changes/ChangesListBooleanFilterGroupTest.php index d98311f1b6..a30702f480 100644 --- a/tests/phpunit/includes/changes/ChangesListBooleanFilterGroupTest.php +++ b/tests/phpunit/includes/changes/ChangesListBooleanFilterGroupTest.php @@ -45,13 +45,6 @@ class ChangesListBooleanFilterGroupTest extends MediaWikiTestCase { $group = new ChangesListBooleanFilterGroup( $definition ); - $specialPage = $this->getMockBuilder( 'ChangesListSpecialPage' ) - ->setConstructorArgs( [ - 'ChangesListSpecialPage', - '', - ] ) - ->getMockForAbstractClass(); - $this->assertArrayEquals( [ 'name' => 'some-group', @@ -91,7 +84,7 @@ class ChangesListBooleanFilterGroupTest extends MediaWikiTestCase { ], ], - $group->getJsData( $specialPage ), + $group->getJsData(), /** ordered= */ false, /** named= */ true ); diff --git a/tests/phpunit/includes/changes/ChangesListBooleanFilterTest.php b/tests/phpunit/includes/changes/ChangesListBooleanFilterTest.php index 2c0c22d7b5..000f01772d 100644 --- a/tests/phpunit/includes/changes/ChangesListBooleanFilterTest.php +++ b/tests/phpunit/includes/changes/ChangesListBooleanFilterTest.php @@ -108,13 +108,6 @@ class ChangesListBooleanFilterTest extends MediaWikiTestCase { } public function testIsFeatureAvailableOnStructuredUi() { - $specialPage = $this->getMockBuilder( 'ChangesListSpecialPage' ) - ->setConstructorArgs( [ - 'ChangesListSpecialPage', - '', - ] ) - ->getMockForAbstractClass(); - $groupA = new ChangesListBooleanFilterGroup( [ 'name' => 'groupA', 'priority' => 1, @@ -133,7 +126,7 @@ class ChangesListBooleanFilterTest extends MediaWikiTestCase { $this->assertEquals( true, - $foo->isFeatureAvailableOnStructuredUi( $specialPage ), + $foo->isFeatureAvailableOnStructuredUi(), 'Same filter appears on both' ); @@ -148,7 +141,7 @@ class ChangesListBooleanFilterTest extends MediaWikiTestCase { $this->assertEquals( false, - $bar->isFeatureAvailableOnStructuredUi( $specialPage ), + $bar->isFeatureAvailableOnStructuredUi(), 'Only on unstructured UI' ); @@ -163,7 +156,7 @@ class ChangesListBooleanFilterTest extends MediaWikiTestCase { $this->assertEquals( true, - $baz->isFeatureAvailableOnStructuredUi( $specialPage ), + $baz->isFeatureAvailableOnStructuredUi(), 'Legacy filter does not appear directly in new UI, but equivalent ' . 'does and is marked with isReplacedInStructuredUi' ); diff --git a/tests/phpunit/includes/changes/ChangesListStringOptionsFilterGroupTest.php b/tests/phpunit/includes/changes/ChangesListStringOptionsFilterGroupTest.php index 019e2570d9..dc868a89b6 100644 --- a/tests/phpunit/includes/changes/ChangesListStringOptionsFilterGroupTest.php +++ b/tests/phpunit/includes/changes/ChangesListStringOptionsFilterGroupTest.php @@ -73,12 +73,6 @@ class ChangesListStringOptionsFilterGroupTest extends MediaWikiTestCase { [ 'name' => 'foo', ], - [ - 'name' => 'bar', - 'isAllowedCallable' => function () { - return false; - }, - ], [ 'name' => 'baz', ], @@ -141,25 +135,7 @@ class ChangesListStringOptionsFilterGroupTest extends MediaWikiTestCase { } public function provideNoOpModifyQuery() { - $isAllowedFalse = [ - 'isAllowedCallable' => function () { - return false; - }, - ]; - - $allDisallowedFilters = [ - [ - 'name' => 'disallowed1', - ] + $isAllowedFalse, - - [ - 'name' => 'disallowed2', - ] + $isAllowedFalse, - - [ - 'name' => 'disallowed3', - ] + $isAllowedFalse, - ]; + $noFilters = []; $normalFilters = [ [ @@ -172,9 +148,9 @@ class ChangesListStringOptionsFilterGroupTest extends MediaWikiTestCase { return [ [ - $allDisallowedFilters, + $noFilters, 'disallowed1;disallowed3', - 'The queryCallable should not be called if no filters are allowed', + 'The queryCallable should not be called if there are no filters', ], [ @@ -254,8 +230,6 @@ class ChangesListStringOptionsFilterGroupTest extends MediaWikiTestCase { $group = new ChangesListStringOptionsFilterGroup( $definition ); - $specialPage = $this->getSpecialPage(); - $this->assertArrayEquals( [ 'name' => 'some-group', @@ -294,7 +268,7 @@ class ChangesListStringOptionsFilterGroupTest extends MediaWikiTestCase { 'foo-description', ], ], - $group->getJsData( $specialPage ), + $group->getJsData(), /** ordered= */ false, /** named= */ true ); diff --git a/tests/phpunit/includes/specialpage/AbstractChangesListSpecialPageTestCase.php b/tests/phpunit/includes/specialpage/AbstractChangesListSpecialPageTestCase.php index 621d6a240a..03e341afeb 100644 --- a/tests/phpunit/includes/specialpage/AbstractChangesListSpecialPageTestCase.php +++ b/tests/phpunit/includes/specialpage/AbstractChangesListSpecialPageTestCase.php @@ -13,15 +13,42 @@ abstract class AbstractChangesListSpecialPageTestCase extends MediaWikiTestCase */ protected $changesListSpecialPage; + protected $oldPatrollersGroup; + protected function setUp() { + global $wgGroupPermissions; + parent::setUp(); $this->setMwGlobals( 'wgRCWatchCategoryMembership', true ); + + if ( isset( $wgGroupPermissions['patrollers'] ) ) { + $this->oldPatrollersGroup = $wgGroupPermissions['patrollers']; + } + + $wgGroupPermissions['patrollers'] = [ + 'patrol' => true, + ]; + } + + protected function tearDown() { + global $wgGroupPermissions; + + parent::tearDown(); + + if ( $this->oldPatrollersGroup !== null ) { + $wgGroupPermissions['patrollers'] = $this->oldPatrollersGroup; + } } /** * @dataProvider provideParseParameters */ public function testParseParameters( $params, $expected ) { + $context = $this->changesListSpecialPage->getContext(); + $context = new DerivativeContext( $context ); + $context->setUser( $this->getTestUser( [ 'patrollers' ] )->getUser() ); + $this->changesListSpecialPage->setContext( $context ); + $this->changesListSpecialPage->registerFilters(); $opts = new FormOptions(); diff --git a/tests/phpunit/mocks/MockChangesListFilter.php b/tests/phpunit/mocks/MockChangesListFilter.php index cbf306ed57..aeb9f0f22c 100644 --- a/tests/phpunit/mocks/MockChangesListFilter.php +++ b/tests/phpunit/mocks/MockChangesListFilter.php @@ -1,7 +1,7 @@