-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
PHPORM-143 Ensure date are read using local timezone #2739
Conversation
public function testDateUseLocalTimeZone(): void | ||
{ | ||
$tz = 'Australia/Sydney'; | ||
date_default_timezone_set($tz); |
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.
This test permanently alters the state of the environment. Would it be preferable to capture the current timezone first and then restore it at the end of the test?
If you're concerned about a failure interrupting things, you can use tearDown()
, but I don't think that's necessary.
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 timezone is reset before each test by this: https://github.com/orchestral/testbench-core/blob/bf3de053bc6566210c0c336635d03e97e270f205/src/Concerns/CreatesApplication.php#L389-L391
But I can explicitly reset the timezone to prevent any regression.
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 didn't realize the tests in this project used OrchestraTestCase instead of PHPUnit directly. I think a comment explaining that would be sufficient.
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 pending the timezone things in the tests.
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 with a comment explaining the timezone being changed isn't a problem.
Fix PHPORM-143
Fix #2719
In #2705 (a5cf5cb), the conversion from
MongoDB\BSON\UTCDateTime
toCarbon\Date
was modified in a way that no longer change use the local timezone.UTCDateTime::asDateTime
always return aDateTime
instance withUTC
timezone, while le previousDate::createFromTimestampMs($timestampMs)
was using the local timezone.Checklist
Update documentation for new features