From b31c87ab91b3e2bbb80d3b814953ea3790498c36 Mon Sep 17 00:00:00 2001 From: Stephane Bisson Date: Wed, 18 Oct 2017 06:28:43 -0400 Subject: [PATCH] RCFilters: define consistent interface in ChangesListFilterGroup Get rid of isPerGroupRequestParameter and define a consistent interface that all filter groups can implement. Change-Id: Ib904bcdc697c65722a0041ac611d1e00c577389f --- .../changes/ChangesListBooleanFilterGroup.php | 26 ++++++++- includes/changes/ChangesListFilterGroup.php | 40 ++++++++++--- .../ChangesListStringOptionsFilterGroup.php | 39 +++++-------- .../specialpage/ChangesListSpecialPage.php | 51 ++++++++-------- includes/specials/SpecialRecentchanges.php | 58 ++++++++----------- includes/specials/SpecialWatchlist.php | 35 ++++------- ...hangesListStringOptionsFilterGroupTest.php | 5 +- .../mocks/MockChangesListFilterGroup.php | 14 +++-- 8 files changed, 143 insertions(+), 125 deletions(-) diff --git a/includes/changes/ChangesListBooleanFilterGroup.php b/includes/changes/ChangesListBooleanFilterGroup.php index 0622211f0a..7bab97bac2 100644 --- a/includes/changes/ChangesListBooleanFilterGroup.php +++ b/includes/changes/ChangesListBooleanFilterGroup.php @@ -1,5 +1,7 @@ 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 ); + } } } diff --git a/includes/changes/ChangesListFilterGroup.php b/includes/changes/ChangesListFilterGroup.php index e9140da209..48c6e840a0 100644 --- a/includes/changes/ChangesListFilterGroup.php +++ b/includes/changes/ChangesListFilterGroup.php @@ -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 ); } diff --git a/includes/changes/ChangesListStringOptionsFilterGroup.php b/includes/changes/ChangesListStringOptionsFilterGroup.php index 2d1521d5d0..653344a983 100644 --- a/includes/changes/ChangesListStringOptionsFilterGroup.php +++ b/includes/changes/ChangesListStringOptionsFilterGroup.php @@ -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; } diff --git a/includes/specialpage/ChangesListSpecialPage.php b/includes/specialpage/ChangesListSpecialPage.php index 3f45250508..bd8c4b6ec7 100644 --- a/includes/specialpage/ChangesListSpecialPage.php +++ b/includes/specialpage/ChangesListSpecialPage.php @@ -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 diff --git a/includes/specials/SpecialRecentchanges.php b/includes/specials/SpecialRecentchanges.php index f4135a4d61..c3ce78e301 100644 --- a/includes/specials/SpecialRecentchanges.php +++ b/includes/specials/SpecialRecentchanges.php @@ -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 diff --git a/includes/specials/SpecialWatchlist.php b/includes/specials/SpecialWatchlist.php index fcf1bfb8b9..921a6dd465 100644 --- a/includes/specials/SpecialWatchlist.php +++ b/includes/specials/SpecialWatchlist.php @@ -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; diff --git a/tests/phpunit/includes/changes/ChangesListStringOptionsFilterGroupTest.php b/tests/phpunit/includes/changes/ChangesListStringOptionsFilterGroupTest.php index dd27e32e98..acac26f04c 100644 --- a/tests/phpunit/includes/changes/ChangesListStringOptionsFilterGroupTest.php +++ b/tests/phpunit/includes/changes/ChangesListStringOptionsFilterGroupTest.php @@ -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 ); } diff --git a/tests/phpunit/mocks/MockChangesListFilterGroup.php b/tests/phpunit/mocks/MockChangesListFilterGroup.php index f2891ce932..e50b9b4ed2 100644 --- a/tests/phpunit/mocks/MockChangesListFilterGroup.php +++ b/tests/phpunit/mocks/MockChangesListFilterGroup.php @@ -1,5 +1,7 @@ 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 ) { } } -- 2.20.1