Review due time calculation for new cron events

Please have a look into this MR, it provides the back-box and a data provider, where we can easily add all additional use cases to make sure that we're doing the right thing.

I've manually restored your commit in this branch, hope the copy-paste was right.

Thanks for fixing this, I guess this happened because I had accidentally committed to the dev branch instead of the MR and then reverted that commit after I had also committed to the MR. That must have screwed up things.

I'd suggest that we test for the applies method of CronEvent. Have added the test data from the already existing Kernel test into that Unit test.

This is good. I assume this is in addition to the unit test I had already prepared.

There it can be seen that the current approach fails for "* * * * *", which should always run on first time.

This is most likely because of return $currentTime > $this->getNextRunTimestamp($lastRun, $currentTime, $frequency); - the comparison there should be >=.

However, my proposal for handling initial due calculation has another weakness: it only works for regular schedules like every week, every day or every hour, etc. But for infrequent schedules it would work that well, e.g. if a schedule is defined for Monday, Tuesday and Friday. That may result in a wrong calculation for the initial execution of that event. Everything else would be just fine as it is.

Thinking about it again, the only reliable solution would be if we knew the date and time when the ECA config entity containing that event were created. We could use that timestamp as the lastRunTimestamp and the existing calculation would come back with the always correct timestamp for the next due time.

As config entities don't have create and update properties, we would have to store the timestamp to the ECA state when the ECA entity gets saved and if no such key exists yet in the state environment. To achieve that, eca_base may have to hook into the entity save event and do its magic. Thoughts?

This is most likely because of return $currentTime > $this->getNextRunTimestamp($lastRun, $currentTime, $frequency); - the comparison there should be >=.

This and 2 more adjustments were required to now have all tests passing:

• When getting the current timestamp in isDue we need to cut off seconds so that we have a timestamp for a full minute. Otherwise the current time is something like 10:20:05 and then the next due timestamp would be 10:21:00
• When falling back to current time on getNextRunTimestamp, we have to go back 1 second as well. That would give us a fallback timestamp of 10:19:59 which will then give us a due timestamp of 10:20:00

Either a hook, or ECA gives plugin configurations a chance to do something on entity-level savings, defined by an interface.

Implemented an interface EcaAddPluginInterface which is called for all plugins when saving an ECA entity. This then also covers the scenario when importing a model or when enabling a module which comes with an ECA config entity.

All existing tests pass, so this looks solid now - at least I hope so.

Regarding the 2 failed tests, when debugging isDue, here is what we get:

$current = '2022-06-18 15:00';
$last = '2022-06-18 14:00';
$next = '2022-06-19 00:00';

So, the calculation of the next due time is exactly what we expect. What's not correct yet is the current time and that seems do be an issue with the mocked state service. I'll have a look.

Confirmed for the test "Every minute today.". When adding timezone to the mock like this

    $now = new \DateTime('now', new \DateTimeZone('UTC'));
    $today_0000am = new DateTime($now->format('Y-m-d 00:00:01'), new \DateTimeZone('UTC'));
    $today_0100am = new DateTime($now->format('Y-m-d 01:00:01'), new \DateTimeZone('UTC'));

then that test behaves as expected. But now 3 others are failing. Still looking.

['30 0 * * *', FALSE, 'Every day at 00:30 AM.'], needs to be ['30 0 * * *', TRUE, 'Every day at 00:30 AM.'], as last run was at 0:00 and this one should execute at 0:30.

['1 0 * * ' . $weekday_today, FALSE, 'Today at 00:01 AM.'], needs to be ['1 0 * * ' . $weekday_today, TRUE, 'Today at 00:01 AM.'], as last run was at 0:00 and this one should execute at 0:01.

And the same applies for "Today at 00:30 AM." and "Today at 01:00 AM."

I guess, what you wanted to test, requires a one hour shift in the mock for both timestamps. Then I only have to adjust the test "Today at 02:00 AM." to get everything passed.

