From d233035cb445953fff3478d79f8c111c911cd4ac Mon Sep 17 00:00:00 2001
From: Matthew Flaschen <mflaschen@wikimedia.org>
Date: Wed, 15 Mar 2017 00:23:29 -0400
Subject: [PATCH] RCFilters: Don't allow underscore in filter or group names

This is reserved for the client-side which joins 'someGroup'
and 'somefilter' to make 'someGroup__somefilter' as an internal
ID.

Change-Id: I1b6ca9f337dd48e10705c46ef5027c3156254e01
---
 includes/changes/ChangesListFilter.php          | 12 +++++++++++-
 includes/changes/ChangesListFilterGroup.php     | 11 ++++++++++-
 .../changes/ChangesListFilterGroupTest.php      | 17 +++++++++++++++++
 .../includes/changes/ChangesListFilterTest.php  | 16 ++++++++++++++++
 4 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/includes/changes/ChangesListFilter.php b/includes/changes/ChangesListFilter.php
index 4ac63878b8..cd7415449a 100644
--- a/includes/changes/ChangesListFilter.php
+++ b/includes/changes/ChangesListFilter.php
@@ -110,6 +110,8 @@ abstract class ChangesListFilter {
 	 */
 	protected $priority;
 
+	const RESERVED_NAME_CHAR = '_';
+
 	/**
 	 * Create a new filter with the specified configuration.
 	 *
@@ -122,7 +124,8 @@ abstract class ChangesListFilter {
 	 *
 	 * @param array $filterDefinition ChangesListFilter definition
 	 *
-	 * $filterDefinition['name'] string Name of filter
+	 * $filterDefinition['name'] string Name of filter; use lowercase with no
+	 *  punctuation
 	 * $filterDefinition['cssClassSuffix'] string CSS class suffix, used to mark
 	 *  that a particular row belongs to this filter (when a row is included by the
 	 *  filter) (optional)
@@ -151,6 +154,13 @@ abstract class ChangesListFilter {
 				'ChangesListFilterGroup this filter belongs to' );
 		}
 
+		if ( strpos( $filterDefinition['name'], self::RESERVED_NAME_CHAR ) !== false ) {
+			throw new MWException( 'Filter names may not contain \'' .
+				self::RESERVED_NAME_CHAR .
+				'\'.  Use the naming convention: \'lowercase\''
+			);
+		}
+
 		$this->name = $filterDefinition['name'];
 
 		if ( isset( $filterDefinition['cssClassSuffix'] ) ) {
diff --git a/includes/changes/ChangesListFilterGroup.php b/includes/changes/ChangesListFilterGroup.php
index a4cc287e8d..30ab55225f 100644
--- a/includes/changes/ChangesListFilterGroup.php
+++ b/includes/changes/ChangesListFilterGroup.php
@@ -123,11 +123,13 @@ abstract class ChangesListFilterGroup {
 
 	const DEFAULT_PRIORITY = -100;
 
+	const RESERVED_NAME_CHAR = '_';
+
 	/**
 	 * Create a new filter group with the specified configuration
 	 *
 	 * @param array $groupDefinition Configuration of group
-	 * * $groupDefinition['name'] string Group name
+	 * * $groupDefinition['name'] string Group name; use camelCase with no punctuation
 	 * * $groupDefinition['title'] string i18n key for title (optional, can be omitted
 	 * *  only if none of the filters in the group display in the structured UI)
 	 * * $groupDefinition['type'] string A type constant from a subclass of this one
@@ -142,6 +144,13 @@ abstract class ChangesListFilterGroup {
 	 * *  changes list entries are filtered out.
 	 */
 	public function __construct( array $groupDefinition ) {
+		if ( strpos( $groupDefinition['name'], self::RESERVED_NAME_CHAR ) !== false ) {
+			throw new MWException( 'Group names may not contain \'' .
+				self::RESERVED_NAME_CHAR .
+				'\'.  Use the naming convention: \'camelCase\''
+			);
+		}
+
 		$this->name = $groupDefinition['name'];
 
 		if ( isset( $groupDefinition['title'] ) ) {
diff --git a/tests/phpunit/includes/changes/ChangesListFilterGroupTest.php b/tests/phpunit/includes/changes/ChangesListFilterGroupTest.php
index 718d4b31ca..f712a2f863 100644
--- a/tests/phpunit/includes/changes/ChangesListFilterGroupTest.php
+++ b/tests/phpunit/includes/changes/ChangesListFilterGroupTest.php
@@ -4,6 +4,23 @@
  * @covers ChangesListFilterGroup
  */
 class ChangesListFilterGroupTest extends MediaWikiTestCase {
+	// @codingStandardsIgnoreStart
+	/**
+	 * @expectedException MWException
+	 * @expectedExceptionMessage Group names may not contain '_'.  Use the naming convention: 'camelCase'
+	 */
+	// @codingStandardsIgnoreEnd
+	public function testReservedCharacter() {
+		new MockChangesListFilterGroup(
+			[
+				'type' => 'some_type',
+				'name' => 'group_name',
+				'priority' => 1,
+				'filters' => [],
+			]
+		);
+	}
+
 	public function testAutoPriorities() {
 		$group = new MockChangesListFilterGroup(
 			[
diff --git a/tests/phpunit/includes/changes/ChangesListFilterTest.php b/tests/phpunit/includes/changes/ChangesListFilterTest.php
index b63482fc7a..c212560c08 100644
--- a/tests/phpunit/includes/changes/ChangesListFilterTest.php
+++ b/tests/phpunit/includes/changes/ChangesListFilterTest.php
@@ -24,6 +24,22 @@ class ChangesListFilterTest extends MediaWikiTestCase {
 
 	}
 
+	// @codingStandardsIgnoreStart
+	/**
+	 * @expectedException MWException
+	 * @expectedExceptionMessage Filter names may not contain '_'.  Use the naming convention: 'lowercase'
+	 */
+	// @codingStandardsIgnoreEnd
+	public function testReservedCharacter() {
+		$filter = new MockChangesListFilter(
+			[
+				'group' => $this->group,
+				'name' => 'some_name',
+				'priority' => 1,
+			]
+		);
+	}
+
 	/**
 	 * @expectedException MWException
 	 * @expectedExceptionMessage Supersets can only be defined for filters in the same group
-- 
2.20.1