Merge "RCFilters: Prevent duplicate filter names"

diff --git a/docs/hooks.txt b/docs/hooks.txt
index 4b356dd..f307f45 100644 (file)
--- a/docs/hooks.txt
+++ b/docs/hooks.txt
@@ -1015,10 +1015,11 @@ $opts: FormOptions for this request
 'ChangesListSpecialPageStructuredFilters': Called to allow extensions to register
 filters for pages inheriting from ChangesListSpecialPage (in core: RecentChanges,
 RecentChangesLinked, and Watchlist).  Generally, you will want to construct
-new ChangesListBooleanFilter or ChangesListStringOptionsFilter objects.  You can
-then either add them to existing ChangesListFilterGroup objects (accessed through
-$special->getFilterGroup), or create your own.  If you create new groups, you
-must register them with $special->registerFilterGroup.
+new ChangesListBooleanFilter or ChangesListStringOptionsFilter objects.
+
+When constructing them, you specify which group they belong to.  You can reuse
+existing groups (accessed through $special->getFilterGroup), or create your own.
+If you create new groups, you must register them with $special->registerFilterGroup.
 $special: ChangesListSpecialPage instance
 
 'ChangeTagAfterDelete': Called after a change tag has been deleted (that is,

diff --git a/includes/changes/ChangesListFilter.php b/includes/changes/ChangesListFilter.php
index cd74154..22e797d 100644 (file)
--- a/includes/changes/ChangesListFilter.php
+++ b/includes/changes/ChangesListFilter.php
@@ -113,7 +113,8 @@ abstract class ChangesListFilter {
        const RESERVED_NAME_CHAR = '_';
 
        /**
-        * Create a new filter with the specified configuration.
+        * Creates a new filter with the specified configuration, and registers it to the
+        * specified group.
         *
         * It infers which UI (it can be either or both) to display the filter on based on
         * which messages are provided.
@@ -161,6 +162,11 @@ abstract class ChangesListFilter {
                        );
                }
 
+               if ( $this->group->getFilter( $filterDefinition['name'] ) ) {
+                       throw new MWException( 'Two filters in a group cannot have the ' .
+                               "same name: '{$filterDefinition['name']}'" );
+               }
+
                $this->name = $filterDefinition['name'];
 
                if ( isset( $filterDefinition['cssClassSuffix'] ) ) {

diff --git a/includes/changes/ChangesListFilterGroup.php b/includes/changes/ChangesListFilterGroup.php
index 30ab552..d2ad204 100644 (file)
--- a/includes/changes/ChangesListFilterGroup.php
+++ b/includes/changes/ChangesListFilterGroup.php
@@ -315,10 +315,10 @@ abstract class ChangesListFilterGroup {
         * Get filter by name
         *
         * @param string $name Filter name
-        * @return ChangesListFilter Specified filter
+        * @return ChangesListFilter|null Specified filter, or null if it is not registered
         */
        public function getFilter( $name ) {
-               return $this->filters[$name];
+               return isset( $this->filters[$name] ) ? $this->filters[$name] : null;
        }
 
        /**

diff --git a/includes/specialpage/ChangesListSpecialPage.php b/includes/specialpage/ChangesListSpecialPage.php
index e92f697..8f702ba 100644 (file)
--- a/includes/specialpage/ChangesListSpecialPage.php
+++ b/includes/specialpage/ChangesListSpecialPage.php
@@ -663,10 +663,12 @@ abstract class ChangesListSpecialPage extends SpecialPage {
         *
         * @param string $groupName Name of group
         *
-        * @return ChangesListFilterGroup
+        * @return ChangesListFilterGroup|null Group, or null if not registered
         */
        public function getFilterGroup( $groupName ) {
-               return $this->filterGroups[$groupName];
+               return isset( $this->filterGroups[$groupName] ) ?
+                       $this->filterGroups[$groupName] :
+                       null;
        }
 
        // Currently, this intentionally only includes filters that display

diff --git a/tests/phpunit/includes/changes/ChangesListFilterGroupTest.php b/tests/phpunit/includes/changes/ChangesListFilterGroupTest.php
index f712a2f..465a9d1 100644 (file)
--- a/tests/phpunit/includes/changes/ChangesListFilterGroupTest.php
+++ b/tests/phpunit/includes/changes/ChangesListFilterGroupTest.php
@@ -51,4 +51,29 @@ class ChangesListFilterGroupTest extends MediaWikiTestCase {
                        )
                );
        }
+
+       // Get without warnings
+       public function testGetFilter() {
+               $group = new MockChangesListFilterGroup(
+                       [
+                               'type' => 'some_type',
+                               'name' => 'groupName',
+                               'isFullCoverage' => true,
+                               'priority' => 1,
+                               'filters' => [
+                                       [ 'name' => 'foo' ],
+                               ],
+                       ]
+               );
+
+               $this->assertEquals(
+                       'foo',
+                       $group->getFilter( 'foo' )->getName()
+               );
+
+               $this->assertEquals(
+                       null,
+                       $group->getFilter( 'bar' )
+               );
+       }
 }

diff --git a/tests/phpunit/includes/changes/ChangesListFilterTest.php b/tests/phpunit/includes/changes/ChangesListFilterTest.php
index c212560..1d87aeb 100644 (file)
--- a/tests/phpunit/includes/changes/ChangesListFilterTest.php
+++ b/tests/phpunit/includes/changes/ChangesListFilterTest.php
@@ -40,6 +40,30 @@ class ChangesListFilterTest extends MediaWikiTestCase {
                );
        }
 
+       // @codingStandardsIgnoreStart
+       /**
+        * @expectedException MWException
+        * @expectedExceptionMessage Two filters in a group cannot have the same name: 'somename'
+        */
+       // @codingStandardsIgnoreEnd
+       public function testDuplicateName() {
+               new MockChangesListFilter(
+                       [
+                               'group' => $this->group,
+                               'name' => 'somename',
+                               'priority' => 1,
+                       ]
+               );
+
+               new MockChangesListFilter(
+                       [
+                               'group' => $this->group,
+                               'name' => 'somename',
+                               'priority' => 2,
+                       ]
+               );
+       }
+
        /**
         * @expectedException MWException
         * @expectedExceptionMessage Supersets can only be defined for filters in the same group