Problem/Motivation

With current implementations uuids with uppercase letters are not valid. There should not be difference between uppercase and lowercase letters in uuids?

Proposed resolution

Fix regex pattern in Uuid to consider uppercase letters.

Remaining tasks

User interface changes

API changes

Data model changes

Comments

mbovan created an issue. See original summary.

cilefen’s picture

As far as I know, uppercase characters are not allowed in UUIDs, so an uppercased string should be invalid.

djevans’s picture

According to RFC4122:

The hexadecimal values "a" through "f" are output as lower case characters and are case insensitive on input.

So the current regex pattern in Uuid::isValid() is correct, if we're reading an existing entity's UUID.
Would it be better to:

a) add something to the long description of the function, to recommend wrapping the argument in strtolower() if it contains digits A-F, or
b) add a default parameter to the method signature - something like this:

public static function isValid($uuid, $allow_uppercase = FALSE) {
  $pattern = '/^' . self::VALID_PATTERN . '$/';
  if ($allow_uppercase) {
    $pattern .= 'i';
  }
  return (bool) preg_match($pattern, $uuid);
}

I'd suggest a) as being simpler.

mbovan’s picture

Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new529 bytes

Extended method documentation with a comment about handling uppercase letters in a UUID.

cilefen’s picture

StatusFileSize
new536 bytes

Re #3:

The hexadecimal values "a" through "f" are output as lower case characters and are case insensitive on input.

I see now. Wouldn't this be better?

cilefen’s picture

Oh, I see #3 vs #5, the issue is input vs output.

dawehner’s picture

Issue tags: +Needs tests

Yeah better adapt the regex we use. Do you pointing to the right location in the RFC from within the documentation?

cilefen’s picture

To anyone interested, you need to edit Drupal\Tests\Component\Uuid\UuidTest::providerTestValidation and add some uppercased UUIDs.

cilefen’s picture

Issue tags: +Novice

The test modification is suitable for a Novice.

djevans’s picture

Assigned: Unassigned » djevans
djevans’s picture

StatusFileSize
new1.56 KB

Attached a patch to update the regex (with documentation), and test a UUID with uppercase digits.

cilefen’s picture

@djevans That's looking pretty good.

The last submitted patch, 4: uuid_isvalid_is-2576331-4.patch, failed testing.

The last submitted patch, 5: uuid_isvalid_is-2576331-5.patch, failed testing.

cilefen’s picture

I am wondering what the effect of this patch could be on storing UUIDs. Could we now store an upper-cased UUID and maybe output it the same way, which (I guess) by the RFC is not ok?

mbovan’s picture

Is Uuid::isValid() considered when we generate a UUID? I guess not.

Btw, for some reason, I have all UUIDs in my database saved in uppercase format.

djevans’s picture

@mbovan No, UuidInterface::generate() doesn't make any validation checks.

You can also pass arbitrary UUIDs into Entity::create(), so something like this will work (although maybe it shouldn't):

use \Drupal\node\Entity\Node;

$values = ['type' => 'article', 'title' => 'Foo', 'uuid' => '123'];
$node = Node::create($values);
$node->save();
berdir’s picture

We could define a valid uuid constraint on UuidItem but that seems like a separate task.

We've seen uppercase UUID's when importing data from external systems and yes, for some weird reasons, @mbovan's Drupal generates uppercase UUID's ;)

It's not exactly obvious that you have to convert them to lowercase or somewhere down the line it suddenly won't work. But I guess that's how it is, I agree that we shouldn't allow something that is clearly against the standard.

berdir’s picture

From that quite in #3:

The hexadecimal values "a" through "f" are output as lower case characters and are case insensitive on input.

However, see my explanation in #2613926: The PECL UUID implementation can return invalid UUIDs, it's better to enforce it, so we should possibly close this as duplicate of that issue?

cilefen’s picture

Status: Needs review » Closed (duplicate)

I agree.