Currently the UUID pattern is hardcoded in Uuid::isValid() method. The downside of this is that any contrib module that wants to use the UUID pattern in regex has to copy and paste the string. I think developers would find it more useful if the pattern was moved to a class constant and so then it could be reused.

An example of a use case for this is a UUID url resolver, as currently exists in the d7 UUID contrib module.

CommentFileSizeAuthor
#9 interdiff-1847932-1-9.txt1.73 KBskwashd
#9 uuid_pattern_constant-1847932-9.patch858 bytesskwashd
#1 drupal-1847932-uuid-pattern-class-const.patch1.13 KBskwashd

Comments

skwashd’s picture

skwashd commented
StatusFileSize
new drupal-1847932-uuid-pattern-class-const.patch1.13 KB
skwashd’s picture

skwashd commented
Status: Active » Needs review

Updating status.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Component/Uuid/Uuid.php
@@ -52,7 +57,7 @@ public function generate() {
-    return preg_match("/^[0-9a-f]{8}-([0-9a-f]{4}-){3}[0-9a-f]{12}$/", $uuid);
+    return preg_match('/^' . self::VALID_PATTERN . '$/', $uuid);

This should use late static binding per default, so use "static" instead of "self".

Why are the start and end characters "^" and "$" not part of the pattern? They are part of the regex, right?

Also: the "/" delimiter should be escaped in case the pattern contains slashes as well.

skwashd’s picture

skwashd commented
Status: Needs work » Needs review

This should use late static binding per default, so use "static" instead of "self".

I thought about this, but given UUIDs are defined in RFC 4122 the value shouldn't be overridden by sub classes.

Why are the start and end characters "^" and "$" not part of the pattern? They are part of the regex, right?

Because then the pattern can only be used for matching a complete UUID and nothing else. The whole idea of this is to allow contrib to use the pattern for matching UUIDs in larger strings. If I include the '^' and '$', then you can't do something like this:
if (preg_match('#^uuid/node/(' . Uuid::VALID_PATTERN . ')$#', $url, $m)) { /* do stuff */ }.

Also: the "/" delimiter should be escaped in case the pattern contains slashes as well.

Escaping the "/" will break the regex. If the pattern did contain slashes, which is shouldn't as per the RFC, then the slashes in the pattern should be escaped not the delimiter.

Personally I prefer using "#" over "/" as a delimiter for web stuff these days as it is less likely to require escaping in patterns. The existing code uses slashes as the delimiters, so I'm leaving them as they are.

Setting back to needs review. Hopefully the test bot can clone properly this time.

skwashd’s picture

skwashd commented

#1: drupal-1847932-uuid-pattern-class-const.patch queued for re-testing.

skwashd’s picture

skwashd commented

The tests passed, but the status hasn't updated here. http://qa.drupal.org/pifr/test/390413

alansaviolobo queued 1: drupal-1847932-uuid-pattern-class-const.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: drupal-1847932-uuid-pattern-class-const.patch, failed testing.

skwashd’s picture

skwashd commented
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new uuid_pattern_constant-1847932-9.patch858 bytes
new interdiff-1847932-1-9.txt1.73 KB

Rerolled patch and attached interdiff.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Reviewed & tested by the community

Ok, the constant seems to be a better idea anyway. Looks good!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 393fccc and pushed to 8.0.x. Thanks!

  • alexpott committed 393fccc on 8.0.x 
    Issue #1847932 by skwashd: Added Make the UUID pattern a constant.

Status: Fixed » Closed (fixed)

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