Changing cronvalidator to use more robust regular expressions

[MellenIO]

This resolves #122 - by using a few more robust regular expressions (from this SO answer) we can better test for a wider range of regular expressions - including non-standard macros where available.

[MellenIO]

Edited the original comment to reflect that it will close the related issue.
@Ethan3600 it looks like my PR is failing on unrelated issues (looks like a change in Magento somewhere) so my branch cannot be merged. Is there any plan for this?

[Ethan3600]

Please target the 1.x-develop branch. You may need to rebase 😨

[Ethan3600]

Thanks for contributing this! I'll take a more in depth look this weekend.

[MellenIO]

Are these all supported by Magento's cron parser?

https://docs.oracle.com/cd/E12058_01/doc/doc.1014/e12030/cron_expressions.htm
Looks like they might not all be - these are the list of test cases that Magento bases them off of, I think in fact a regex solution might not work so well (Magento itself will preg_split and then parse each individual block if necessary).
I wonder if, instead of rewriting the validation to match from PHP to JS, we instead create a validation controller (which we then AJAX request) that calls something like:

public function __construct(ScheduleFactory $scheduleFactory) {
    $this->schedule = $scheduleFactory->create();
}

public function execute() {
    $expr = $this->getRequest('expr');
    $this->schedule->setCronExpr($expr);
    try {
        $this->schedule->trySchedule();
        // .. good cron
    }
    catch (CronException $cex) {
        // ... bad cron
    }
}

(just as an example here - code written hastily!)

The cron parser (under \Magento\Cron\Model\Schedule) will throw a CronException if anything goes wrong in the parsing between the two methods. This has benefits of being completely inline with what Magento accepts, but it has the downside of looking a bit sketchy.

Sorry for the issue with being on the wrong branch btw - in my defence, PHPStorm defaulted to 1.x and there are no changes between the 2 ;) depending on what's decided above ^ I'll probably just close this PR and make a new one.

[Ethan3600]

Haha no worries regarding using 1.x-develop vs 1.x.

I think an AJAX call is kind of overkill. IMO, since this validation already exists when the cron is registered in the DB, we only need to do very "light" validation on the client side.

I think we just want to prevent people putting in "asdlfkjasdlfj" for example 😆

So, if a user were to put an incorrect cron expression, they'd get an error when it runs and they'd go back and figure it out from there.

[Ethan3600]

we could probably get away with just this.

[Ethan3600]

Actually, maybe this:

((((\d+,)+\d+|(\*,)\d+|(\d+(\/|-)\d+)|\d+|\*\/?) ?){5,7})

[Ethan3600]

Added support for something like */3 and *,5

[schmengler]

If unsure, we could go for (\W+ ){4,6}\W+ - 5 or seven blocks, separated by space. And let the server handle the details.

[MellenIO]

I'm happy to implement either solution tbh - I think there's generally no harm with trying to validate for more terms, one thing to note is that we should ensure there's adequate server sided validation. Maybe we validate for what @schmengler is suggesting in the frontend, and then do a more stringent process in the controller?

[Ethan3600]

That makes sense. See if you can add some validation inside this save function.

This way we can get the loose client side validation and a better server side validation. From a UX perspective, if something doesn't pass the server side validation. They should be redirected to the edit page and a message should appear saying it's an invalid cron.

What do you guys think?

[MellenIO]

@Ethan3600 I'm happy with that, I'll work on implementing that some point this week if I get the time. Sorry for the delayed response, btw - I had to mute notifications from the magento org and this ended up being one of the casualties! 😂

[Ethan3600]

No need to apologize!! That's the beauty of open source. No due dates or deadlines! We just get to build cool stuff and share it :).

[choussamaster]

Hi any news about this branch is it stable ?

[mehdichaouch]

UP

[MellenIO]

Hey all,
Sorry for not paying attention to this PR, I don't really pay too much attention to Github. I will push up a commit for this tonight with @arnoudhgz 's suggested change.

@Ethan3600 it looks like the PR is currently failing due to the 2.2-develop branch missing from Magento. Is this just because this PR is old?

[Ethan3600]

Hey @MellenIO no worries at all! I've also not been paying too much attention to Github 😞. Regarding the failures, I also think it's because it's an old PR. I bet if you rebased on top of the 1.x branch it'll go away.