-
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-229 Make Query\Builder return objects instead of array to match Laravel behavior #3107
Conversation
@@ -451,8 +455,7 @@ public function toMql(): array | |||
$options['projection'] = $projection; | |||
} | |||
|
|||
// Fix for legacy support, converts the results to arrays instead of objects. | |||
$options['typeMap'] = ['root' => 'array', 'document' => 'array']; | |||
$options['typeMap'] = ['root' => 'object', 'document' => 'array']; |
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.
Shall we update the type of document to object
also, to get the same type for root and embedded documents?
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.
If the root document is returned as an object, it would make sense to return all embedded documents as objects as well.
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.
Yes, butLaravel have sort of embedded document with "JSON" fields. They are returned as array...
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.
Ah, I see. In that case, let's leave it as-is for consistency with Laravel, even though it seems strange in general.
Is it possible that all v5 pr be merge this week? I just have an expectation that v5 will be release this week. But if not, no pressure just want those big changes to be merge so we can evaluate if we need to add rdbms or just this package can do fine itself. |
Note that we could leverage the |
Tracking in PHPC-2426 |
83a7f8e
to
fa2c542
Compare
…h Laravel's behavior
Fix PHPORM-229
Laravel Query Builder always returns objects (source).
This is a major breaking change that requires changing all array access into property access. Returning a
MongoDB\Model\BSONDocument
andMongoDB\Model\BSONArray
is not possible as the data is casted with native PHP cast feature in Laravel code:(object) $result
and(array) $result
.Checklist