Problem/Motivation

Drupal\Component\Utility\Color::validateHex('###FF0') return TRUE.

/**
 * Validates whether a hexadecimal color value is syntactically correct.
 *
 * @param $hex
 *   The hexadecimal string to validate. May contain a leading '#'. May use
 *   the shorthand notation (e.g., '123' for '112233').
 *
 * @return bool
 *   TRUE if $hex is valid or FALSE if it is not.
 */
public static function validateHex($hex) {
  // Must be a string.
  $valid = is_string($hex);
  // Hash prefix is optional.
  $hex = ltrim($hex, '#');
  // Must be either RGB or RRGGBB.
  $length = mb_strlen($hex);
  $valid = $valid && ($length === 3 || $length === 6);
  // Must be a valid hex value.
  $valid = $valid && ctype_xdigit($hex);
  return $valid;
}

Proposed resolution

Decide if this is the correct behavior.
Correct if it's an error.

Remaining tasks

Add a test coverage.

CommentFileSizeAuthor
#17 3056454-17.patch2.54 KBKrzysztof Domański
#17 interdiff-15-17.txt585 bytesKrzysztof Domański
#15 3056454-15.patch2.54 KBKrzysztof Domański
#15 3056454-11-15.txt497 bytesKrzysztof Domański
#11 3056454-11.patch2.53 KBKrzysztof Domański
#11 3056454-11-test-only.patch1.7 KBKrzysztof Domański
#11 3056454-9-11.txt561 bytesKrzysztof Domański
#9 3056454-9.patch2.48 KBKrzysztof Domański
#9 interdiff-7-9.txt936 bytesKrzysztof Domański
#7 3056454-7.patch2.34 KBKrzysztof Domański
#7 interdiff-5-7.txt508 bytesKrzysztof Domański
#5 3056454-5.patch2.31 KBKrzysztof Domański
#5 interdiff-2-5.txt1.29 KBKrzysztof Domański
#2 3056454-2.patch2.36 KBKrzysztof Domański
#2 3056454-test-only.patch1.66 KBKrzysztof Domański
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Krzysztof Domański created an issue. See original summary.

Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented
Status: Active » Needs review
FileSize
3056454-test-only.patch1.66 KB
3056454-2.patch2.36 KB
johnwebdev’s picture

johnwebdev CreditAttribution: johnwebdev commented

Nice with test coverage :)

  1. +++ b/core/lib/Drupal/Component/Utility/Color.php
@@ -20,8 +20,10 @@ class Color {
+    if (substr($hex, 0, 1) == '#') {

    Let's use === instead. 

  2. +++ b/core/tests/Drupal/Tests/Component/Utility/ColorTest.php
@@ -12,6 +12,61 @@
+   * Tests validate Hex color.

    We could use the @covers annotation here. 

  3. +++ b/core/tests/Drupal/Tests/Component/Utility/ColorTest.php
@@ -12,6 +12,61 @@
+   * @return array
+   *   An array of test data.

    We can omit this @return.

The last submitted patch, 2: 3056454-test-only.patch, failed testing. View results

Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented
FileSize
interdiff-2-5.txt1.29 KB
3056454-5.patch2.31 KB

Status: Needs review » Needs work

The last submitted patch, 5: 3056454-5.patch, failed testing. View results

Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented
FileSize
interdiff-5-7.txt508 bytes
3056454-7.patch2.34 KB
Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented
Status: Needs work » Needs review
Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented
FileSize
interdiff-7-9.txt936 bytes
3056454-9.patch2.48 KB

We can do the validation with a regural expression.

   public static function validateHex($hex) {
-    // Must be a string.
-    $valid = is_string($hex);
-    // Hash prefix is optional.
-    $hex = ltrim($hex, '#');
-    // Must be either RGB or RRGGBB.
-    $length = mb_strlen($hex);
-    $valid = $valid && ($length === 3 || $length === 6);
-    // Must be a valid hex value.
-    $valid = $valid && ctype_xdigit($hex);
-    return $valid;
+    if (!is_string($hex)) {
+      return FALSE;
+    }
+    return preg_match('/^[#]?([0-9a-fA-F]{3}){1,2}$/', $hex);
   }
John Cook’s picture

John Cook CreditAttribution: John Cook at Creode commented
Status: Needs review » Needs work
Issue tags: +Novice

This looks really good @Krzysztof Domański.

Adding test coverage where there wasn't any before is alway good.

The regex expression is simple enough so it does not need to be commented.

There isn't a test for this code path.

+++ b/core/lib/Drupal/Component/Utility/Color.php
@@ -18,16 +18,10 @@ class Color {
+    if (!is_string($hex)) {
+      return FALSE;
+    }

Adding a test of [FALSE, 123456] (as an integer) to providerTestValidateHex() would then ensure that this code is covered.

Going back to 'needs work' with the 'novice' tag for adding the extra test.

Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented
Status: Needs work » Needs review
FileSize
3056454-9-11.txt561 bytes
3056454-11-test-only.patch1.7 KB
3056454-11.patch2.53 KB

The last submitted patch, 11: 3056454-11-test-only.patch, failed testing. View results

John Cook’s picture

John Cook CreditAttribution: John Cook at Creode commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice

Thanks for adding the extra test @Krzysztof Domański.

Everything looks good so I'm removing the tags that are no longer required and setting to RTBC.

The last submitted patch, 11: 3056454-11-test-only.patch, failed testing. View results

Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented
FileSize
3056454-11-15.txt497 bytes
3056454-15.patch2.54 KB

The validateHex() function should return TRUE or FALSE but the preg_match() function returns 1, 0 or FALSE.

 * @return bool
 *   TRUE if $hex is valid or FALSE if it is not.
 */
public static function validateHex($hex) {

 * @return int|false <b>preg_match</b> returns 1 if the <i>pattern</i>
 * matches given <i>subject</i>, 0 if it does not, or <b>FALSE</b>
 * if an error occurred.
 * @since 4.0
 * @since 5.0
 */
function preg_match ($pattern, $subject, array &$matches = null, $flags = 0, $offset = 0) {}
John Cook’s picture

John Cook CreditAttribution: John Cook at Creode commented
Status: Reviewed & tested by the community » Needs work

Good catch @Krzysztof Domański.

+++ b/core/tests/Drupal/Tests/Component/Utility/ColorTest.php
@@ -13,6 +13,60 @@
+    $this->assertEquals($expected, Color::validateHex($value));

If assertEquals() is changed to assertSame() we can make sure that the returned value is of correct type as well.

Going back to needs work to fix this.

Otherwise it'll be ready for RTBC.

Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented
Status: Needs work » Reviewed & tested by the community
FileSize
interdiff-15-17.txt585 bytes
3056454-17.patch2.54 KB
John Cook’s picture

John Cook CreditAttribution: John Cook at Creode commented

The patch in #17 fixes all the points mentioned here.

The fix for the bug has resulted in shorter code as an understandable regular expression. And good tests covering a range of possible situations has been added to ensure the correct results in the future.

I'm happy for this issue to be RTBC.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

adding issue credits

  • larowlan committed de56b25 on 8.8.x 
    Issue #3056454 by Krzysztof Domański, John Cook, johndevman: Hexadecimal...

  • larowlan committed 7d1eff9 on 8.7.x 
    Issue #3056454 by Krzysztof Domański, John Cook, johndevman: Hexadecimal...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Fixed

Committed de56b25 and pushed to 8.8.x. Thanks!

c/p to 8.7.x

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.