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.

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

Conversation

alcaeus
Copy link
Member

@alcaeus alcaeus commented Jul 18, 2025

PHPORM-369

Checklist

  • Add tests and ensure they pass

This addresses an issue highlighted in a discussion about rename_embedded_id_field: when using DB::table()->insert, we pass $values directly to aliasIdForQuery. Since $values contains a list of documents at this point, every document is treated as an embedded document and misses out on the ID handling for root documents. To work around this, we call aliasIdForQuery on each document through array_map.
I also added a test with a model that has $incrementing set to false, as the wrong behaviour is bypassed when $incrementing is its default true value.

@alcaeus alcaeus requested a review from jmikola July 18, 2025 11:56
@alcaeus alcaeus self-assigned this Jul 18, 2025
@alcaeus alcaeus requested a review from a team as a code owner July 18, 2025 11:56
@jmikola jmikola mentioned this pull request Jul 18, 2025
1 task
'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.

@alcaeus alcaeus changed the title Fix ID handling when using insert method instead of save PHPORM-369: Fix ID handling when using insert method instead of save Jul 21, 2025
@alcaeus alcaeus merged commit 161cb8c into mongodb:5.4 Jul 21, 2025
71 checks passed
@alcaeus alcaeus deleted the fix-insert-id-conversion branch July 21, 2025 06:59
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.

2 participants