Valid cron expression is not accepted

[hollodotme]

Input

$expression = '2,17,35,47 5-7,11-13 * * *';

Expected behaviour

• CronExpression::isValidExpression($expression) returns true
• CronExpression::factory($expression) returns a CronExpression instance.

Referring to crontab.guru this is a valid expression.

Actual behaviour

• CronExpression::isValidExpression($expression) returns false
• CronExpression::factory($expression) throws an InvalidArgumentException with message Invalid CRON field value 5-7,11-13 at position 1

Identified cause

Range and list at the same time in the same field are not accepted.

See https://github.com/dragonmantank/cron-expression/blob/v2.0.0/src/Cron/AbstractField.php#L210

---

I would like to provide a PR, if you confirm this is a valid issue.

I once implemented a cron expression validator myself, which allows this expression. I updated it today to be compatible with the PHPUnit 6+ version.

[hollodotme]

I forked this repo and added 2 more tests with all the valid/invalid expressions from my crontab-validator tests.

See: https://github.com/hollodotme/cron-expression/blob/upstream/issue-5/tests/Cron/CronExpressionTest.php#L462 ff.

Seems there are a couple of expressions that do not pass validation:

php vendor/bin/phpunit                                                                                                                                                                                                                              1 ↵  1420  17:36:35
PHPUnit 6.5.2 by Sebastian Bergmann and contributors.

...............................................................  63 / 530 ( 11%)
................................F..............F............... 126 / 530 ( 23%)
..........F.........................F..........F............... 189 / 530 ( 35%)
......................................F........................ 252 / 530 ( 47%)
......................................................F........ 315 / 530 ( 59%)
.......................................F.......F............... 378 / 530 ( 71%)
............F....................F...........F.F..F..F........F 441 / 530 ( 83%)
..............F.F..........................F...F.F.FFFFFFFFF... 504 / 530 ( 95%)
..........................                                      530 / 530 (100%)

Time: 427 ms, Memory: 6.00MB

[adjenks]

#4 Seems to be a byproduct of this.

[dragonmantank]

Thanks for this On mobile, but I don't believe you can have ranges and lists as part of a single cron element. This was a break from 1.x, as 2.x tries to stick to a stock syntax provided by default systems like cronie, as 1.x was _very_ generous in what it accepted. There are also multiple cron syntaxes, which doesn't help. I'll double check this when I get back to a PC.

[dragonmantank]

You are correct on the check. We explicitly disallowed ranges and lists in the same expression.

And here's where I misread part of the cronie source code:

https://github.com/cronie-crond/cronie/blob/master/src/entry.c#L431

get_list does indeed call get_range, so we should allow lists of ranges.

Marking this as a bug. If you have a PR that would be great, but if not looks like if we need to remove that constraint, and make sure that each split of the list expands ranges out.

[hollodotme]

Thanks for your quick replies. I also checked the underlying standard of crontab (IEEE Std1003.2-1992) again at http://linuxreviews.org/man/crontab/. From the EXTENSIONS section:

Lists and ranges are allowed to co-exist in the same field. "1-3,7-9" would be rejected by ATT or BSD cron -- they want to see "1-3" or "7,8,9" ONLY.

Ranges can include "steps", so "1-9/2" is the same as "1,3,5,7,9".

So yes, it indeed depends on the system.

The best way to solve this would be to provide the possibility of multiple validation strategies which is a bigger effort of course.

What I can do:

1. Start a PR to allow lists and ranges to close this issue.
2. Start a branch to allow multiple validation strategies for next major release and provide it as a PR, if you agree.

[matzeeable]

Hi, I am interested in this cron parser and the possibility to use ranges and lists together. @hollodotme I see you already created a fork for this. Did you get this to run? Have you merged it to your master so it can be installed through composer? :-)

[hollodotme]

@matzeeable I am still working on the patch for this repo. But because I needed a "quick" solution which the patch will be based on, I created https://github.com/hollodotme/crontab-expression with the following features:

• validate cron expressions (using https://github.com/hollodotme/crontab-validator)
• check if an expression is due

In my repo the expression is not mutable at the moment, because I didn't need it. That's the main reason I did not finish the patch yet.

[dragonmantank]

@hollodotme Can you check the latest master? I've made a change that should allow for lists and ranges in the same expression. Thanks!

[dragonmantank]

Closing this issue for now. We added support in master for lists and ranges in the same expression.