PHPORM-234 Convert dates in DB Query results #3119
Conversation
GromNaN
force-pushed
the
PHPORM-234
branch
3 times, most recently
from
6e7ed8b to
053a893
Compare
| ['name' => 'John Doe', 'birthday' => Date::parse('1980-01-01 00:00:00')], | ||
| ['name' => 'Robert Roe', 'birthday' => Date::parse('1982-01-01 00:00:00')], | ||
| ['name' => 'Mark Moe', 'birthday' => Date::parse('1983-01-01 00:00:00.1')], | ||
| ['name' => 'Frank White', 'birthday' => Date::parse('1975-01-01 12:12:12.1')], |
There was a problem hiding this comment.
Changed the date to avoid the issue reported in PHPC-2429
GromNaN
force-pushed
the
PHPORM-234
branch
3 times, most recently
from
8c21036 to
4425180
Compare
GromNaN
requested review from
mana2-bot and
jmikola
and removed request for
mana2-bot
GromNaN
force-pushed
the
PHPORM-234
branch
3 times, most recently
from
b668fb4 to
ca2082f
Compare
| ]); | ||
|
|
||
| $user = DB::table('users') | ||
| ->where('birthday', new UTCDateTime(Date::parse('1980-01-01 00:00:00'))) | ||
| ->where('birthday', Date::parse('1980-01-01 00:00:00')) |
There was a problem hiding this comment.
Noted that Date::parse() returns a Carbon instance
src/Query/Builder.php
Outdated
| if ($value instanceof UTCDateTime) { | ||
| $values[$key] = Date::instance($value->toDateTime()) | ||
| ->setTimezone(new DateTimeZone(date_default_timezone_get())); | ||
| } elseif (is_array($value) || $value instanceof stdClass) { |
There was a problem hiding this comment.
This changes is_object() to a stdClass check. Was that intentional?
I'm not familiar with how aliasIdForResult() is used, but this looks like it may be an optimization to avoid unnecessary recursion. aliasIdForResult() only handled array and stdClass instances and returned $values as-is otherwise.
There was a problem hiding this comment.
Yes, but this method may accept any class defined in the typeMap. Not something supported for now as the typeMap cannot be modified. Tracked in PHPORM-233
| if (is_array($value) || is_object($value)) { | ||
| if ($value instanceof UTCDateTime) { | ||
| $values[$key] = Date::instance($value->toDateTime()) | ||
| ->setTimezone(new DateTimeZone(date_default_timezone_get())); |
There was a problem hiding this comment.
I'll note that ODM also applies the default timezone in its DateType class.
There was a problem hiding this comment.
Good to know, thanks for the investigation.
src/Eloquent/DocumentModel.php
Outdated
| /** @inheritdoc */ | ||
| public function fromDateTime($value) | ||
| /** | ||
| * Convert a DateTime to a storable UTCDateTime. |
There was a problem hiding this comment.
Should this refer to DateTimeInterface? You can also write "DateTimeInterface (including Carbon)" for more clarity.
Related thoughts (no action needed) follow.
Although UTCDateTime accepts a DateTimeInterface, it doesn't actually rely on any of its methods. That interface is effectively used in place of a union on DateTime and DateTimeImmutable. The UTCDateTime constructor accesses the internal php_date_obj struct directly.
Thankfully, this is safe because DateTimeInterface is not able to be implemented by userland classes. So any instance will be either a DateTime or DateTimeImmutable object with the internal struct.
CHANGELOG.md
Outdated
| @@ -5,6 +5,7 @@ All notable changes to this project will be documented in this file. | |||
|
|
|||
| * **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** Make Query\Builder return objects instead of array to match Laravel behavior by @GromNaN in [#3107](https://github.com/mongodb/laravel-mongodb/pull/3107) | |||
| * **BREAKING CHANGE** Convert BSON `UTCDateTime` objects into `Carbon` date in query results by @GromNaN in [#3119](https://github.com/mongodb/laravel-mongodb/pull/3119) | |||
There was a problem hiding this comment.
Would it be worth noting that the converted object gets the default timezone applied?
If Carbon objects were previously converted to UTCDateTime and left as-is, I suppose users would already be accustomed to having any original time zone dropped for UTC. But seeing that Carbon types are now returned might lead them to believe that the original Carbon object's timezone is restored, which isn't necessarily the case.
Doing so would require preserving the original timezone alongside the UTCDateTime in some kind of nested object (see POC in PHPC-640). I'm not advocating for such an implementation -- just pointing out that it was a consideration during the early designs of PHPC's BSON APIs.
There was a problem hiding this comment.
Storing the timezone is something we could consider with custom date cast (need a mutator)
Fix PHPORM-234
Related to #3105 (comment)
Blocked by PHPC-2429Checklist