Skip to content

[9.x] Improve UUID and ULID support for Eloquent - #44146

Merged
taylorotwell merged 6 commits into
9.xfrom
improve-uuid-support
Sep 16, 2022
Merged

[9.x] Improve UUID and ULID support for Eloquent#44146
taylorotwell merged 6 commits into
9.xfrom
improve-uuid-support

Conversation

@driesvints

@driesvints driesvints commented Sep 15, 2022

Copy link
Copy Markdown
Member

Improves upon #44074 by:

  • Allowing to more easily use the trait to just be used on non-primary key columns
  • Adds a special ulid column on Blueprint that creates a char(26) behind the scenes
  • Automatically sets incrementing to false when a string type primary key is used
  • Adds more tests

I was doubting on checking to see if I could add support to use both traits at the same time (so you could both fill in UUID and ULID columns) but I guess that scenario isn't so common.

if ($this->getKeyType() === 'string') {
return false;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This might need to be rethought. In my mind a string key type column can never be incrementing but I might be wrong.

@driesvints
driesvints marked this pull request as ready for review September 15, 2022 15:34
$query->shouldReceive('insertGetId')->once()->with([], 'id')->andReturn('string id');
$query->shouldReceive('insert')->once()->with(['id' => 'string id']);
$query->shouldReceive('getConnection')->once();
$model->expects($this->once())->method('newModelQuery')->willReturn($query);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I had to rewrite the test here because of the change I did in the getIncrementing method. I think the current test wasn't realistic as I can't see how you can increment with string-based keys. With string-based keys you always need to set them beforehand afaik (like I adjusted the test to).

Comment thread src/Illuminate/Database/Eloquent/Concerns/HasUuids.php
@driesvints driesvints changed the title [9.x] Improvde UUID and ULID support for Eloquent [9.x] Improve UUID and ULID support for Eloquent Sep 15, 2022
@taylorotwell

taylorotwell commented Sep 15, 2022

Copy link
Copy Markdown
Member

@driesvints why was the change to getKeyType not needed on HasUlids too?

@driesvints

driesvints commented Sep 16, 2022

Copy link
Copy Markdown
Member Author

@taylorotwell whoops, you're right. Pushed.

I first was thinking of doing it through a property but when you apply a trait with a property directly on a model you cannot overwrite it. The current solution is way better and more frictionless :)

@taylorotwell
taylorotwell merged commit 9235af3 into 9.x Sep 16, 2022
@taylorotwell
taylorotwell deleted the improve-uuid-support branch September 16, 2022 15:20
@martio

martio commented Sep 22, 2022

Copy link
Copy Markdown
Contributor

Good job! 👍

@martio

martio commented Sep 22, 2022

Copy link
Copy Markdown
Contributor

Maybe it's nice to add support for a binary column type for UUID? 😏

@The-Hasanov

The-Hasanov commented Sep 22, 2022

Copy link
Copy Markdown

Hi @driesvints . This update break jenssegers/laravel-mongodb
mongodb/laravel-mongodb#2451

public function getIncrementing()
{
    if ($this->getKeyType() === 'string') {
         return false;
       }

      return $this->incrementing;
 }

@driesvints

Copy link
Copy Markdown
Member Author

@The-Hasanov I replied on the related PR fix: mongodb/laravel-mongodb#2452

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.

6 participants