[Scheduler] Fix stateful scheduler #51651
Conversation
| $delta = $run->format('U.u') - $from; | ||
| $recurrencesPassed = floor($delta / $this->intervalInSeconds); | ||
| $nextRunTimestamp = sprintf('%.6F', ($recurrencesPassed + 1) * $this->intervalInSeconds + $from); | ||
| $nextRun = \DateTimeImmutable::createFromFormat('U.u', $nextRunTimestamp, $fromDate->getTimezone()); | ||
| $nextRun = \DateTimeImmutable::createFromFormat('U.u', $nextRunTimestamp)->setTimezone($fromDate->getTimezone()); |
There was a problem hiding this comment.
It looks like this change is not needed, is it?
There was a problem hiding this comment.
From my testing, UTC is always forced in the DateTime instance when it is created from unix timestamp (timezone constructor arg has no effect), so to have the same timezone, it must be set after the instance is created.
But you are right in that this fix is not related to the issue and can/should probably be excluded here.
There was a problem hiding this comment.
Can you submit another PR on 6.3 for this change then?
| @@ -133,6 +140,6 @@ private function canBeConvertedToSeconds(\DateInterval $interval): bool | |||
|
|
|||
| private function calcInterval(\DateInterval $interval): float | |||
| { | |||
| return $this->from->setTimestamp(0)->add($interval)->format('U.u'); | |||
| return (new \DateTimeImmutable('@0'))->add($interval)->format('U.u'); | |||
There was a problem hiding this comment.
Why do we need this change?
We might add (float) to make Psalm happy :)
There was a problem hiding this comment.
I originally made this because $this->from was not always available at this point, I'm not sure if that's the case anymore
|
This must be merged on 6.4 (because of the BC break, which is allowed in 6.4 for experimental components, but not for a patch release). |
@fabpot Would you like to take over this, or if you had done work on this already, discard this MR as you probably have better knowledge in this 😅 |
Actually, I like your code better, so if you take into account my few comments, I will be able to merge it fast. |
f761fc9 to
2d5856b
Compare
|
Sorry I think I misunderstood your comment @fabpot and retargeted this to 6.4 😅 The triggers have changed quite much between the 6.3&6.4 branches, but I should still target 6.3 and leave conflict resolution to someone else? Oh my, I think I have notified everyone... sorry guys! I will finish for today not to cause any more trouble. |
|
Thank you @valtzu. |
…valtzu) This PR was merged into the 6.3 branch. Discussion ---------- [Scheduler] Match next run timezone with "from" timezone | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | yes | New feature? | no | Deprecations? | no | License | MIT As discussed in #51651 (comment), when a datetime object is created from unix timestamp, the timezone constructor argument is ignored as demonstrated in https://onlinephp.io/c/b07d1 and also mentioned in [PHP documentation](https://www.php.net/manual/en/datetime.construct.php#refsect1-datetime.construct-parameters): > The $timezone parameter and the current timezone are ignored when the $datetime parameter either is a UNIX timestamp (e.g. `@946684800`) or specifies a timezone (e.g. 2010-01-28T15:00:00+02:00). This change shouldn't break any existing logic, given the places where this time is used already include timezone in the date format string. ### Changes in effect ```diff ------------- ------------------------------------ --------------------------- Message Trigger Next Run ------------- ------------------------------------ --------------------------- - TestMessage PeriodicalTrigger: every 2 seconds 2023-09-16T15:54:46+00:00 + TestMessage PeriodicalTrigger: every 2 seconds 2023-09-16T18:54:46+03:00 ------------- ------------------------------------ --------------------------- ``` Commits ------- 9baf427 Match next run timezone with from timezone
Stateful scheduler seems rather broken at the moment, see #51384 (comment).
Let's fix it by storing the original first run start time, that way it's always possible to recalculate the state.
Catching-up works now:
Whereas before it would only start on from the current item, possibly skipping previous items like stated in #51646. (is this a bc break?)
I will be waiting for input from authors of the related issues.
One test is failing because because
getNextRunDateis called with2020-02-20T01:59:00+02:00and then next run date is expected at2020-02-20T02:09:00+02:00but we get2020-02-20T02:00:00+02:00because that's set asfrom. I don't quite get the logic, I would assume that it is expected to be run immediately onfrom🤔