-
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
DOCSP-42957: DateTimeInterface in queries #3140
DOCSP-42957: DateTimeInterface in queries #3140
Conversation
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.
The same applies in the other direction. UTCDateTime objects received as results are converted to Carbon
date with the default timezone. #3119
I think this should be mentioned in the query builder docs, with an example.
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 left a suggestion that might change the scope of the PR, so requesting another look!
…laravel-mongodb into DOCSP-42957-datetimeinterface
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.
Adding a 2nd example would make more sense that changing the existing one.
->where('released', new UTCDateTime( | ||
Carbon::create(2010, 1, 15, 0, 0, 0, 'UTC') | ||
))->get(); |
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.
The previous example was a lot better as it used whereDate
method that convert the string to a date object. I don't think it makes sense to change it.
The purpose is to show that users can work with Carbon
object without worring with UTCDateTime
. This example shows the opposite of what should be done.
->where('released', Carbon::create(2010, 1, 15));
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 added back the old example and kept this one - lmk what you think
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.
Thanks, that's perfect.
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.
your changes really cleared things up! left some small suggestions
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
JIRA - https://jira.mongodb.org/browse/DOCSP-42957
Staging - https://preview-mongodbnorareidy.gatsbyjs.io/laravel/DOCSP-42957-stage/upgrade/#version-5.x-breaking-changes
Checklist