Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[9.x] UUID and ULID support for Eloquent #44074

Merged
merged 10 commits into from
Sep 13, 2022
Merged

[9.x] UUID and ULID support for Eloquent #44074

merged 10 commits into from
Sep 13, 2022

Conversation

driesvints
Copy link
Member

@driesvints driesvints commented Sep 9, 2022

This PR brings UUID & ULID support for Eloquent as primary keys. By simply applying a trait, any Eloquent model will have a UUID or ULID generated as their primary key.

For ULID support, an optional symfony/uid dependency will need to be pulled in.

@driesvints driesvints changed the title UUID and ULID support for Eloquent [9.x] UUID and ULID support for Eloquent Sep 9, 2022
@X-Coder264
Copy link
Contributor

@driesvints Awesome PR - I was just thinking about using ULIDs in my next project.

Yesterday Symfony added UUID v7 (which is similar to ULID) support (symfony/symfony#47525) to symfony/uid. Would it be possible to add an option to choose whether we want UUID v4 or v7 (as currently Str::orderedUuid() always returns a v4 UUID) ?

@SjorsO
Copy link
Contributor

SjorsO commented Sep 10, 2022

HasUlidPrimaryKey and HasUuidPrimaryKey might be better names than HasPrimaryUlid and HasPrimaryUuid.

@driesvints
Copy link
Member Author

Yesterday Symfony added UUID v7 (which is similar to ULID) support (symfony/symfony#47525) to symfony/uid. Would it be possible to add an option to choose whether we want UUID v4 or v7 (as currently Str::orderedUuid() always returns a v4 UUID) ?

I'm not sure. We don't use symfony/uid for UUID's but ramsey/uuid. It does not seem that library has support for v7 yet. It also does not seem Str::orderedUuid() has support for specifying the type of UUID so I don't think this is possible?

@driesvints
Copy link
Member Author

HasUlidPrimaryKey and HasUuidPrimaryKey might be better names than HasPrimaryUlid and HasPrimaryUuid.

Done

@driesvints driesvints marked this pull request as ready for review September 12, 2022 09:22
public static function bootHasUlidPrimaryKey()
{
static::creating(function (self $model) {
if (empty($model->{$model->getKeyName()})) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't be blank($model->getKey()) ?

@taylorotwell taylorotwell merged commit 93ff1bc into 9.x Sep 13, 2022
@taylorotwell taylorotwell deleted the eloquent-uuid branch September 13, 2022 19:29
@DarkGhostHunter
Copy link
Contributor

Just came here to say that ULID will be saved into the database as a 26 character string, which occupies 208 bits, almost double than an UUID (128 bit).

If the column is an UUID, most database engines will return an error.

What it could work is setting the key type to "uuid", so then the primary key is detected as an ULID, it will change to RFC4122.

@Jubeki
Copy link
Contributor

Jubeki commented Sep 15, 2022

Yesterday Symfony added UUID v7 (which is similar to ULID) support (symfony/symfony#47525) to symfony/uid. Would it be possible to add an option to choose whether we want UUID v4 or v7 (as currently Str::orderedUuid() always returns a v4 UUID) ?

I'm not sure. We don't use symfony/uid for UUID's but ramsey/uuid. It does not seem that library has support for v7 yet. It also does not seem Str::orderedUuid() has support for specifying the type of UUID so I don't think this is possible?

It seems like ramsey/uuid now has support for v7: https://github.com/ramsey/uuid/releases/tag/4.5.0

@driesvints
Copy link
Member Author

I'll look into it 👍

@larskoole
Copy link

Maybe creating a ulid() macro for the migrations would be nice too.

Currently I'm adding the following macro to the AppServiceProvicer:

Blueprint::macro('ulid', function (string $column = 'ulid') {
    return $this->addColumn('char', $column, ['length' => 26]);
});

Which enables me to do this in my migrations:

Schema::create('users', function (Blueprint $table) {
    $table->ulid('id')->primary();
    ...
});

@tomb1n0
Copy link

tomb1n0 commented Sep 15, 2022

Awesome! Just this week i'd created a trait that used Str::orderedUuid() exactly like this. Great to have this officially supported inside Laravel

@driesvints
Copy link
Member Author

@larskoole I'll also check into that.

@mariomarquesdev
Copy link

@driesvints Could we have an option to set which column to be the uuid?

For example, we could keep the default behaviour of looking at the primary key but it would be nice to have something like getUuidColumn function or something like that inside each Model to override that default behaviour.

Potential usage:

table: users
id (auto increment to be used for relations) | uuid

@larskoole
Copy link

@driesvints Could we have an option to set which column to be the uuid?

For example, we could keep the default behaviour of looking at the primary key but it would be nice to have something like getUuidColumn function or something like that inside each Model to override that default behaviour.

Potential usage:

table: users id (auto increment to be used for relations) | uuid

Already there, you can override the uniqueIds() method from the trait inside of your model.

https://github.com/laravel/framework/pull/44074/files#diff-0dbc036e590312e780e3fac6936e57aa9628b31679dd979e098cd73fc34d85cfR35-R43

@BrandonSurowiec
Copy link
Contributor

BrandonSurowiec commented Sep 15, 2022

If someone wanted to use this for columns other than the primary id and updates uniqueIds() to not contain the primary key, the getKeyType() and getIncrementing() shouldn't be overriding $this->keyType and $this->incrementing. We'll need to PR a fix for that.

@driesvints
Copy link
Member Author

If someone wanted to use this for columns other than the primary id and updates uniqueIds() to not contain the primary key, the getKeyType() and getIncrementing() shouldn't be overriding $this->keyType and $this->incrementing. We'll need to PR a fix for that.

I'll think about that a bit more as well.

@driesvints
Copy link
Member Author

Worked on some of the above stuff here: #44146. I'll check into UUID v7 support later.

@@ -46,6 +46,7 @@
"league/commonmark": "Required to use Str::markdown() and Stringable::markdown() (^2.0.2).",
"ramsey/uuid": "Required to use Str::uuid() (^4.2.2).",
"symfony/process": "Required to use the composer class (^6.0).",
"symfony/uid": "Required to generate ULIDs for Eloquent (^6.0).",
Copy link
Contributor

@tanthammar tanthammar Sep 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there something wrong with the requirement?
I had to manually composer require symfony/uid to get Str::ulid() to work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is in the suggests session. So to use its functionality you should manually install this dependency.

@driesvints
Copy link
Member Author

I sent in a PR to use uuid v7 in Laravel v10 here: #44210

@martio
Copy link
Contributor

martio commented Sep 29, 2022

@driesvints @taylorotwell I added missing morphs methods for ULID support 👉 PR 44364 😊

@driesvints
Copy link
Member Author

We reverted the changes to make UUID v7 the default in Laravel v10. Please see #44210

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.