Fix bug with Auto-Cache Engine cron disappearing in some scenarios

[raamdev]

See wpsharks/comet-cache#613

[raamdev]

@jaswsinc A review, please. Here's what I did to fix wpsharks/comet-cache#613 and why:

• Reset crons_setup to 0 on uninstall (always); this ensures that cron events are always recreated upon activation.
• Clear all scheduled cron events on uninstall (always); while this isn't completely necessary given the above change, I feel that it removes confusion around why _cron_zencache_auto_cache disappears (reason: because it uses a custom schedule) and _cron_zencache_cleanup stays behind (reason: it uses a built-in schedule). Instead of trying to explain that, let's just remove them both on uninstall and recreate them both on activation.
• Reset the timestamp in Plugin.php so that any previous installations currently experiencing this issue are fixed (i.e., force both events to be recreated, so that a possibly missing Auto-Cache Engine cron event is recreated).

[jaswrks]

Looks great Raam! You might also consider adding this block to the deactivate() handler just above that routine, because deactivating a plugin that sets a custom schedule can result in this data loss also.

[jaswrks]

After reviewing my own comment here: wpsharks/comet-cache#613 (comment)

I suggest that we also validate the configured cache cleanup schedule before we attempt to use it. In addition, the crons_setup key should include a hash of the serialized array of configured CRON jobs on the WordPress installation where it runs. That way we can automatically pick up changes in the set of configured CRON schedules, and revalidate and reschedule our own CRON jobs.

[raamdev]

@jaswsinc writes...

I suggest that we also validate the configured cache cleanup schedule before we attempt to use it

Do you mean at the start of running the hooked function (e.g., cacheCleanup() and autoCache()), or before the add_action() that attaches those methods to the cron events?

[raamdev]

Oh, I think you're referring to validating $this->options['cache_cleanup_schedule']. If that's the case, then we'll need to do that before this line, right? Actually, that whole conditional might need to be rewritten a bit.

I see the intention with that line was to prevent unnecessarily calling wp_clear_scheduled_hook() and wp_schedule_event() if it was not necessary to recreate those events, correct?

So I'm thinking we need to validate $this->options['cache_cleanup_schedule'] and if it fails, run this conditional along with reseting the value of $this->options['cache_cleanup_schedule'] to the default hourly value, then clear and reschedule the events, correct?

[raamdev]

I suggest that we also validate the configured cache cleanup schedule before we attempt to use it.

I think this will do it: 861cc45

[raamdev]

In addition, the crons_setup key should include a hash of the serialized array of configured CRON jobs on the WordPress installation where it runs. That way we can automatically pick up changes in the set of configured CRON schedules, and revalidate and reschedule our own CRON jobs.

I'm not seeing why this would be necessary given my last commit (861cc45). Am I missing something?

[raamdev]

@jaswsinc Well, if we created a hash of wp_get_schedules() we could detect when the schedules change and then force-recreate our own schedules/events to ensure that they stay there (for example if a site owner accidentally deleted the every15m schedule, it would not get recreated automatically unless we were doing so whenever the wp_get_schedules() changed). Is that what you were referring to?

[jaswrks]

Oh, I think you're referring to validating $this->options['cache_cleanup_schedule']. If that's the case, then we'll need to do that before this line, right. Actually, that whole conditional might need to be rewritten a bit.

Exactly, yes.

I see the intention with that line was to prevent unnecessarily calling wp_clear_scheduled_hook() and wp_schedule_event() if it was unnecessary, correct?

Right. To only reschedule when necessary.

So I'm thinking we need to validate $this->options['cache_cleanup_schedule'] and if it fails, run this conditional along with reseting the value of $this->options['cache_cleanup_schedule'] to the default hourly value, then clear and reschedule the events, correct?

Right, exactly. And, whatever we do there should be optimized in every way possible since it will run on every page view. So our strategy should be one that reduces the amount of work it takes to run a quick check. Calling wp_get_schedules() could be a part of that validation, and that's why I suggested a hash.

