RCFilters: define consistent interface in ChangesListFilterGroup
authorStephane Bisson <sbisson@wikimedia.org>
Wed, 18 Oct 2017 10:28:43 +0000 (06:28 -0400)
committerRoan Kattouw <roan.kattouw@gmail.com>
Wed, 18 Oct 2017 19:44:51 +0000 (12:44 -0700)
Get rid of isPerGroupRequestParameter and define a consistent
interface that all filter groups can implement.

Change-Id: Ib904bcdc697c65722a0041ac611d1e00c577389f

includes/changes/ChangesListBooleanFilterGroup.php
includes/changes/ChangesListFilterGroup.php
includes/changes/ChangesListStringOptionsFilterGroup.php
includes/specialpage/ChangesListSpecialPage.php
includes/specials/SpecialRecentchanges.php
includes/specials/SpecialWatchlist.php
tests/phpunit/includes/changes/ChangesListStringOptionsFilterGroupTest.php
tests/phpunit/mocks/MockChangesListFilterGroup.php

index 0622211..7bab97b 100644 (file)
@@ -1,5 +1,7 @@
 <?php
 
+use Wikimedia\Rdbms\IDatabase;
+
 /**
  * If the group is active, any unchecked filters will
  * translate to hide parameters in the URL.  E.g. if 'Human (not bot)' is checked,
@@ -61,7 +63,27 @@ class ChangesListBooleanFilterGroup extends ChangesListFilterGroup {
        /**
         * @inheritDoc
         */
-       public function isPerGroupRequestParameter() {
-               return false;
+       public function modifyQuery( IDatabase $dbr, ChangesListSpecialPage $specialPage,
+               &$tables, &$fields, &$conds, &$query_options, &$join_conds,
+               FormOptions $opts, $isStructuredFiltersEnabled
+       ) {
+               /** @var ChangesListBooleanFilter $filter */
+               foreach ( $this->getFilters() as $filter ) {
+                       if ( $filter->isActive( $opts, $isStructuredFiltersEnabled ) ) {
+                               $filter->modifyQuery( $dbr, $specialPage, $tables, $fields, $conds,
+                                       $query_options, $join_conds );
+                       }
+               }
+       }
+
+       /**
+        * @inheritDoc
+        */
+       public function addOptions( FormOptions $opts, $allowDefaults, $isStructuredFiltersEnabled ) {
+               /** @var ChangesListBooleanFilter $filter */
+               foreach ( $this->getFilters() as $filter ) {
+                       $defaultValue = $allowDefaults ? $filter->getDefault( $isStructuredFiltersEnabled ) : false;
+                       $opts->add( $filter->getName(), $defaultValue );
+               }
        }
 }
index e9140da..48c6e84 100644 (file)
@@ -27,6 +27,8 @@
 // What to call it.  FilterStructure?  That would also let me make
 // setUnidirectionalConflict protected.
 
