RCFilters: Remove isAllowedCallable and isAllowed
authorMatthew Flaschen <mflaschen@wikimedia.org>
Fri, 31 Mar 2017 05:07:31 +0000 (01:07 -0400)
committerMatthew Flaschen <mflaschen@wikimedia.org>
Fri, 31 Mar 2017 05:49:44 +0000 (01:49 -0400)
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

13 files changed:
includes/changes/ChangesListBooleanFilter.php
includes/changes/ChangesListFilter.php
includes/changes/ChangesListFilterGroup.php
includes/changes/ChangesListStringOptionsFilter.php
includes/changes/ChangesListStringOptionsFilterGroup.php
includes/specialpage/ChangesListSpecialPage.php
includes/specials/SpecialRecentchanges.php
includes/specials/SpecialWatchlist.php
tests/phpunit/includes/changes/ChangesListBooleanFilterGroupTest.php
tests/phpunit/includes/changes/ChangesListBooleanFilterTest.php
tests/phpunit/includes/changes/ChangesListStringOptionsFilterGroupTest.php
tests/phpunit/includes/specialpage/AbstractChangesListSpecialPageTestCase.php
tests/phpunit/mocks/MockChangesListFilter.php

index d0c4b77..4117a11 100644 (file)
@@ -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();
        }
 
        /**
index 22e797d..b3a16a8 100644 (file)
@@ -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
         *
index d2ad204..4ff5520 100644 (file)
@@ -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'],
index b6a8774..1c977b9 100644 (file)
@@ -11,7 +11,7 @@ class ChangesListStringOptionsFilter extends ChangesListFilter {
        /**
         * @inheritdoc
         */
-       public function displaysOnUnstructuredUi( ChangesListSpecialPage $specialPage ) {
+       public function displaysOnUnstructuredUi() {
                return false;
        }
 }
index 86ae33f..723ef39 100644 (file)
@@ -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();
index 2ece5aa..ad9a248 100644 (file)
@@ -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 );
                                        }
index 3e7971a..1f88d61 100644 (file)
@@ -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' ) );
+               }
        }
 
        /**
index 9066148..365736f 100644 (file)
@@ -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' ) );
+               }
        }
 
        /**
index d98311f..a30702f 100644 (file)
@@ -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
                );
index 2c0c22d..000f017 100644 (file)
@@ -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'
                );
index 019e257..dc868a8 100644 (file)
@@ -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
                );
index 621d6a2..03e341a 100644 (file)
@@ -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();
index cbf306e..aeb9f0f 100644 (file)
@@ -1,7 +1,7 @@
 <?php
 
 class MockChangesListFilter extends ChangesListFilter {
-       public function displaysOnUnstructuredUi( ChangesListSpecialPage $specialPage ) {
+       public function displaysOnUnstructuredUi() {
                throw new MWException(
                        'Not implemented: If the test relies on this, put it one of the ' .
                        'subclasses\' tests (e.g. ChangesListBooleanFilterTest) ' .