DST handling fixes (fixes #111, #112) (#115)

* DST fix (attempt 1) * More tests and further fixes WIP as I believe the change to HoursField::isSatisfiedBy is going to break expressions with multiple parts - but one thing at a time! * WIP More tests and further fixes; API change: isSatisfiedBy now requires knowledge of which direction we're travelling in to handle checking initial values correctly * WIP Save Point - Trying to fix one thing breaks another, but I have an idea... * WIP Abstracted NextRunDateTime which keeps track of offset changes, regardless of whether they occurred when changing minute or hour (and to persist them until a next change is made) * WIP Fix easy fixes * WIP More test fixes * All current tests pass! * Fix for issue #112; Use a cache of timezone transitions to avoid having to modify date/time objects every single time we want to check if hour is satisfied All tests pass * Cleanup All tests pass * Cleanup All tests pass * Cleanup - backing out the NextRunDateTime abstraction; All tests pass * Cleanup; All tests pass * Cleanup (diff tidy, restoring deleted tests); All tests pass * Cleanup (diff tidy); All tests pass * Fix CI issues * Fix CI issues Co-authored-by: Chris Tankersley <chris@ctankersley.com>
dragonmantank · Jan 5, 2022 · 5d6c9a7 · 5d6c9a7
1 parent 7a364e7
commit 5d6c9a7
Show file tree

Hide file tree

Showing 15 changed files with 724 additions and 82 deletions.
diff --git a/src/Cron/AbstractField.php b/src/Cron/AbstractField.php
@@ -4,6 +4,8 @@
 
 namespace Cron;
 
+use DateTimeInterface;
+
 /**
  * Abstract CRON expression field.
  */
@@ -300,4 +302,28 @@ public function validate(string $value): bool
 
         return \in_array($value, $this->fullRange, true);
     }
+
+    protected function timezoneSafeModify(DateTimeInterface $dt, string $modification): DateTimeInterface
+    {
+        $timezone = $dt->getTimezone();
+        $dt = $dt->setTimezone(new \DateTimeZone("UTC"));
+        $dt = $dt->modify($modification);
+        $dt = $dt->setTimezone($timezone);
+        return $dt;
+    }
+
+    protected function setTimeHour(DateTimeInterface $date, bool $invert, int $originalTimestamp): DateTimeInterface
+    {
+        $date = $date->setTime((int)$date->format('H'), ($invert ? 59 : 0));
+
+        // setTime caused the offset to change, moving time in the wrong direction
+        $actualTimestamp = $date->format('U');
+        if ((! $invert) && ($actualTimestamp <= $originalTimestamp)) {
+            $date = $this->timezoneSafeModify($date, "+1 hour");
+        } elseif ($invert && ($actualTimestamp >= $originalTimestamp)) {
+            $date = $this->timezoneSafeModify($date, "-1 hour");
+        }
+
+        return $date;
+    }
 }
