wpsharks / comment-mail

A WordPress plugin enabling email subscriptions for comments.
http://comment-mail.com
GNU General Public License v3.0
8 stars 3 forks source link

checkCronSetup runs on every request #344

Closed mundschenk-at closed 6 years ago

mundschenk-at commented 6 years ago

It appears that the method Plugin::checkCronSetup runs on every request and alway triggers several indirect calls to load_all_options (via update_option and _get_cron_array). Why is this necessary?

Environment: I've disabled the WP_CRON mechanism and run all my cronjobs (wp-cron.php) via the real cron.

raamdev commented 6 years ago

@mundschenk-at That routine sets up, validates schedules, and reschedules events if necessary, however it should only be calling update_option() if the necessary cron events have not already been scheduled. See https://github.com/websharks/comment-mail/blob/6573c2dba69877b2789962e7534d85274ed8c621/src/includes/classes/Plugin.php#L2593-L2623

mundschenk-at commented 6 years ago

@raamdev I know, I've looked at the code. Still, somehow this gets triggered on every request. For what it's worth, this is a multisite installation (with comment-mail being only enabled on some sites).

mundschenk-at commented 6 years ago

@raamdev I've added some debugging output to Plugin.php. The problem is that somehow, sha1(serialize(wp_get_schedules())) seems to alternate between two different hash values, triggering a "rebuild" on every check. Further debugging has shown that this stems from from the 10min schedule created by Broken Links Checker.

I've got no clue why that schedule is not added on every request, but why would adding a new schedule necessitate a rescheduling in the first place?

raamdev commented 6 years ago

Further debugging has shown that this stems from from the 10min schedule created by Broken Links Checker.

That's great to know! Thank you very much for sharing the results of your debugging. I wonder if the Broken Links Checker plugin is re-creating the 10min schedule on every request, causing the hash value to change?

why would adding a new schedule necessitate a rescheduling in the first place?

It's less about adding a new schedule and more about changing an existing schedule. For example, there are plugins that let you customize the timing of existing cron schedules (e.g., WP Crontrol). So if a site owner modified the every5m or hourly cron schedules so that they recurred at something other than every 5 minutes or every 1 hour, or if they used the WP Crontrol plugin to delete the scheduled Comment Mail cron events, Comment Mail needs to know about this and reschedule the events (because these events are critical to the function of the plugin).

So the way Comment Mail checks if the cron schedules have been modified is by storing a hash of the output from wp_get_schedules() and comparing that hash to the value stored in the Comment Mail crons_setup_on_wp_with_schedules option. If they differ, that means the cron schedules have changed and so Comment Mail runs through the setup routine to be safe.

So it sounds like the Broken Links Checker plugin is doing something funky with the cron schedules that is causing this odd behavior. The output of wp_get_schedules() should not change unless the cron schedules are actually modified.

mundschenk-at commented 6 years ago

They only added the cron schedules when on the admin side (or DOING_CRON). I've created a PR to fix this.