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

diff --git a/includes/specialpage/ChangesListSpecialPage.php b/includes/specialpage/ChangesListSpecialPage.php
index 97a0f50..27e278b 100644 (file)
--- 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 a47d91b..f9164a1 100644 (file)
--- 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( '<strong>' . htmlspecialchars( $title ) . '</strong>' );

diff --git a/tests/phpunit/includes/specialpage/AbstractChangesListSpecialPageTestCase.php b/tests/phpunit/includes/specialpage/AbstractChangesListSpecialPageTestCase.php
index 03e341a..b101857 100644 (file)
--- 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 6028573..c8c65dc 100644 (file)
--- 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 e9c7d4b..8f3f585 100644 (file)
--- 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 ],
+                       ],
+               ];
+       }
 }