Skip to content

PHPORM-369: Fix ID handling when using insert method instead of save #3429

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jul 21, 2025
+68 −1
Merged

PHPORM-369: Fix ID handling when using insert method instead of save #3429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7740c10
Fix ID handling when using insert method instead of save
alcaeus Jul 18, 2025
e54aeb2
Add test with non-incrementing model
alcaeus Jul 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/Query/Builder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -745,7 +745,10 @@ public function insert(array $values)
$values = [$values];
}

$values = $this->aliasIdForQuery($values);
$values = array_map(
$this->aliasIdForQuery(...),
$values,
);

$options = $this->inheritConnectionOptions();

Expand Down
22 changes: 22 additions & 0 deletions tests/ModelTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
use MongoDB\Laravel\Tests\Models\IdIsString;
use MongoDB\Laravel\Tests\Models\Item;
use MongoDB\Laravel\Tests\Models\MemberStatus;
use MongoDB\Laravel\Tests\Models\NonIncrementing;
use MongoDB\Laravel\Tests\Models\Soft;
use MongoDB\Laravel\Tests\Models\SqlUser;
use MongoDB\Laravel\Tests\Models\User;
Expand Down Expand Up @@ -56,6 +57,7 @@ public function tearDown(): void
Book::truncate();
Item::truncate();
Guarded::truncate();
NonIncrementing::truncate();

parent::tearDown();
}
Expand Down Expand Up @@ -106,6 +108,26 @@ public function testInsert(): void
$this->assertEquals(35, $user->age);
}

public function testInsertNonIncrementable(): void
{
$connection = DB::connection('mongodb');
$connection->setRenameEmbeddedIdField(false);

$nonIncrementing = new NonIncrementing();
$nonIncrementing->id = '12345';
$nonIncrementing->name = 'John Doe';

$nonIncrementing->save();

$this->assertTrue($nonIncrementing->exists);
$this->assertEquals(1, NonIncrementing::count());

$check = NonIncrementing::find($nonIncrementing->id);
$this->assertInstanceOf(NonIncrementing::class, $check);
$this->assertSame('12345', $check->id);
$this->assertEquals('John Doe', $check->name);
}

public function testUpdate(): void
{
$user = new User();
Expand Down
26 changes: 26 additions & 0 deletions tests/Models/NonIncrementing.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?php

declare(strict_types=1);

namespace MongoDB\Laravel\Tests\Models;

use Illuminate\Database\Eloquent\Model;
use MongoDB\Laravel\Eloquent\DocumentModel;

/**
* @property string $id
* @property string $name
*/
class NonIncrementing extends Model
{
use DocumentModel;

protected $keyType = 'string';
protected $connection = 'mongodb';

protected $fillable = [
'name',
];
protected static $unguarded = true;
public $incrementing = false;
Copy link
Member

Choose a reason for hiding this comment

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

I noted in #3428 (comment) you mentioned that $incrementing = true (Eloquent's default per Primary Keys) was responsible for a different code path being utilized.

Assuming this library is going to generate ObjectIds regardless, I would think it'd be ideal if $incrementing had absolutely no effect on our behavior. Is this something we have no control over (within reason)?

Copy link
Member

Choose a reason for hiding this comment

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

Noted that this goes back to Illuminate\Database\Eloquent\Model::performInsert().

When $incrementing is true, Model::insertAndSetId() relies on Builder::insertGetId() (where $values appears to be a single document). Otherwise, it delegates to Builder::insert(), which has the logic to wrap the singular document in an array before applying aliasIdForQuery().

So the bug fix looks correct. I think there's still an edge case in the "document or documents?" detection within Builder::insert(), but that' outside the scope of this PR. This assumption could be problematic if all fields in a document were arrays (e.g. _id is an embedded document):

As soon as we find a value that is not an array we assume the user is inserting a single document.

}
16 changes: 16 additions & 0 deletions tests/QueryBuilderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,22 @@ public function testInsert()
$this->assertIsArray($user->tags);
}

#[TestWith([true])]
#[TestWith([false])]
public function testInsertWithCustomId(bool $renameEmbeddedIdField)
{
$connection = DB::connection('mongodb');
$connection->setRenameEmbeddedIdField($renameEmbeddedIdField);

$data = ['id' => 'abcdef', 'name' => 'John Doe'];

DB::table('users')->insert($data);

$user = User::find('abcdef');
$this->assertInstanceOf(User::class, $user);
$this->assertSame('abcdef', $user->id);
}

public function testInsertGetId()
{
$id = DB::table('users')->insertGetId(['name' => 'John Doe']);
Expand Down
Loading