PHPORM-236 Remove _id from query results#3136
Conversation
|
|
||
| $this->assertInstanceOf(Movie::class, $movie); | ||
| $this->expectOutputRegex('/^{"_id":"[a-z0-9]{24}","title":"The Shawshank Redemption","directors":\["Frank Darabont","Rob Reiner"\],"id":"[a-z0-9]{24}"}$/'); | ||
| $this->expectOutputRegex('/^{"title":"The Shawshank Redemption","directors":\["Frank Darabont","Rob Reiner"\],"id":"[a-z0-9]{24}"}$/'); |
There was a problem hiding this comment.
We can't preserve the key/property order without creating a new object/array. This overhead and complexity is not justified here. "id" will always be last.
There was a problem hiding this comment.
I realize this was in the original pattern, so it's not a change from this PR, but what causes the ObjectId to be printed as a plain string? It doesn't look like this library overrides Model::toJson(), so is the ObjectId typing dropped entirely for the model's in-memory representation?
There was a problem hiding this comment.
By default, id is casted into string when Model::toArray() is called.
jmikola
left a comment
jmikola
left a comment
There was a problem hiding this comment.
A few questions, but I don't think any changes are required.
|
|
||
| * Remove support for Laravel 10 by @GromNaN in [#3123](https://github.com/mongodb/laravel-mongodb/pull/3123) | ||
| * **BREAKING CHANGE** Use `id` as an alias for `_id` in commands and queries for compatibility with Eloquent packages by @GromNaN in [#3040](https://github.com/mongodb/laravel-mongodb/pull/3040) | ||
| * **BREAKING CHANGE** Use `id` as an alias for `_id` in commands and queries for compatibility with Eloquent packages by @GromNaN in [#3040](https://github.com/mongodb/laravel-mongodb/pull/3040) and [#3136](https://github.com/mongodb/laravel-mongodb/pull/3136) |
There was a problem hiding this comment.
I added an association between PHPORM-147 and PHPORM-236 since these appear to be closely related.
|
|
||
| $this->assertInstanceOf(Movie::class, $movie); | ||
| $this->expectOutputRegex('/^{"_id":"[a-z0-9]{24}","title":"The Shawshank Redemption","directors":\["Frank Darabont","Rob Reiner"\],"id":"[a-z0-9]{24}"}$/'); | ||
| $this->expectOutputRegex('/^{"title":"The Shawshank Redemption","directors":\["Frank Darabont","Rob Reiner"\],"id":"[a-z0-9]{24}"}$/'); |
There was a problem hiding this comment.
I realize this was in the original pattern, so it's not a change from this PR, but what causes the ObjectId to be printed as a plain string? It doesn't look like this library overrides Model::toJson(), so is the ObjectId typing dropped entirely for the model's in-memory representation?
| $item = DB::table('items')->where('id', 'fork')->first(); | ||
| $this->assertEquals('fork', $item->id); | ||
|
|
||
| // tags.id is translated into tags._id in query |
There was a problem hiding this comment.
There are plenty of assertions in this test for _id not existing on the model object(s), but what actually asserts that _id is stored in the database? Presumably, the query below would work even if id was never translated to _id on the way to the server.
If it's not the intention to test that here, feel free to resolve. This comment made me think otherwise.
There was a problem hiding this comment.
I added a query on _id field in this test.
| $this->assertCount(5, $all); | ||
| $this->assertEquals(new ObjectId(sprintf('%024d', 5)), $all[0]->_id); | ||
| $this->assertEquals(new ObjectId(sprintf('%024d', 5)), $all[0]->id); | ||
| $this->assertEquals(sprintf('%024d', 5), $all[0]->id, 'id field is added for compatibility with DatabaseFailedJobProvider'); |
There was a problem hiding this comment.
Unrelated to the diff, but would this benefit from using assertSame to also check that a string type is used?
There was a problem hiding this comment.
They are not the same. 2 distinct objects line 80.
I add assertions on the type of the "id" property.
|
This change results in omitting |
|
See #3184 for the feature request to request the |
4 participants
Fix PHPORM-236
related to #3040
Checklist