-
Notifications
You must be signed in to change notification settings - Fork 1.4k
PHPORM-147 Make id an alias for _id
#3040
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
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. |
alcaeus
left a comment
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
_idfield - The value of
_idis returned when fetching theidfield - Using
idin 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
rustagir
left a comment
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
idas 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)Usermodel laravel/breeze#357Breaking change: it is no longer possible to set a different value for
idand_id. If you need a distinct ID field, use an other name.Checklist