-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
PHPORM-147 Make id
an alias for _id
#3040
Conversation
e7015c0
to
7535a88
Compare
Should this target a major release? |
Seems like a minor breaking change, since id is already correlated with _id. Hopefully this get merged soon. This is a must update that can also unblock more laravel packages together with DocumentModel trait. |
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'll admit, I'm not entirely convinced we want to break BC in a minor release, but given the state of the whole _id
vs. id
situation, it might be better to just get this over with.
Correct me if I'm wrong, but as I understand the current situation is:
- Models always have an
_id
field - The value of
_id
is returned when fetching theid
field - Using
id
in the query builder is already changed to use_id
With this change, we're using the value of id
for _id
when saving models, but throw if there are different values for id
and _id
. This ensures that other libraries are able to query MongoDB models the same way they query relational models. Did I understand all of this correctly?
laravel-mongodb/src/Eloquent/DocumentModel.php Lines 75 to 81 in f654b83
|
dbee100
to
6efb2a4
Compare
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.
lgtm
I was on the fence about unsetting laravel-mongodb/src/Query/Builder.php Line 1662 in 7551f76
It makes it impossible to work with Thanks for your feedback. It seems that we should unset |
I dont know if this an issue or anything that can be improved for now I made adjustment on my side by unsetting the _id field Address model $address = Address::find(objectIdhere);
// unset($address ['_id']); Solution
$users = User::insert([
'name' => $user->fullname,
'username' => $user->username,
'address' => $address->toArray()]) Cannot have both "id" and "_id" fields. {"exception":"[object] (InvalidArgumentException(code: 0): Cannot have both "id" and "_id" fields. at /app/vendor/mongodb/laravel-mongodb/src/Query/Builder.php:1620) |
We can also detect when |
Thanks for replying so fast, I edited my question now its below to the answer hahaha. So the aliasIdForResult will just return the id field, but we cant access the native _id Objectid. This would be in line to Laravel behavior but probably needs to be documented. This is acceptable though just like prisma uses id for mongodb too. |
When using v5 I always assume that _id is non existent so I always use id both for saving data and retrieving it. |
Fix PHPORM-147
Fix #3022 (PHPORM-201)
Fix #2489
Laravel and Eloquent uses
id
as conventional field name for models and various queries. MongoDB's primary key is called_id
. The name is source of various issues with Laravel packages:DatabaseSessionHandler
[Feature Request] SessionServiceProvider for database #3022 (comment)User
model laravel/breeze#357Breaking change: it is no longer possible to set a different value for
id
and_id
. If you need a distinct ID field, use an other name.Checklist