[13.x] Refactor UUID handling to use Symfony's Uid component#59661
[13.x] Refactor UUID handling to use Symfony's Uid component#59661lukaskleinschmidt wants to merge 4 commits intolaravel:13.xfrom
Conversation
|
| } | ||
|
|
||
| return $fields->getVersion() === $version; | ||
| return $version === (int) $uuid->toString()[14]; |
There was a problem hiding this comment.
That is an implementation detail—can't we abstract that away?
There was a problem hiding this comment.
We do get back the specific class implementations like UuidV1 or UuidV7.
So doing something like this would also work:
return match ($version) {
'nil', 0 => $uuid instanceof NilUuid,
'max' => $uuid instanceof MaxUuid,
1 => $uuid instanceof Uuid1,
//...
7 => $uuid instanceof Uuid7,
default => false,
}But will fail for v2 uuids.
There was a problem hiding this comment.
I would prefer that if not for v2—maybe, we would need a wrapper for this? 🤔
There was a problem hiding this comment.
Do you have something specific in mind.
I can't think of a different way to check for a v2 version right now, other than checking the string directly like above.
|
Yeah also running into v2 issues in some tests. Not sure if a "custom V2" implementation would be a viable solution. |
If we leave that out or handle this significantly different, this would be a breaking change |
|
After some digging I do unterstand why I totally see this being a breaking change to remove it altogether. https://laravel.com/docs/13.x/validation#rule-uuid Therefore, I am currently unsure whether it is worthwhile trying to find a solution for this PR. |
|
One idea. /**
* Get the version of a given UUID.
*/
public static function uuidVersion(string $uuid): int|string
{
$uuid = Uuid::fromString($uuid);
return match (true) {
// Not strictly necessary, but maybe preferred
// $uuid instanceof UuidV1 => 1,
// $uuid instanceof UuidV3 => 3,
// $uuid instanceof UuidV4 => 4,
// $uuid instanceof UuidV5 => 5,
// $uuid instanceof UuidV6 => 6,
// $uuid instanceof UuidV7 => 7,
// $uuid instanceof UuidV8 => 8,
$uuid instanceof NilUuid => 'nil',
$uuid instanceof MaxUuid => 'max',
$uuid instanceof Uuid => (int) $uuid->toRfc4122()[14],
};
}
public static function isUuid($value, $version = null)
{
//...
return $version == static::uuidVersion($uuid);
}
/**
* Create a Carbon instance from a given ordered UUID or ULID.
*/
public static function createFromId(Uuid|Ulid|string $id): static
{
if (is_string($id)) {
$id = Ulid::isValid($id) ? Ulid::fromString($id) : Uuid::fromString($id);
}
if ($id instanceof TimeBasedUidInterface) {
return static::createFromInterface($id->getDateTime());
}
return match (Str::uuidVersion($id)) {
2 => static::createFromInterface(BinaryUtil::hexToDateTime(
'0'.substr($id->toString(), 15, 3).substr($id->toString(), 9, 4).'00000000'
)),
default => throw new InvalidArgumentException('The given ID must be a time-based UUID or a ULID.'),
};
} |
|
Thanks for your pull request to Laravel! Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include. If applicable, please consider releasing your code as a package so that the community can still take advantage of your contributions! |
This PR refactors
Uuidsto also be generated and validated using thesymfony/uidpackage, which is already used forUlids.This completely removes the currently used
ramesy/uuidpackage from the framework.The main reason behind this PR:
Originally, I only wanted to use
brick/money@0.13.0in a current project.However, the required/allowed
brick/mathversions forbrick/moneyandramesy/uuidare incompatible.There is an open issue and several open pull requests for this issue in the
ramsey/uuidrepository:ramsey/uuid#632
ramsey/uuid#633
ramsey/uuid#634
I also found a (rather old) discussion on this topic:
#47631
Digging a bit deeper in the laravel framework it it looks like the
ramesy/uuidpackage could be easily replaced by the already usedsymfony/uidpackage. The impact of this change should be minimal, as developers actively working with theramesy/uuidpackage can simply include it manually.