Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#17 | 3056454-17.patch | 2.54 KB | Krzysztof Domański |
#17 | interdiff-15-17.txt | 585 bytes | Krzysztof Domański |
#11 | 3056454-11-test-only.patch | 1.7 KB | Krzysztof Domański |
Comments
Comment #2
Krzysztof DomańskiComment #3
johnwebdev CreditAttribution: johnwebdev commentedNice with test coverage :)
Let's use === instead.
We could use the @covers annotation here.
We can omit this @return.
Comment #5
Krzysztof DomańskiComment #7
Krzysztof DomańskiComment #8
Krzysztof DomańskiComment #9
Krzysztof DomańskiWe can do the validation with a regural expression.
Comment #10
John Cook CreditAttribution: John Cook at Creode commentedThis 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.
Adding a test of
[FALSE, 123456]
(as an integer) toproviderTestValidateHex()
would then ensure that this code is covered.Going back to 'needs work' with the 'novice' tag for adding the extra test.
Comment #11
Krzysztof DomańskiComment #13
John Cook CreditAttribution: John Cook at Creode commentedThanks 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.
Comment #15
Krzysztof DomańskiThe
validateHex()
function should return TRUE or FALSE but thepreg_match()
function returns 1, 0 or FALSE.Comment #16
John Cook CreditAttribution: John Cook at Creode commentedGood catch @Krzysztof Domański.
If
assertEquals()
is changed toassertSame()
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.
Comment #17
Krzysztof DomańskiComment #18
John Cook CreditAttribution: John Cook at Creode commentedThe 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.
Comment #19
larowlanadding issue credits
Comment #22
larowlanCommitted de56b25 and pushed to 8.8.x. Thanks!
c/p to 8.7.x