From 5bd1b43e6a3aeadc25abbf24aa1d893ad7d20bae Mon Sep 17 00:00:00 2001 From: Stephane Bisson Date: Tue, 6 Dec 2016 10:25:35 -0500 Subject: [PATCH] Fix contradictory RC filters and add back-compat Some combinations of RC filters should never appear together because they guarantee to return no data and cannot be visually represented in the new RC filters UI (ERI project). Examples include: * 'hidemyself' and 'hidebyothers' * 'hideminor' and 'hidemajor' * All of the filters in the changeType group (which is extended by extensions) This also handles an old special case, but it now redirects instead of doing it silently: hideanons=1 & hideliu=1 & hidebots=1 -> hideliu=1 & hidebots=1 hideanons=1 & hideliu=1 & hidebots=0 -> hidehumans=1 Bug: T151873 Change-Id: Id08dccd07b262ce61c9d38563f19a0ab181e2341 --- .../specialpage/ChangesListSpecialPage.php | 91 ++++++++++++++-- includes/specials/SpecialRecentchanges.php | 11 +- ...AbstractChangesListSpecialPageTestCase.php | 82 ++++++++++++-- .../ChangesListSpecialPageTest.php | 101 ++++++++++-------- .../specials/SpecialRecentchangesTest.php | 21 +++- 5 files changed, 232 insertions(+), 74 deletions(-) diff --git a/includes/specialpage/ChangesListSpecialPage.php b/includes/specialpage/ChangesListSpecialPage.php index 97a0f5009d..27e278babe 100644 --- a/includes/specialpage/ChangesListSpecialPage.php +++ b/includes/specialpage/ChangesListSpecialPage.php @@ -960,7 +960,87 @@ abstract class ChangesListSpecialPage extends SpecialPage { * @param FormOptions $opts */ public function validateOptions( FormOptions $opts ) { - // nothing by default + if ( $this->fixContradictoryOptions( $opts ) ) { + $query = wfArrayToCgi( $this->convertParamsForLink( $opts->getChangedValues() ) ); + $this->getOutput()->redirect( $this->getPageTitle()->getCanonicalURL( $query ) ); + } + } + + /** + * Fix invalid options by resetting pairs that should never appear together. + * + * @param FormOptions $opts + * @return bool True if any option was reset + */ + private function fixContradictoryOptions( FormOptions $opts ) { + $contradictorySets = []; + + $fixed = $this->fixBackwardsCompatibilityOptions( $opts ); + + foreach ( $this->filterGroups as $filterGroup ) { + if ( $filterGroup instanceof ChangesListBooleanFilterGroup ) { + $filters = $filterGroup->getFilters(); + $allInGroupEnabled = array_reduce( + $filters, + function ( $carry, $filter ) use ( $opts ) { + return $carry && $opts[ $filter->getName() ]; + }, + /* initialValue */ count( $filters ) > 0 + ); + + if ( $allInGroupEnabled ) { + foreach ( $filters as $filter ) { + $opts->reset( $filter->getName() ); + } + + $fixed = true; + } + } + } + + return $fixed; + } + + /** + * Fix a special case (hideanons=1 and hideliu=1) in a special way, for backwards + * compatibility. + * + * This is deprecated and may be removed. + * + * @param FormOptions $opts + * @return bool True if this change was mode + */ + private function fixBackwardsCompatibilityOptions( FormOptions $opts ) { + if ( $opts['hideanons'] && $opts['hideliu'] ) { + $opts->reset( 'hideanons' ); + if ( !$opts['hidebots'] ) { + $opts->reset( 'hideliu' ); + $opts['hidehumans'] = 1; + } + + return true; + } + + return false; + + } + + /** + * Convert parameters values from true/false to 1/0 + * so they are not omitted by wfArrayToCgi() + * Bug 36524 + * + * @param array $params + * @return array + */ + protected function convertParamsForLink( $params ) { + foreach ( $params as &$value ) { + if ( $value === false ) { + $value = '0'; + } + } + unset( $value ); + return $params; } /** @@ -1049,15 +1129,6 @@ abstract class ChangesListSpecialPage extends SpecialPage { '' ); - // It makes no sense to hide both anons and logged-in users. When this occurs, try a guess on - // what the user meant and either show only bots or force anons to be shown. - - // ------- - - // XXX: We're no longer doing this handling. To preserve back-compat, we need to complete - // T151873 (particularly the hideanons/hideliu/hidebots/hidehumans part) in conjunction - // with merging this. - if ( !$this->runMainQueryHook( $tables, $fields, $conds, $query_options, $join_conds, $opts ) ) { diff --git a/includes/specials/SpecialRecentchanges.php b/includes/specials/SpecialRecentchanges.php index a47d91bc29..f9164a189f 100644 --- a/includes/specials/SpecialRecentchanges.php +++ b/includes/specials/SpecialRecentchanges.php @@ -814,16 +814,7 @@ class SpecialRecentChanges extends ChangesListSpecialPage { * @return string */ function makeOptionsLink( $title, $override, $options, $active = false ) { - $params = $override + $options; - - // T38524: false values have be converted to "0" otherwise - // wfArrayToCgi() will omit it them. - foreach ( $params as &$value ) { - if ( $value === false ) { - $value = '0'; - } - } - unset( $value ); + $params = $this->convertParamsForLink( $override + $options ); if ( $active ) { $title = new HtmlArmor( '' . htmlspecialchars( $title ) . '' ); diff --git a/tests/phpunit/includes/specialpage/AbstractChangesListSpecialPageTestCase.php b/tests/phpunit/includes/specialpage/AbstractChangesListSpecialPageTestCase.php index 03e341afeb..b101857e5a 100644 --- a/tests/phpunit/includes/specialpage/AbstractChangesListSpecialPageTestCase.php +++ b/tests/phpunit/includes/specialpage/AbstractChangesListSpecialPageTestCase.php @@ -28,6 +28,20 @@ abstract class AbstractChangesListSpecialPageTestCase extends MediaWikiTestCase $wgGroupPermissions['patrollers'] = [ 'patrol' => true, ]; + + // Deprecated + $this->setTemporaryHook( + 'ChangesListSpecialPageFilters', + null + ); + + # setup the ChangesListSpecialPage (or subclass) object + $this->changesListSpecialPage = $this->getPage(); + $context = $this->changesListSpecialPage->getContext(); + $context = new DerivativeContext( $context ); + $context->setUser( $this->getTestUser( [ 'patrollers' ] )->getUser() ); + $this->changesListSpecialPage->setContext( $context ); + $this->changesListSpecialPage->registerFilters(); } protected function tearDown() { @@ -44,13 +58,6 @@ abstract class AbstractChangesListSpecialPageTestCase extends MediaWikiTestCase * @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(); foreach ( $expected as $key => $value ) { // Register it as null so sets aren't rejected. @@ -73,4 +80,65 @@ abstract class AbstractChangesListSpecialPageTestCase extends MediaWikiTestCase /** named= */ true ); } + + /** + * @dataProvider validateOptionsProvider + */ + public function testValidateOptions( $optionsToSet, $expectedRedirect, $expectedRedirectOptions ) { + $redirectQuery = []; + $redirected = false; + $output = $this->getMockBuilder( OutputPage::class ) + ->disableProxyingToOriginalMethods() + ->disableOriginalConstructor() + ->getMock(); + $output->method( 'redirect' )->willReturnCallback( + function ( $url ) use ( &$redirectQuery, &$redirected ) { + $urlParts = wfParseUrl( $url ); + $query = isset( $urlParts[ 'query' ] ) ? $urlParts[ 'query' ] : ''; + parse_str( $query, $redirectQuery ); + $redirected = true; + } + ); + $ctx = new RequestContext(); + + // Give users patrol permissions so we can test that. + $user = $this->getTestSysop()->getUser(); + $ctx->setUser( $user ); + + // Disable this hook or it could break changeType + // depending on which other extensions are running. + $this->setTemporaryHook( + 'ChangesListSpecialPageStructuredFilters', + null + ); + + $ctx->setOutput( $output ); + $clsp = $this->changesListSpecialPage; + $clsp->setContext( $ctx ); + $opts = $clsp->getDefaultOptions(); + + foreach ( $optionsToSet as $option => $value ) { + $opts->setValue( $option, $value ); + } + + $clsp->validateOptions( $opts ); + + $this->assertEquals( $expectedRedirect, $redirected, 'redirection' ); + + if ( $expectedRedirect ) { + if ( count( $expectedRedirectOptions ) > 0 ) { + $expectedRedirectOptions += [ + 'title' => $clsp->getPageTitle()->getPrefixedText(), + ]; + } + + $this->assertArrayEquals( + $expectedRedirectOptions, + $redirectQuery, + /* $ordered= */ false, + /* $named= */ true, + 'redirection query' + ); + } + } } diff --git a/tests/phpunit/includes/specialpage/ChangesListSpecialPageTest.php b/tests/phpunit/includes/specialpage/ChangesListSpecialPageTest.php index 60285734d5..c8c65dc4a0 100644 --- a/tests/phpunit/includes/specialpage/ChangesListSpecialPageTest.php +++ b/tests/phpunit/includes/specialpage/ChangesListSpecialPageTest.php @@ -15,23 +15,26 @@ use Wikimedia\TestingAccessWrapper; * @covers ChangesListSpecialPage */ class ChangesListSpecialPageTest extends AbstractChangesListSpecialPageTestCase { - protected function setUp() { - parent::setUp(); - - # setup the rc object - $this->changesListSpecialPage = $this->getPage(); - } - protected function getPage() { - return TestingAccessWrapper::newFromObject( - $this->getMockForAbstractClass( - 'ChangesListSpecialPage', + $mock = $this->getMockBuilder( ChangesListSpecialPage::class ) + ->setConstructorArgs( [ 'ChangesListSpecialPage', '' ] ) + ->setMethods( [ 'getPageTitle' ] ) + ->getMockForAbstractClass(); + + $mock->method( 'getPageTitle' )->willReturn( + Title::makeTitle( NS_SPECIAL, 'ChangesListSpecialPage' ) ); + + $mock = TestingAccessWrapper::newFromObject( + $mock + ); + + return $mock; } /** helper to test SpecialRecentchanges::buildMainQueryConds() */ @@ -48,6 +51,7 @@ class ChangesListSpecialPageTest extends AbstractChangesListSpecialPageTestCase } $this->changesListSpecialPage->setContext( $context ); + $this->changesListSpecialPage->filterGroups = []; $formOptions = $this->changesListSpecialPage->setup( null ); #  Filter out rc_timestamp conditions which depends on the test runtime @@ -230,22 +234,6 @@ class ChangesListSpecialPageTest extends AbstractChangesListSpecialPageTestCase ); } - public function testRcHidemyselfHidebyothersFilter() { - $user = $this->getTestUser()->getUser(); - $this->assertConditions( - [ # expected - "rc_user_text != '{$user->getName()}'", - "rc_user_text = '{$user->getName()}'", - ], - [ - 'hidemyself' => 1, - 'hidebyothers' => 1, - ], - "rc conditions: hidemyself=1 hidebyothers=1 (logged in)", - $user - ); - } - public function testRcHidepageedits() { $this->assertConditions( [ # expected @@ -372,22 +360,6 @@ class ChangesListSpecialPageTest extends AbstractChangesListSpecialPageTestCase ); } - public function testRcHidepatrolledHideunpatrolledFilter() { - $user = $this->getTestSysop()->getUser(); - $this->assertConditions( - [ # expected - "rc_patrolled = 0", - "rc_patrolled = 1", - ], - [ - 'hidepatrolled' => 1, - 'hideunpatrolled' => 1, - ], - "rc conditions: hidepatrolled=1 hideunpatrolled=1", - $user - ); - } - public function testHideCategorization() { $this->assertConditions( [ @@ -576,6 +548,8 @@ class ChangesListSpecialPageTest extends AbstractChangesListSpecialPageTestCase } public function testGetStructuredFilterJsData() { + $this->changesListSpecialPage->filterGroups = []; + $definition = [ [ 'name' => 'gub-group', @@ -893,4 +867,47 @@ class ChangesListSpecialPageTest extends AbstractChangesListSpecialPageTestCase $this->changesListSpecialPage->areFiltersInConflict() ); } + + public function validateOptionsProvider() { + return [ + [ + [ 'hideanons' => 1, 'hideliu' => 1, 'hidebots' => 1 ], + true, + [ 'hideliu' => 1, 'hidebots' => 1, ], + ], + + [ + [ 'hideanons' => 1, 'hideliu' => 1, 'hidebots' => 0 ], + true, + [ 'hidebots' => 0, 'hidehumans' => 1 ], + ], + + [ + [ 'hidemyself' => 1, 'hidebyothers' => 1 ], + true, + [], + ], + [ + [ 'hidebots' => 1, 'hidehumans' => 1 ], + true, + [], + ], + [ + [ 'hidepatrolled' => 1, 'hideunpatrolled' => 1 ], + true, + [], + ], + [ + [ 'hideminor' => 1, 'hidemajor' => 1 ], + true, + [], + ], + [ + // changeType + [ 'hidepageedits' => 1, 'hidenewpages' => 1, 'hidecategorization' => 1, 'hidelog' => 1, ], + true, + [], + ], + ]; + } } diff --git a/tests/phpunit/includes/specials/SpecialRecentchangesTest.php b/tests/phpunit/includes/specials/SpecialRecentchangesTest.php index e9c7d4b327..8f3f585de7 100644 --- a/tests/phpunit/includes/specials/SpecialRecentchangesTest.php +++ b/tests/phpunit/includes/specials/SpecialRecentchangesTest.php @@ -10,15 +10,15 @@ use Wikimedia\TestingAccessWrapper; * @covers SpecialRecentChanges */ class SpecialRecentchangesTest extends AbstractChangesListSpecialPageTestCase { - protected function setUp() { - parent::setUp(); - - # setup the CLSP object - $this->changesListSpecialPage = TestingAccessWrapper::newFromObject( + protected function getPage() { + return TestingAccessWrapper::newFromObject( new SpecialRecentchanges ); } + // Below providers should only be for features specific to + // RecentChanges. Otherwise, it should go in ChangesListSpecialPageTest + public function provideParseParameters() { return [ [ 'limit=123', [ 'limit' => '123' ] ], @@ -32,4 +32,15 @@ class SpecialRecentchangesTest extends AbstractChangesListSpecialPageTestCase { [ 'tagfilter=foo', [ 'tagfilter' => 'foo' ] ], ]; } + + public function validateOptionsProvider() { + return [ + [ + // hidebots=1 is default for Special:RecentChanges + [ 'hideanons' => 1, 'hideliu' => 1 ], + true, + [ 'hideliu' => 1 ], + ], + ]; + } } -- 2.20.1