Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

PHPORM-236 Remove _id from query results #3136

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
GromNaN merged 3 commits into mongodb:5.0 from GromNaN:PHPORM-236
Sep 5, 2024
Merged

Conversation

@GromNaN
Copy link
Member

@GromNaN GromNaN commented Sep 5, 2024

Fix PHPORM-236
related to #3040

Checklist

  • Add tests and ensure they pass
  • Add an entry to the CHANGELOG.md file

@GromNaN GromNaN added this to the 5.0 milestone Sep 5, 2024
@GromNaN GromNaN marked this pull request as ready for review September 5, 2024 10:35
@GromNaN GromNaN requested a review from a team as a code owner September 5, 2024 10:35

$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}"}$/');
Copy link
Member Author

@GromNaN GromNaN Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@jmikola jmikola Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@GromNaN GromNaN Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default, id is casted into string when Model::toArray() is called.

Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Member

@jmikola jmikola Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added an association between PHPORM-147 and PHPORM-236 since these appear to be closely related.

GromNaN reacted with thumbs up emoji

$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}"}$/');
Copy link
Member

@jmikola jmikola Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

@jmikola jmikola Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@GromNaN GromNaN Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a query on _id field in this test.

jmikola reacted with thumbs up emoji
$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');
Copy link
Member

@jmikola jmikola Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to the diff, but would this benefit from using assertSame to also check that a string type is used?

Copy link
Member Author

@GromNaN GromNaN Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are not the same. 2 distinct objects line 80.
I add assertions on the type of the "id" property.

@GromNaN GromNaN enabled auto-merge (squash) September 5, 2024 17:54
@GromNaN GromNaN merged commit 837078f into mongodb:5.0 Sep 5, 2024
Copy link

This change results in omitting _id from toArray() and toJson(). Thus, introducing a high-impact breaking change.

@GromNaN GromNaN deleted the PHPORM-236 branch March 5, 2025 17:00
Copy link
Member Author

GromNaN commented Mar 5, 2025

See #3184 for the feature request to request the _id field.
mshamaseen reacted with thumbs up emoji

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jmikola jmikola jmikola approved these changes

+1 more reviewer

@rustagir rustagir rustagir approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Projects

None yet

Milestone

5.0

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /