Merge "RCFilters: Prevent duplicate filter names"

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,

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'] ) ) {

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;
 	}
 
 	/**

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

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' )
+		);
+	}
 }

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