-
Notifications
You must be signed in to change notification settings - Fork 11.2k
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
[10.x] Fix createOrFirst
on transactions
#48144
Changes from 6 commits
cfad4d7
ce2db36
9f45005
dbc41c3
d4dd95b
a7e57c8
fabfff8
6854a55
c9c2c6a
06dc97e
73a4382
38f2bb2
1745169
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -580,7 +580,9 @@ public function firstOrCreate(array $attributes = [], array $values = []) | |
public function createOrFirst(array $attributes = [], array $values = []) | ||
{ | ||
try { | ||
return $this->create(array_merge($attributes, $values)); | ||
return $this->withSavepointIfNeeded(function () use ($attributes, $values) { | ||
return $this->create(array_merge($attributes, $values)); | ||
}); | ||
} catch (UniqueConstraintViolationException $exception) { | ||
return $this->where($attributes)->first(); | ||
} | ||
|
@@ -2029,4 +2031,19 @@ public function __clone() | |
{ | ||
$this->query = clone $this->query; | ||
} | ||
|
||
/** | ||
* Wraps the given Closure with a transaction savepoint if needed. | ||
* | ||
* @template TModelValue | ||
* | ||
* @param \Closure(): TModelValue $scope | ||
* @return TModelValue | ||
*/ | ||
public function withSavepointIfNeeded(Closure $scope): mixed | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure about adding this public method. I thought about just wrapping the insert part in a Let me know what y'all think. Open to suggestions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For MySQL/SQLServer/SQLite, this is not necessary; Postgres just needs it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I figured it wouldn't be a problem to wrap in savepoint transactions for the other drivers as well. It doesn't seem to hurt. |
||
{ | ||
return $this->getQuery()->getConnection()->transactionLevel() > 0 | ||
? $this->getQuery()->getConnection()->transaction($scope) | ||
: $scope(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
<?php | ||
|
||
namespace Illuminate\Tests\Integration\Database; | ||
|
||
use Illuminate\Support\Facades\DB; | ||
|
||
trait WithEloquentIntegrationTests | ||
{ | ||
public function testCreateOrFirst() | ||
{ | ||
$user1 = $this->eloquentModelClass::createOrFirst(['email' => 'taylorotwell@gmail.com']); | ||
|
||
$this->assertSame('taylorotwell@gmail.com', $user1->email); | ||
$this->assertNull($user1->name); | ||
|
||
$user2 = $this->eloquentModelClass::createOrFirst( | ||
['email' => 'taylorotwell@gmail.com'], | ||
['name' => 'Taylor Otwell'] | ||
); | ||
|
||
$this->assertEquals($user1->id, $user2->id); | ||
$this->assertSame('taylorotwell@gmail.com', $user2->email); | ||
$this->assertNull($user2->name); | ||
|
||
$user3 = $this->eloquentModelClass::createOrFirst( | ||
['email' => 'abigailotwell@gmail.com'], | ||
['name' => 'Abigail Otwell'] | ||
); | ||
|
||
$this->assertNotEquals($user3->id, $user1->id); | ||
$this->assertSame('abigailotwell@gmail.com', $user3->email); | ||
$this->assertSame('Abigail Otwell', $user3->name); | ||
|
||
$user4 = $this->eloquentModelClass::createOrFirst( | ||
['name' => 'Dries Vints'], | ||
['name' => 'Nuno Maduro', 'email' => 'nuno@laravel.com'] | ||
); | ||
|
||
$this->assertSame('Nuno Maduro', $user4->name); | ||
} | ||
|
||
public function testCreateOrFirstWithinTransaction() | ||
{ | ||
$user1 = $this->eloquentModelClass::createOrFirst(['email' => 'taylor@laravel.com']); | ||
|
||
DB::transaction(function () use ($user1) { | ||
$user2 = $this->eloquentModelClass::createOrFirst( | ||
['email' => 'taylor@laravel.com'], | ||
['name' => 'Taylor Otwell'] | ||
); | ||
|
||
$this->assertEquals($user1->id, $user2->id); | ||
$this->assertSame('taylor@laravel.com', $user2->email); | ||
$this->assertNull($user2->name); | ||
}); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really liked how it read before:
createOrFirst
was essentially callingcreate()
thenfirst()
, now it has more noise. I thought about pushing the savepoint to thecreate()
method, which would keep this part as it was, but then it would be a bigger change.