dragonmantank / cron-expression

CRON for PHP: Calculate the next or previous run date and determine if a CRON expression is due
MIT License
4.57k stars 124 forks source link

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

Open dragonmantank opened 7 months ago

dragonmantank commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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'