[ticket/11538] Use regex for testing color value and improve tests

We are now using a regex with preg_match() in order to properly check
if the entered color value is in hex color format or not. A proper
error message is triggered if an incorrect color value is entered and
the prepended '#' is removed if necessary.

PHPBB3-11538

3 files changed, 30 insertions, 42 deletions

diff --git a/phpBB/includes/ucp/ucp_groups.php b/phpBB/includes/ucp/ucp_groups.php
index c1db19774a..3f06e74159 100644
--- a/phpBB/includes/ucp/ucp_groups.php
+++ b/phpBB/includes/ucp/ucp_groups.php
@@ -595,18 +595,22 @@ class ucp_groups
 								$error[] = $user->lang['FORM_INVALID'];
 							}
 
-							if (!sizeof($error))
+							if (!empty($submit_ary['colour']))
 							{
-								// Make sure maximum length of 6 of group color is not exceeded
-								if (strpos($submit_ary['colour'], '#') === 0)
+								preg_match('/^(#?)+(?:[0-9a-fA-F]{6}|[0-9a-fA-F]{3})\b/', $submit_ary['colour'], $group_colour);
+
+								if (sizeof($group_colour))
 								{
-									$submit_ary['colour'] = substr($submit_ary['colour'], 1, 6);
+									$submit_ary['colour'] = (strpos($group_colour[0], '#') !== false) ? str_replace('#', '', $group_colour[0]) : $group_colour[0];
 								}
 								else
 								{
-									$submit_ary['colour'] = substr($submit_ary['colour'], 0, 6);
+									$error[] = $user->lang['COLOUR_INVALID'];
 								}
+							}
 
+							if (!sizeof($error))
+							{
 								// Only set the rank, colour, etc. if it's changed or if we're adding a new
 								// group. This prevents existing group members being updated if no changes
 								// were made.
diff --git a/phpBB/language/en/common.php b/phpBB/language/en/common.php
index baf398b146..129deb551c 100644
--- a/phpBB/language/en/common.php
+++ b/phpBB/language/en/common.php
@@ -120,6 +120,7 @@ $lang = array_merge($lang, array(
 	'CLICK_VIEW_PRIVMSG'	=> '%sGo to your inbox%s',
 	'COLLAPSE_VIEW'			=> 'Collapse view',
 	'CLOSE_WINDOW'			=> 'Close window',
+	'COLOUR_INVALID'		=> 'The colour value you entered is invalid.',
 	'COLOUR_SWATCH'			=> 'Colour swatch',
 	'COMMA_SEPARATOR'		=> ', ',	// Used in pagination of ACP & prosilver, use localised comma if appropriate, eg: Ideographic or Arabic
 	'CONFIRM'				=> 'Confirm',
diff --git a/tests/functional/ucp_groups_test.php b/tests/functional/ucp_groups_test.php
index 010727cb55..f570c6af8d 100644
--- a/tests/functional/ucp_groups_test.php
+++ b/tests/functional/ucp_groups_test.php
@@ -12,50 +12,33 @@
 */
 class phpbb_functional_ucp_groups_test extends phpbb_functional_test_case
 {
-	public function test_groups_manage()
+	public function groups_manage_test_data()
+	{
+		return array(
+			array('#AA0000', 'GROUP_UPDATED'),
+			array('AA0000', 'GROUP_UPDATED'),
+			array('AA0000v', 'COLOUR_INVALID'),
+			array('vAA0000', 'COLOUR_INVALID'),
+			array('AAG000', 'COLOUR_INVALID'),
+			array('#a00', 'GROUP_UPDATED'),
+			array('ag0', 'COLOUR_INVALID'),
+			array('#ag0', 'COLOUR_INVALID'),
+		);
+	}
+
+	/**
+	* @dataProvider groups_manage_test_data
+	*/
+	public function test_groups_manage($input, $expected)
 	{
-		$values = array();
 		$this->login();
 		$this->add_lang(array('ucp', 'acp/groups'));
 
 		$crawler = $this->request('GET', 'ucp.php?i=groups&mode=manage&action=edit&g=5&sid=' . $this->sid);
 		$this->assert_response_success();
 		$form = $crawler->selectButton($this->lang('SUBMIT'))->form();
-		$form['group_colour']->setValue('#AA0000');
-		$crawler = $this->client->submit($form);
-		$this->assertContains($this->lang('GROUP_UPDATED'), $crawler->text());
-
-		$crawler = $this->request('GET', 'ucp.php?i=groups&mode=manage&action=edit&g=5&sid=' . $this->sid);
-		$this->assert_response_success();
-		$form = $crawler->selectButton($this->lang('SUBMIT'))->form();
-		$values = $form->getValues();
-		$this->assertContains('AA0000', $values['group_colour']);
-		$form['group_colour']->setValue('AA0000');
-		$crawler = $this->client->submit($form);
-		$this->assertContains($this->lang('GROUP_UPDATED'), $crawler->text());
-
-		$crawler = $this->request('GET', 'ucp.php?i=groups&mode=manage&action=edit&g=5&sid=' . $this->sid);
-		$this->assert_response_success();
-		$form = $crawler->selectButton($this->lang('SUBMIT'))->form();
-		$values = $form->getValues();
-		$this->assertContains('AA0000', $values['group_colour']);
-		$form['group_colour']->setValue('AA0000v');
+		$form['group_colour']->setValue($input);
 		$crawler = $this->client->submit($form);
-		$this->assertContains($this->lang('GROUP_UPDATED'), $crawler->text());
-
-		$crawler = $this->request('GET', 'ucp.php?i=groups&mode=manage&action=edit&g=5&sid=' . $this->sid);
-		$this->assert_response_success();
-		$form = $crawler->selectButton($this->lang('SUBMIT'))->form();
-		$values = $form->getValues();
-		$this->assertContains('AA0000', $values['group_colour']);
-		$form['group_colour']->setValue('vAA0000');
-		$crawler = $this->client->submit($form);
-		$this->assertContains($this->lang('GROUP_UPDATED'), $crawler->text());
-
-		$crawler = $this->request('GET', 'ucp.php?i=groups&mode=manage&action=edit&g=5&sid=' . $this->sid);
-		$this->assert_response_success();
-		$form = $crawler->selectButton($this->lang('SUBMIT'))->form();
-		$values = $form->getValues();
-		$this->assertContains('vAA000', $values['group_colour']);
+		$this->assertContains($this->lang($expected), $crawler->text());
 	}
 }