mtdowling#137 Add check for multiple question marks (mtdowling#148)

* mtdowling#137 Add check for multiple question marks * mtdowling#137 Validate position of question marks in expression
loneregister · Aug 10, 2023 · 8e78e44 · 8e78e44
1 parent 782ca59
commit 8e78e44
Show file tree

Hide file tree

Showing 2 changed files with 18 additions and 2 deletions.
diff --git a/src/Cron/CronExpression.php b/src/Cron/CronExpression.php
@@ -201,13 +201,22 @@ public function setExpression(string $value): CronExpression
         $split = preg_split('/\s/', $value, -1, PREG_SPLIT_NO_EMPTY);
         Assert::isArray($split);
 
-        $this->cronParts = $split;
-        if (\count($this->cronParts) < 5) {
+        $notEnoughParts = \count($split) < 5;
+
+        $questionMarkInInvalidPart = array_key_exists(0, $split) && $split[0] === '?'
+            || array_key_exists(1, $split) && $split[1] === '?'
+            || array_key_exists(3, $split) && $split[3] === '?';
+
+        $tooManyQuestionMarks = array_key_exists(2, $split) && $split[2] === '?'
+            && array_key_exists(4, $split) && $split[4] === '?';
+
+        if ($notEnoughParts || $questionMarkInInvalidPart || $tooManyQuestionMarks) {
             throw new InvalidArgumentException(
                 $value . ' is not a valid CRON expression'
             );
         }
 
+        $this->cronParts = $split;
         foreach ($this->cronParts as $position => $part) {
             $this->setPart($position, $part);
         }

diff --git a/tests/Cron/CronExpressionTest.php b/tests/Cron/CronExpressionTest.php
@@ -563,6 +563,13 @@ public function testValidationWorks(): void
         // Issue #125, this is just all sorts of wrong
         $this->assertFalse(CronExpression::isValidExpression('990 14 * * mon-fri0345345'));
 
+        // Issue #137, multiple question marks are not allowed
+        $this->assertFalse(CronExpression::isValidExpression('0 8 ? * ?'));
+        // Question marks are only allowed in dom and dow part
+        $this->assertFalse(CronExpression::isValidExpression('? * * * *'));
+        $this->assertFalse(CronExpression::isValidExpression('* ? * * *'));
+        $this->assertFalse(CronExpression::isValidExpression('* * * ? *'));
+
         // see https://github.com/dragonmantank/cron-expression/issues/5
         $this->assertTrue(CronExpression::isValidExpression('2,17,35,47 5-7,11-13 * * *'));
     }