+use Wikimedia\Rdbms\IDatabase;
+
 /**
  * Represents a filter group (used on ChangesListSpecialPage and descendants)
  *
@@ -326,14 +328,6 @@ abstract class ChangesListFilterGroup {
                return isset( $this->filters[$name] ) ? $this->filters[$name] : null;
        }
 
-       /**
-        * Check whether the URL parameter is for the group, or for individual filters.
-        * Defaults can also be defined on the group if and only if this is true.
-        *
-        * @return bool True if and only if the URL parameter is per-group
-        */
-       abstract public function isPerGroupRequestParameter();
-
        /**
         * Gets the JS data in the format required by the front-end of the structured UI
         *
@@ -449,4 +443,34 @@ abstract class ChangesListFilterGroup {
                        }
                ) );
        }
+
+       /**
+        * Modifies the query to include the filter group.
+        *
+        * The modification is only done if the filter group is in effect.  This means that
+        * one or more valid and allowed filters were selected.
+        *
+        * @param IDatabase $dbr Database, for addQuotes, makeList, and similar
+        * @param ChangesListSpecialPage $specialPage Current special page
+        * @param array &$tables Array of tables; see IDatabase::select $table
+        * @param array &$fields Array of fields; see IDatabase::select $vars
+        * @param array &$conds Array of conditions; see IDatabase::select $conds
+        * @param array &$query_options Array of query options; see IDatabase::select $options
+        * @param array &$join_conds Array of join conditions; see IDatabase::select $join_conds
+        * @param FormOptions $opts Wrapper for the current request options and their defaults
+        * @param bool $isStructuredFiltersEnabled True if the Structured UI is currently enabled
+        */
+       abstract public function modifyQuery( IDatabase $dbr, ChangesListSpecialPage $specialPage,
+               &$tables, &$fields, &$conds, &$query_options, &$join_conds,
+               FormOptions $opts, $isStructuredFiltersEnabled );
+
+       /**
+        * All the options represented by this filter group to $opts
+        *
+        * @param FormOptions $opts
+        * @param bool $allowDefaults
+        * @param bool $isStructuredFiltersEnabled
+        */
+       abstract public function addOptions( FormOptions $opts, $allowDefaults,
+               $isStructuredFiltersEnabled );
 }
index 2d1521d..653344a 100644 (file)
@@ -35,7 +35,6 @@ use Wikimedia\Rdbms\IDatabase;
  *
  * @since 1.29
  */