<?php
sha1(serialize(wp_get_schedules()));

---

It might help if we break apart those bits of information into separate option keys, because there are enough of them now that it's beginning to create some unnecessary complexity. We are trying to stuff everything into crons_setup at the moment, when actually we could break it down a bit.

'crons_setup' => '0', // A timestamp when last set up.
'crons_setup_on_namesapce' => '', // The namespace on which they were set up.
'crons_setup_with_cache_cleanup_schedule' => '', // The cleanup schedule selected by site owner during last setup.
'crons_setup_on_wp_with_schedules' => '', // A sha1 hash of `wp_get_schedules()`

With those new option keys we can have something easier to read....

if ($this->options['crons_setup'] < 1439005906
    || $this->options['crons_setup_on_namespace'] !== __NAMESPACE__
    || $this->options['crons_setup_with_cache_cleanup_schedule'] !== $this->options['cache_cleanup_schedule']
    || $this->options['crons_setup_on_wp_with_schedules'] !== sha1(serialize(wp_get_schedules()))) {

    // ... Recreate.
}

---

One possible problem with this, is the sequence of events with respect to us calling upon wp_get_schedules(). Some plugins that will filter this array of configured schedules, might not do that until the init hook is fired. ZenCache setup (where this code exists now), runs before the init hook, using the hook after_setup_theme. So the array of schedules that we get from a call to wp_get_schedules() in this block of code might not give us an accurate reading.

Even if we attached ZenCache to the init hook, we would need to do that on a very late priority so that we would be checking wp_get_schedules() after everything else has been attached to it.

So for that reason, you might think about moving this set of checks against the crons_setup_* related keys, to a separate method somewhere, and then attaching that method to the init hook and running it very late in the game.

add_action('init', array($this, 'checkCronSetup'), PHP_INT_MAX);

[jaswrks]

We have this file where we could move that entire CRON-related block:
https://github.com/websharks/zencache-pro/blob/000000-dev/src/includes/closures/Plugin/CronUtils.php

[raamdev]

We have this file where we could move that entire CRON-related block:

Already done. 😄 About to push a commit.

[raamdev]

It might help if we break apart those bits of information into separate option keys

Totally agree. I started to rewrite the code to stuff the hash into the existing cron_setup option and was thinking that it was getting overly complex just as you posted the above suggestion. :-)

One possible problem with this, is the sequence of events with respect to us calling upon wp_get_schedules().
[...]
you might think about moving this set of checks against the crons_setup_* related keys, to a separate method somewhere, and then attaching that method to the init hook and running it very late in the game.

Yep, I totally agree again. A separate method is exactly what's in order.

[raamdev]

@jaswsinc Thanks for the great pointers and ideas for how to improve this. :-) See 7a652a7

I ran some tests to confirm things are behaving as expected. One thing to point out is my inclusion of wp_next_scheduled() in the conditional. I discovered we should add that when I tried manually deleting the _cron_zencache_cleanup event just to see what would happen (we weren't detecting that possibility, so it never came back). Now you can try to delete either of our cron events and they will just reappear immediately (as long as ZenCache is active).

[raamdev]

This bug in your example had me scratching my head for several minutes when things failed during my initial tests (my fault for copying/pasting, haha):

'crons_setup_on_namesapce' => '',

[jaswrks]

'crons_setup_on_namesapce' => '',

🙈 Sorry!

See 7a652a7

Outstanding! That looks so much better organized now too.

• I suggest wrapping this line with the strip-from="lite" comment markers.
• It looks like there are a couple of lines in this class member where you could do the same.

Otherwise it looks perfect to me.

[jaswrks]

I love the resetCronSetup() method and how you refactored the installation utilities to take advantage of that. Nice!

[raamdev]

strip-from="lite" comment markers.

Done. Thanks! f67b8b4

Otherwise it looks perfect to me.

Awesome. Thanks so much for the reviews!