diff --git a/src/Cron/CronExpression.php b/src/Cron/CronExpression.php
@@ -240,14 +240,32 @@ public function getPreviousRunDate($currentTime = 'now', int $nth = 0, bool $all
      */
     public function getMultipleRunDates(int $total, $currentTime = 'now', bool $invert = false, bool $allowCurrentDate = false, $timeZone = null): array
     {
+        $timeZone = $this->determineTimeZone($currentTime, $timeZone);
+
+        if ('now' === $currentTime) {
+            $currentTime = new DateTime();
+        } elseif ($currentTime instanceof DateTime) {
+            $currentTime = clone $currentTime;
+        } elseif ($currentTime instanceof DateTimeImmutable) {
+            $currentTime = DateTime::createFromFormat('U', $currentTime->format('U'));
+        } elseif (\is_string($currentTime)) {
+            $currentTime = new DateTime($currentTime);
+        }
+
+        Assert::isInstanceOf($currentTime, DateTime::class);
+        $currentTime->setTimezone(new DateTimeZone($timeZone));
+
         $matches = [];
         for ($i = 0; $i < $total; ++$i) {
             try {
-                $currentTime = $this->getRunDate($currentTime, 0, $invert, $i === 0 ? $allowCurrentDate : false, $timeZone);
-                $matches[] = $currentTime;
+                $result = $this->getRunDate($currentTime, 0, $invert, $allowCurrentDate, $timeZone);
             } catch (RuntimeException $e) {
                 break;
             }
+
+            $allowCurrentDate = false;
+            $currentTime = clone $result;
+            $matches[] = $result;
         }
 
         return $matches;
@@ -364,7 +382,9 @@ protected function getRunDate($currentTime = null, int $nth = 0, bool $invert =
 
         Assert::isInstanceOf($currentDate, DateTime::class);
         $currentDate->setTimezone(new DateTimeZone($timeZone));
-        $currentDate->setTime((int) $currentDate->format('H'), (int) $currentDate->format('i'), 0);
+        // Workaround for setTime causing an offset change: https://bugs.php.net/bug.php?id=81074
+        $currentDate = DateTime::createFromFormat("!Y-m-d H:iO", $currentDate->format("Y-m-d H:iP"), $currentDate->getTimezone());
+        $currentDate->setTimezone(new DateTimeZone($timeZone));
 
         $nextRun = clone $currentDate;
 
@@ -380,7 +400,7 @@ protected function getRunDate($currentTime = null, int $nth = 0, bool $invert =
             $fields[$position] = $this->fieldFactory->getField($position);
         }
 
-        if (isset($parts[2]) && isset($parts[4])) {
+        if (isset($parts[self::DAY]) && isset($parts[self::WEEKDAY])) {
             $domExpression = sprintf('%s %s %s %s *', $this->getExpression(0), $this->getExpression(1), $this->getExpression(2), $this->getExpression(3));
             $dowExpression = sprintf('%s %s * %s %s', $this->getExpression(0), $this->getExpression(1), $this->getExpression(3), $this->getExpression(4));
 
@@ -409,10 +429,10 @@ protected function getRunDate($currentTime = null, int $nth = 0, bool $invert =
                 $field = $fields[$position];
                 // Check if this is singular or a list
                 if (false === strpos($part, ',')) {
-                    $satisfied = $field->isSatisfiedBy($nextRun, $part);
+                    $satisfied = $field->isSatisfiedBy($nextRun, $part, $invert);
                 } else {
                     foreach (array_map('trim', explode(',', $part)) as $listPart) {
-                        if ($field->isSatisfiedBy($nextRun, $listPart)) {
+                        if ($field->isSatisfiedBy($nextRun, $listPart, $invert)) {
                             $satisfied = true;
 
                             break;
@@ -430,8 +450,7 @@ protected function getRunDate($currentTime = null, int $nth = 0, bool $invert =
 
             // Skip this match if needed
             if ((!$allowCurrentDate && $nextRun == $currentDate) || --$nth > -1) {
-                $this->fieldFactory->getField(0)->increment($nextRun, $invert, $parts[0] ?? null);
-
+                $this->fieldFactory->getField(self::MINUTE)->increment($nextRun, $invert, $parts[self::MINUTE] ?? null);
                 continue;
             }
 
@@ -458,7 +477,7 @@ protected function determineTimeZone($currentTime, ?string $timeZone): string
         }
 
         if ($currentTime instanceof DateTimeInterface) {
-            return $currentTime->getTimeZone()->getName();
+            return $currentTime->getTimezone()->getName();
         }
 
         return date_default_timezone_get();

diff --git a/src/Cron/DayOfMonthField.php b/src/Cron/DayOfMonthField.php
@@ -79,7 +79,7 @@ private static function getNearestWeekday(int $currentYear, int $currentMonth, i
     /**
      * {@inheritdoc}
      */
-    public function isSatisfiedBy(DateTimeInterface $date, $value): bool
+    public function isSatisfiedBy(DateTimeInterface $date, $value, bool $invert): bool
     {
         // ? states that the field value is to be skipped
         if ('?' === $value) {
@@ -117,10 +117,12 @@ public function isSatisfiedBy(DateTimeInterface $date, $value): bool
      */
     public function increment(DateTimeInterface &$date, $invert = false, $parts = null): FieldInterface
     {
-        if ($invert) {
-            $date = $date->modify('previous day')->setTime(23, 59);
+        if (! $invert) {
+            $date = $this->timezoneSafeModify($date, '+1 day');
+            $date = $date->setTime(0, 0);
         } else {
-            $date = $date->modify('next day')->setTime(0, 0);
+            $date = $this->timezoneSafeModify($date, '-1 day');
+            $date = $date->setTime(23, 59);
         }
 
         return $this;

diff --git a/src/Cron/DayOfWeekField.php b/src/Cron/DayOfWeekField.php
@@ -4,7 +4,6 @@
 
 namespace Cron;
 
-use DateTime;
 use DateTimeInterface;
 use InvalidArgumentException;
 
@@ -54,10 +53,8 @@ public function __construct()
 
     /**
      * @inheritDoc
-     *
-     * @param \DateTime|\DateTimeImmutable $date
      */
-    public function isSatisfiedBy(DateTimeInterface $date, $value): bool
+    public function isSatisfiedBy(DateTimeInterface $date, $value, bool $invert): bool
     {
         if ('?' === $value) {
             return true;
@@ -76,15 +73,9 @@ public function isSatisfiedBy(DateTimeInterface $date, $value): bool
             $weekday = $this->convertLiterals(substr($value, 0, strpos($value, 'L')));
             $weekday %= 7;
 
-            $tdate = clone $date;
-            $tdate = $tdate->setDate($currentYear, $currentMonth, $lastDayOfMonth);
-            while ($tdate->format('w') != $weekday) {
-                $tdateClone = new DateTime();
-                $tdate = $tdateClone->setTimezone($tdate->getTimezone())
-                    ->setDate($currentYear, $currentMonth, --$lastDayOfMonth);
-            }
-
-            return (int) $date->format('j') === $lastDayOfMonth;
+            $daysInMonth = (int) $date->format('t');
+            $remainingDaysInMonth = $daysInMonth - (int) $date->format('d');
+            return (($weekday === (int) $date->format('w')) && ($remainingDaysInMonth < 7));
         }
 
         // Handle # hash tokens
@@ -156,15 +147,15 @@ public function isSatisfiedBy(DateTimeInterface $date, $value): bool
 
     /**
      * @inheritDoc
-     *
-     * @param \DateTime|\DateTimeImmutable $date
      */
     public function increment(DateTimeInterface &$date, $invert = false, $parts = null): FieldInterface
     {
-        if ($invert) {
-            $date = $date->modify('-1 day')->setTime(23, 59, 0);
+        if (! $invert) {
+            $date = $this->timezoneSafeModify($date, '+1 day');
+            $date = $date->setTime(0, 0);
         } else {
-            $date = $date->modify('+1 day')->setTime(0, 0, 0);
+            $date = $this->timezoneSafeModify($date, '-1 day');
+            $date = $date->setTime(23, 59);
         }
 
         return $this;

diff --git a/src/Cron/FieldInterface.php b/src/Cron/FieldInterface.php
@@ -19,7 +19,7 @@ interface FieldInterface
      *
      * @return bool Returns TRUE if satisfied, FALSE otherwise
      */
-    public function isSatisfiedBy(DateTimeInterface $date, $value): bool;
+    public function isSatisfiedBy(DateTimeInterface $date, $value, bool $invert): bool;
 
     /**
      * When a CRON expression is not satisfied, this method is used to increment