Skip to content

fix: skipped run after daylight saving turnover point (#154) #203

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

CipherdevNL
Copy link

@CipherdevNL CipherdevNL commented Apr 23, 2025

Fixed the skipping runs that are on the daylight saving days as reported in #154 and #202. These examples are also included in a unit test.

As of my understanding, this issue occurs only when a fixed hour is set after the turnover point of 02:00. The issue lies in the timezoneSafeModify function, which converts the datetime into UTC and back towards the original timezone. When going back, PHP is so "kind" of applying the daylight saving time, resulting in one additional hour than expected.

This eventually comes back into the getRunDate, which decides that the time doesn't satisfy the expected hour, as it got moved over by 1 hour. This makes it skip the daylight saving day completely.

As a solution I've implemented an compensation check that will subtract one hour when DST has started, this to make sure the "target" value is actual being met.

@CipherdevNL CipherdevNL force-pushed the master_154 branch 3 times, most recently from 513f284 to a0cce9a Compare April 23, 2025 14:30
@CipherdevNL CipherdevNL marked this pull request as draft April 23, 2025 14:50
@CipherdevNL CipherdevNL marked this pull request as ready for review April 24, 2025 10:04
@thewunder
Copy link

thewunder commented May 2, 2025

I checked out your branch, but

$schedule = new CronExpression('0 10 * * *');
$runs = $schedule->getMultipleRunDates(3, '2025-03-08 09:00:00', timeZone: 'America/Chicago');

Is still skipping the 9th...

EDIT: Woops I didn't have your fix branch checked out... IT WORKS @dragonmantank merge and release please!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants