Skip to content
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

Changed from using modify to add/sub methods on time objects #179

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

dragonmantank
Copy link
Owner

@dragonmantank dragonmantank commented Apr 16, 2024

This swaps out using (almost everywhere) modify() with discreet add() and sub() methods on the DateTime objects.

While it was nice to be able to "write out" the changes to be made, it seems that it is very unsafe. By swapping to DateInterval objects and using add()\sub() calls, this let the engine better handle transitions.

This also drops support for PHP 8.0 and older versions. Date/Time math changed its behavior, and I don't think there is a way to properly handle the transitions in an easily maintainable manner. It is easier to just cut off support.

Breaking or Bug?
One thing this does change as I went through the tests is gets rid of the "run when transitioned" behavior. What this means is that prior to this fix, if a cron should have executed in a dead zone (where due to a transition we don't have a value time), it would run the next hour. So with a cron of 15 2 * * *, if 02:15 does not exist, we would allow 03:15 to satisfy.

I believe this is wrong. The time does not exist, so therefore we should not run, and the library should not try and "fix" this condition. The vixie-cron package for operating systems can handle this because it knows the last time it ran, so if all of a sudden it skips an hour, it can be reasonably sure a timezone transition happened. Anything over 3 hours of a shift it considers a time correction, so has a limited window to determine if it should run missed events.

This library does not have that luxury, is it is intended to run in the default PHP mode of "run, execute, die". Starting to track run times I think starts to fall out of the purview of this library. The problem is we did handle this for a bit, so is this considered now a feature or a bug?

@SeynovAM
Copy link

SeynovAM commented Apr 18, 2024

This PR does not fix the test that was added in this PR.
If simplify this test it will look like this:

$cron = new CronExpression('15 0 * * *');
$tz = 'America/Los_Angeles';

$dtCurrent = $this->createDateTimeExactly('2024-03-10 00:15-08:00', new \DateTimeZone($tz));
$dtExpectedNext = $this->createDateTimeExactly('2024-03-11 00:15-07:00', new \DateTimeZone($tz));
$dtActualNext = $cron->getNextRunDate($dtCurrent, 0, false, $tz);

$this->assertEquals($dtExpectedNext, $dtActualNext);

Time transition in LA occured 10th of March, at 2:00 it increases to 3:00. So time 00:15 exists. And I expect that this lib will jump from 00:15 before time transition to 00:15 of the next day after transition. But that did not happen, it jumps to 01:15 of the same day:

1) Cron\Tests\DaylightSavingsTest::testLosAngelesDST
Failed asserting that two DateTime objects are equal.
--- Expected
+++ Actual
@@ @@
-2024-03-11T00:15:00.000000-0700
+2024-03-10T01:15:00.000000-0800

@SeynovAM
Copy link

SeynovAM commented Apr 18, 2024

By the way the same example in Europe/London timezone does not work in both - neither in my PR nor in this:

$cron = new CronExpression('15 0 * * *');
$tz = 'Europe/London';

$dtCurrent = $this->createDateTimeExactly('2024-03-31 00:15+00:00', new \DateTimeZone($tz));
$dtExpectedNext = $this->createDateTimeExactly('2024-04-01 00:15+00:00', new \DateTimeZone($tz));
$dtActualNext = $cron->getNextRunDate($dtCurrent, 0, false, $tz);

$this->assertEquals($dtExpectedNext, $dtActualNext);

Output:

Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'2024-04-01 00:15+00:00'
+'2024-04-01 01:15+01:00'


$actualDay = (int)$date->format('d');
$actualHour = (int)$date->format('H');
if (($actualDay !== ($originalDay + 1)) && ($actualHour !== 0)) {
$offsetChange = ($previousOffset - $date->getOffset());
$date = $this->timezoneSafeModify($date, "+{$offsetChange} seconds");
$date = $date->add(new \DateInterval("PT{$distance}S"));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like something wrong. There was $offsetChange before PR, now there is $distance


$actualDay = (int)$date->format('d');
$actualHour = (int)$date->format('H');
if (($actualDay !== ($originalDay - 1)) && ($actualHour !== 23)) {
$offsetChange = ($previousOffset - $date->getOffset());
$date = $this->timezoneSafeModify($date, "+{$offsetChange} seconds");
$date = $date->add(new \DateInterval("PT{$offsetChange}H"));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we be sure that $offsetChange is always positive? Because if it will be negative an error will occur here

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