-
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?
7535a88 to
01ad545
Compare
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.
@GromNaN 🫡🙏🏻thanks for being thoughtful of the community
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?
Using
idin the query builder is already changed to use_id
_id was already used by the Query\Builder::find method. But a where('id', <value>) was not converted to { "_id": <value> }. With Eloquent models, there is already an alias when the attribute id is read
laravel-mongodb/src/Eloquent/DocumentModel.php
Lines 75 to 81 in f654b83
2c71461 to
8bbcdbf
Compare
dbee100 to
6efb2a4
Compare
6efb2a4 to
40b453a
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 _id or not. That's why I left this comment:
laravel-mongodb/src/Query/Builder.php
Line 1662 in 7551f76
It makes it impossible to work with _id in result as it would be natively, while _id or id can both be used in a query.
Thanks for your feedback. It seems that we should unset _id indead.
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
Example
Address model
User with address field
$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)
It seems good that it can detect also nested documents that has both id and _id, but is there a way it only returns id or do we just need to keep unsettting the _id field
We can also detect when _id === id in a query and unset(id). That would solve your issue.
I was on the fence about unsetting
_idor not. That's why I left this comment:laravel-mongodb/src/Query/Builder.php
Line 1662 in 7551f76
//unset($values['_id']);It makes it impossible to work with
_idin result as it would be natively, while_idoridcan both be used in a query.Thanks for your feedback. It seems that we should unset
_idindeed.
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.
We can also detect when
_id === idin a query andunset(id). That would solve your issue.
When using v5 I always assume that _id is non existent so I always use id both for saving data and retrieving it.
Uh oh!
There was an error while loading. Please reload this page.
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