-
 class ChangesListStringOptionsFilterGroup extends ChangesListFilterGroup {
        /**
         * Type marker, used by JavaScript
@@ -128,13 +127,6 @@ class ChangesListStringOptionsFilterGroup extends ChangesListFilterGroup {
                }
        }
 
-       /**
-        * @inheritDoc
-        */
-       public function isPerGroupRequestParameter() {
-               return true;
-       }
-
        /**
         * Sets default of filter group.
         *
@@ -170,23 +162,17 @@ class ChangesListStringOptionsFilterGroup extends ChangesListFilterGroup {
        }
 
        /**
-        * Modifies the query to include the filter group.
-        *
-        * The modification is only done if the filter group is in effect.  This means that
-        * one or more valid and allowed filters were selected.
-        *
-        * @param IDatabase $dbr Database, for addQuotes, makeList, and similar
-        * @param ChangesListSpecialPage $specialPage Current special page
-        * @param array &$tables Array of tables; see IDatabase::select $table
-        * @param array &$fields Array of fields; see IDatabase::select $vars
-        * @param array &$conds Array of conditions; see IDatabase::select $conds
-        * @param array &$query_options Array of query options; see IDatabase::select $options
-        * @param array &$join_conds Array of join conditions; see IDatabase::select $join_conds
-        * @param string $value URL parameter value
+        * @inheritDoc
         */
        public function modifyQuery( IDatabase $dbr, ChangesListSpecialPage $specialPage,
-               &$tables, &$fields, &$conds, &$query_options, &$join_conds, $value
+               &$tables, &$fields, &$conds, &$query_options, &$join_conds,
+               FormOptions $opts, $isStructuredFiltersEnabled
        ) {
+               if ( !$this->isActive( $isStructuredFiltersEnabled ) ) {
+                       return;
+               }
+
+               $value = $opts[ $this->getName() ];
                $allowedFilterNames = [];
                foreach ( $this->filters as $filter ) {
                        $allowedFilterNames[] = $filter->getName();
@@ -243,12 +229,19 @@ class ChangesListStringOptionsFilterGroup extends ChangesListFilterGroup {
                return $output;
        }
 
+       /**
+        * @inheritDoc
+        */
+       public function addOptions( FormOptions $opts, $allowDefaults, $isStructuredFiltersEnabled ) {
+               $opts->add( $this->getName(), $allowDefaults ? $this->getDefault() : '' );
+       }
+
        /**
         * Check if this filter group is currently active
         *
         * @param {boolean} $isStructuredUI Is structured filters UI current enabled
         */
-       public function isActive( $isStructuredUI ) {
+       private function isActive( $isStructuredUI ) {
                // STRING_OPTIONS filter groups are exclusively active on Structured UI
                return $isStructuredUI;
        }
index 3f45250..bd8c4b6 100644 (file)
@@ -970,6 +970,23 @@ abstract class ChangesListSpecialPage extends SpecialPage {
                return $unstructuredGroupDefinition;
        }
 
+       /**
+        * @return array The legacy show/hide toggle filters
+        */
+       protected function getLegacyShowHideFilters() {
+               $filters = [];
+               foreach ( $this->filterGroups as $group ) {
+                       if ( $group instanceof  ChangesListBooleanFilterGroup ) {
+                               foreach ( $group->getFilters() as $key => $filter ) {
+                                       if ( $filter->displaysOnUnstructuredUi( $this ) ) {
+                                               $filters[ $key ] = $filter;
+                                       }
+                               }
+                       }
+               }
+               return $filters;
+       }
+
        /**
         * Register all the filters, including legacy hook-driven ones.
         * Then create a FormOptions object with options as specified by the user
@@ -1010,19 +1027,9 @@ abstract class ChangesListSpecialPage extends SpecialPage {
                // If urlversion=2 is set, ignore the filter defaults and set them all to false/empty
                $useDefaults = $this->getRequest()->getInt( 'urlversion' ) !== 2;
 
-               // Add all filters
                /** @var ChangesListFilterGroup $filterGroup */
                foreach ( $this->filterGroups as $filterGroup ) {
-                       // URL parameters can be per-group, like 'userExpLevel',
-                       // or per-filter, like 'hideminor'.
-                       if ( $filterGroup->isPerGroupRequestParameter() ) {
-                               $opts->add( $filterGroup->getName(), $useDefaults ? $filterGroup->getDefault() : '' );
-                       } else {
-                               /** @var ChangesListBooleanFilter $filter */
-                               foreach ( $filterGroup->getFilters() as $filter ) {
-                                       $opts->add( $filter->getName(), $useDefaults ? $filter->getDefault( $structuredUI ) : false );
-                               }
-                       }
+                       $filterGroup->addOptions( $opts, $useDefaults, $structuredUI );
                }
 
                $opts->add( 'namespace', '', FormOptions::STRING );
@@ -1153,9 +1160,9 @@ abstract class ChangesListSpecialPage extends SpecialPage {
                // or per-filter, like 'hideminor'.
 
                foreach ( $this->filterGroups as $filterGroup ) {
-                       if ( $filterGroup->isPerGroupRequestParameter() ) {
+                       if ( $filterGroup instanceof ChangesListStringOptionsFilterGroup ) {
                                $stringParameterNameSet[$filterGroup->getName()] = true;
-                       } elseif ( $filterGroup->getType() === ChangesListBooleanFilterGroup::TYPE ) {
+                       } elseif ( $filterGroup instanceof ChangesListBooleanFilterGroup ) {
                                foreach ( $filterGroup->getFilters() as $filter ) {
                                        $hideParameterNameSet[$filter->getName()] = true;
                                }
@@ -1291,22 +1298,10 @@ abstract class ChangesListSpecialPage extends SpecialPage {
                $dbr = $this->getDB();
                $isStructuredUI = $this->isStructuredFilterUiEnabled();
 
+               /** @var ChangesListFilterGroup $filterGroup */
                foreach ( $this->filterGroups as $filterGroup ) {
-                       // URL parameters can be per-group, like 'userExpLevel',
-                       // or per-filter, like 'hideminor'.
-                       if ( $filterGroup->isPerGroupRequestParameter() ) {
-                               if ( $filterGroup->isActive( $isStructuredUI ) ) {
-                                       $filterGroup->modifyQuery( $dbr, $this, $tables, $fields, $conds,
-                                               $query_options, $join_conds, $opts[$filterGroup->getName()] );
-                               }
-                       } else {
-                               foreach ( $filterGroup->getFilters() as $filter ) {
-                                       if ( $filter->isActive( $opts, $isStructuredUI ) ) {
-                                               $filter->modifyQuery( $dbr, $this, $tables, $fields, $conds,
-                                                       $query_options, $join_conds );
-                                       }
-                               }
-                       }
+                       $filterGroup->modifyQuery( $dbr, $this, $tables, $fields, $conds,
+                               $query_options, $join_conds, $opts, $isStructuredUI );
                }
 
                // Namespace filtering
index f4135a4..c3ce78e 100644 (file)
@@ -915,40 +915,32 @@ class SpecialRecentChanges extends ChangesListSpecialPage {
 
                $links = [];
 
-               $filterGroups = $this->getFilterGroups();
-
-               foreach ( $filterGroups as $groupName => $group ) {
-                       if ( !$group->isPerGroupRequestParameter() ) {
-                               foreach ( $group->getFilters() as $key => $filter ) {
-                                       if ( $filter->displaysOnUnstructuredUi( $this ) ) {
-                                               $msg = $filter->getShowHide();
-                                               $linkMessage = $this->msg( $msg . '-' . $showhide[1 - $options[$key]] );
-                                               // Extensions can define additional filters, but don't need to define the corresponding
-                                               // messages. If they don't exist, just fall back to 'show' and 'hide'.
-                                               if ( !$linkMessage->exists() ) {
-                                                       $linkMessage = $this->msg( $showhide[1 - $options[$key]] );
-                                               }
-
-                                               $link = $this->makeOptionsLink( $linkMessage->text(),
-                                                       [ $key => 1 - $options[$key] ], $nondefaults );
-
-                                               $attribs = [
-                                                       'class' => "$msg rcshowhideoption clshowhideoption",
-                                                       'data-filter-name' => $filter->getName(),
-                                               ];
-
-                                               if ( $filter->isFeatureAvailableOnStructuredUi( $this ) ) {
-                                                       $attribs['data-feature-in-structured-ui'] = true;
-                                               }
-
-                                               $links[] = Html::rawElement(
-                                                       'span',
-                                                       $attribs,
-                                                       $this->msg( $msg )->rawParams( $link )->escaped()
-                                               );
-                                       }
-                               }
+               foreach ( $this->getLegacyShowHideFilters() as $key => $filter ) {
+                       $msg = $filter->getShowHide();
+                       $linkMessage = $this->msg( $msg . '-' . $showhide[1 - $options[$key]] );
+                       // Extensions can define additional filters, but don't need to define the corresponding
+                       // messages. If they don't exist, just fall back to 'show' and 'hide'.
+                       if ( !$linkMessage->exists() ) {
+                               $linkMessage = $this->msg( $showhide[1 - $options[$key]] );
+                       }
+
+                       $link = $this->makeOptionsLink( $linkMessage->text(),
+                               [ $key => 1 - $options[$key] ], $nondefaults );
+
+                       $attribs = [
+                               'class' => "$msg rcshowhideoption clshowhideoption",
+                               'data-filter-name' => $filter->getName(),
+                       ];
+
+                       if ( $filter->isFeatureAvailableOnStructuredUi( $this ) ) {
+                               $attribs['data-feature-in-structured-ui'] = true;
                        }
+
+                       $links[] = Html::rawElement(
+                               'span',
+                               $attribs,
+                               $this->msg( $msg )->rawParams( $link )->escaped()
+                       );
                }
 
                // show from this onward link
index fcf1bfb..921a6dd 100644 (file)
@@ -333,15 +333,8 @@ class SpecialWatchlist extends ChangesListSpecialPage {
 
                        // This is how we handle the fact that HTML forms don't submit
                        // unchecked boxes.
-                       foreach ( $this->filterGroups as $filterGroup ) {
-                               if ( $filterGroup instanceof ChangesListBooleanFilterGroup ) {
-                                       /** @var ChangesListBooleanFilter $filter */
-                                       foreach ( $filterGroup->getFilters() as $filter ) {
-                                               if ( $filter->displaysOnUnstructuredUi() ) {
-                                                       $allBooleansFalse[$filter->getName()] = false;
-                                               }
-                                       }
-                               }
+                       foreach ( $this->getLegacyShowHideFilters() as $filter ) {
+                               $allBooleansFalse[ $filter->getName() ] = false;
                        }
 
                        $params = $params + $allBooleansFalse;
@@ -646,21 +639,15 @@ class SpecialWatchlist extends ChangesListSpecialPage {
                # Spit out some control panel links
                $links = [];
                $namesOfDisplayedFilters = [];
-               foreach ( $this->getFilterGroups() as $groupName => $group ) {
-                       if ( !$group->isPerGroupRequestParameter() ) {
-                               foreach ( $group->getFilters() as $filterName => $filter ) {
-                                       if ( $filter->displaysOnUnstructuredUi( $this ) ) {
-                                               $namesOfDisplayedFilters[] = $filterName;
-                                               $links[] = $this->showHideCheck(
-                                                       $nondefaults,
-                                                       $filter->getShowHide(),
-                                                       $filterName,
-                                                       $opts[$filterName],
-                                                       $filter->isFeatureAvailableOnStructuredUi( $this )
-                                               );
-                                       }
-                               }
-                       }
+               foreach ( $this->getLegacyShowHideFilters() as $filterName => $filter ) {
+                       $namesOfDisplayedFilters[] = $filterName;
+                       $links[] = $this->showHideCheck(
+                               $nondefaults,
+                               $filter->getShowHide(),
+                               $filterName,
+                               $opts[ $filterName ],
+                               $filter->isFeatureAvailableOnStructuredUi( $this )
+                       );
                }
 
                $hiddenFields = $nondefaults;
index dd27e32..acac26f 100644 (file)
@@ -190,6 +190,8 @@ class ChangesListStringOptionsFilterGroupTest extends MediaWikiTestCase {
                $group = new ChangesListStringOptionsFilterGroup( $groupDefinition );
 
                $specialPage = $this->getSpecialPage();
+               $opts = new FormOptions();
+               $opts->add( $groupDefinition[ 'name' ], $input );
 
                $group->modifyQuery(
                        $dbr,
@@ -199,7 +201,8 @@ class ChangesListStringOptionsFilterGroupTest extends MediaWikiTestCase {
                        $conds,
                        $query_options,
                        $join_conds,
-                       $input
+                       $opts,
+                       true
                );
        }
 
index f2891ce..e50b9b4 100644 (file)
@@ -1,5 +1,7 @@
 <?php
 
+use Wikimedia\Rdbms\IDatabase;
+
 class MockChangesListFilterGroup extends ChangesListFilterGroup {
        public function createFilter( array $filterDefinition ) {
                return new MockChangesListFilter( $filterDefinition );
@@ -9,11 +11,11 @@ class MockChangesListFilterGroup extends ChangesListFilterGroup {
                $this->filters[$filter->getName()] = $filter;
        }
 
-       public function isPerGroupRequestParameter() {
-               throw new MWException(
-                       'Not implemented: If the test relies on this, put it one of the ' .
-                       'subclasses\' tests (e.g. ChangesListBooleanFilterGroupTest) ' .
-                       'instead of testing the abstract class'
-               );
+       public function modifyQuery( IDatabase $dbr, ChangesListSpecialPage $specialPage,
+               &$tables, &$fields, &$conds, &$query_options, &$join_conds, FormOptions $opts,
+               $isStructuredFiltersEnabled ) {
+       }
+
+       public function addOptions( FormOptions $opts, $allowDefaults, $isStructuredFiltersEnabled ) {
        }
 }