The scenario I was going for was creation of ECA configuration at 00:00:01 AM while the cron execution was at 01:00:01 AM, therefore ::getTimestamp would have returned 00:00:01 and ::getCurrentTimestamp would have returned 01:00:01.

Yes, that means that all schedules due between 00:00:01 and 01:00:01 would return TRUE, not FALSE as defined in the test data. That's why using 01:00:01 as creation time and 02:00:01 as current time allows to test that everything before creation time will return FALSE, everything between creation and current time will return TRUE and everything later than current time will return FALSE again.

I'll have a look into that one failing test right now.

That was another timezone issue ;-)

In \Drupal\Tests\eca_base\Unit\CronEventTest::appliesData, the weekday timestamps got calculated and they were missing the timezone definition:

    $weekday_today = date('w', (new \DateTime('now', new \DateTimeZone('UTC')))->getTimestamp());
    $weekday_future = (date('w', (new \DateTime('now', new \DateTimeZone('UTC')))->getTimestamp()) + 2) % 7;

That explains why this fails sometimes: the test server of DrupalCI has a different default timezone and hence it comes back with different results.

Tests are passing on DrupalCI now as well.

The additional test cases make sense and it's great to have more cases being tested.

However, the change in isDue doesn't feel right and digging deeper indicates, that this is most likely because of an unclear expectation. Example: "Tomorrow - Every minute today." - here are the values that we get from that test case:

• Current: 21. June 2022 - 02:00:00
• Last: 20. June 2022 - 02:00:00
• Next: 20. June 2022 - 02:01:00

This will return TRUE because the next due timestamp is smaller than the current timestamp.

Now, your expectation in the test case is to not run the task, because at the current time it is a weekday which is excluded from the cron pattern.

However, there have been many missed executions for that cron pattern, i.e. every minute from 02:01 until 23:59 yesterday. This happens, if the cron scheduler didn't run. The current implementation therefore does execute missed cron runs. And therefore returning TRUE in this case is considered correct.

I can see, that one would want to change this. But doing so would open up all sort of additional issues that would have to be captured. Example: a cron pattern of * 1,7,15 * * * would execute every minute every day, but only between 1 and 2, 7 and 8 and 15 and 16. Our isDue would have to check that as well if it gets executed at 08:10 and gets a due time of 07:59. Should it then be executed or not? Well, following the weekday logic from above, it should not, but following the originally implemented algorithm, it should.

This second example is just made up. The two cases - weekday and a couple of hours - are just a fraction of possible cases where we would have to implement additional checks and adjustments, that we would almost have to implement a complete new cron pattern interpreter. If it's even possible.

All the above is complex and difficult to explain. The originally implemented paradigm can be simply explained as this:

The next due date is calculated based on the given cron pattern and the timestamp that cron was last executed. If that due timestamp is in the past, it is a task that should have been executed then and will therefore be executed now.

It then stores the current timestamp as the last execution timestamp so that it won't execute any arbitrary number of missed tasks, it'll only ever execute one missed task from the past.

This is important, because in Drupal the cron (or this cron event) won't be executed every minute the catch all due tasks. It may only be executed once an hour (or any other regular or unregular period) and therefore needs to pick up all missed tasks from the past just once.

I think we both agree that we cannot bring up the perfect solution here that covers every possible aspect and expectation.

Agreed. The good news: the scenario in the test is pretty hypothetical and pretty unlikely in a production environment. I mean, if one defines cron tasks to run every minute, they also have to make sure that the cron scheduler gets executed every minute too. And then the issue would ever occur.

To mimic that scenario also in our test environment, I've changed the $lastRun values in testApplies so that it is set to 23:59 of the previous day of which will be tested in the next step. With the reverted commit for isDue all tests pass without any other changes required. In other words, even your expectation is being met by assuming that the last run was executed late the day before.

If you're ok with that, this one would be ready to go.

Amazing, this was a good round of improvements!!!