-
-
Notifications
You must be signed in to change notification settings - Fork 20
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
Timezone fix #76
Timezone fix #76
Conversation
@@ -70,7 +65,7 @@ final public function getFormattedMemory(): string | |||
|
|||
final protected function markAsRun(int $memory): void | |||
{ | |||
$this->duration = \time() - $this->startTime; | |||
$this->duration = \time() - $this->startTime->getTimestamp(); |
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.
@kbond Wouldn't it be better to use new \DateTimeImmutable('now')
instead of \time()
here too, like in the constructor?
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.
My understanding is, when you call ->getTimestamp()
, it returns the "utc timestamp" which is what time()
provides. Is that not the case?
This timezone stuff makes my head hurt...
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'll try to explain better what I mean.
You changed $this->startTime = \time();
to $this->startTime = new \DateTimeImmutable('now');
in the constructor, because of the issue you mentioned in the PR description.
Before, you were comparing with $this->duration = \time() - $this->startTime;
here (comparing time()
with another time()
).
Now it is $this->duration = \time() - $this->startTime->getTimestamp();
(mixing \time()
and new \DateTimeImmutable('now')
).
What I would suggest is $this->duration = (new \DateTimeImmutable('now'))->getTimestamp() - $this->startTime->getTimestamp();
for consistency, so everything is based on new \DateTimeImmutable('now')
instead of time()
.
I guess if both return an UTC timestamp it shouldn't make a difference... But that issue you had, had to come from somewhere 🙂
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 can change for consistency but I believe the result would be the same(?)
I think the crux of the issue was getStartTime(). This took the start time timestamp and always converted to utc.
I had some timezone weirdness on PHP 8.1. My default system/php timezone is EST, I had not timezone configured in my tasks/schedule (it should use the default). All my daily/weekly/monthly tasks were running 5 hours earlier than they should. I was able to fix the problem by explicitly setting the timezone in my schedule but I shouldn't have to do this.
I couldn't reproduce the exact issue but I believe it was caused by the calculation of the start time. It used
new \DateTime('@'.\time())
which sets the timezone to UTC. I guess the bundle was converting that to EST +5 hours.This change uses
new \DateTime('now')
to calculate the start time which creates the object in the default system timezone.