From: Matthew Flaschen <mflaschen@wikimedia.org>
Date: Thu, 16 Mar 2017 04:06:02 +0000 (-0400)
Subject: RCFilters: Prevent duplicate filter names
X-Git-Tag: 1.31.0-rc.0~3788^2
X-Git-Url: http://git.cyclocoop.org/%22%20.%20generer_url_aide%28?a=commitdiff_plain;h=2f6f69e834c1fa665d3552ad415d4315e6cc41a3;p=lhc%2Fweb%2Fwiklou.git

RCFilters: Prevent duplicate filter names

Explicitly block two filters in the same group from having the same
name.

Before, it would be left to registerFilter, which would just cause
the second one to win.

Also, avoid a getFilter warning when the filter does not exist.
Do the same for getFilterGroup on ChangesListSpecialPage

Finally, a minor related doc fix.

Change-Id: I6b3880a5c7cc381c169bbd969cd4814559b49c91
---

diff --git a/docs/hooks.txt b/docs/hooks.txt
index 149ee4b036..1be9a034e6 100644
--- 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 cd7415449a..22e797d1cf 100644
--- 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 30ab55225f..d2ad204546 100644
--- 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 e92f697c71..8f702bab2d 100644
--- 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 f712a2f863..465a9d11f0 100644
--- 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 c212560c08..1d87aeb959 100644
--- 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