Don't spawn cron requests for suspended blogs

[ryan]

For multisite, spawning cron requests is wasteful and unnecessary. wp_cron()/spawn_cron()/wp-cron.php should check the blog's status and bail if the blog is suspended.

[ryan]

Cribbing from ms_site_check(), the conditions to check:

if ( '1' == $current_blog->deleted || '2' == $current_blog->deleted || '1' == $current_blog->archived || '1' == $current_blog->spam )
 return;

Or from get_blogs_of_user():

if ( $blog->archived || $blog->spam || $blog->deleted )
     return;

[nacin]

I think spawning is unnecessary. wp-cron.php should still be able to be triggered, though. Someone somewhere is surely doing something weird with regards to this.

I think it would be best if bailed early in spawn_cron() and didn't hook wp_cron() directly into sanitize_comment_cookies. That would allow for wp_cron() and wp-cron.php to still execute.

I could also just be overly careful.

[ryan]

I can see it being good and bad, at least when considering core events. A hard freeze prevents future posts from accidentally getting published and pingbacks from accidentally going out from a blog that should be dead. The blog and the cron queue are frozen. Then again, there could be some events that should still be processed. This was requested by some wp.com folks to shut up zombie blogs. How dead is dead enough?

[ryan]

And, yes, spawning should be shut down and the hook avoided. Finishing the patch tomorrow.

[kurtpayne]

Alternate patch

[kurtpayne]

20537.2.patch​ is the same as 20537.diff​, but in a different place. It prevents the cron request from spawning for zombie blogs, but still allows wp-cron.php to be called directly.

Still a bit new to the cron code. Is this right?

[nacin]

Replying to ryan:

How dead is dead enough?

I'm convinced. Both spawn_cron() and wp-cron should bail. Maybe a filter in wp-cron for the creative.

[swissspidy]

20537.2.patch​ is a good start. Needs a refresh using a new filter, so people could still spawn cron requests for these blogs when needed.

[jrf]

Refreshed patch, including filter which allows to force skip/force run cron anyhow.

[jrf]

New patch uploaded which combines the previous two patches and adds a filter as suggested.

Both previous patches are needed if we want to bail early:

• wp_cron() would be the earliest point of entry for the poor mans cron run
• wp-cron.php for those of us who run real cron jobs.

[swissspidy]

Marking this "good-first-bug" as claimed.

[jrf]

The patch is a good start. The docbloc needs to be adjusted to show the global $current_blog added into wp_cron and the new filter is missing a version number in the docbloc

Updated patch for feedback from Slack chat.

[jrf]

Updated docblocks

[jeremyfelt]

Thanks for the patches, @jrf, this looks close.

• Let's use get_blog_details() instead of accessing the global $current_blog. See #22090.
• How about naming the filter skip_cron_for_site and not using the "blog" terminology here?
• Now that WP_Site is available, the filter docs should be adjusted to show that a WP_Site object is provided.

[jrf]

Hi @jeremyfelt

Thanks for the patches, @jrf, this looks close.

• Let's use get_blog_details() instead of accessing the global $current_blog. See #22090.
• How about naming the filter skip_cron_for_site and not using the "blog" terminology here?
• Now that WP_Site is available, the filter docs should be adjusted to show that a WP_Site object is provided.

Thanks for the feedback.

I've adjusted the patch. Changes include:

• Switched over to using get_blog_details() as per your suggestion. This does mean an extra level of nesting as the function is only available when on multi-site.
• Renamed the filter. Some consistence in the naming here would be awesome, get_blog_details() versus the use of site in the filter can lead to confusion.
• Adjusted the documentation to mention WP_Site.
• Adjusted the documentation to make it extra clear when a site is regarded as suspended.
• Updated the @since tag for the filter to 4.6.0.

Little side-note: I would expect WP_Site->public to be false (0) for suspended blogs, but it still shows as 1 (true).

[jeremyfelt]

Thanks, @jrf, I think we're getting close.

20537.2.diff​ makes a couple changes:

• Use $spawn_cron as the variable tracking whether or not to spawn. Default to true rather than false.
• Name the filter spawn_cron_for_site and adjust filter docs accordingly.

Pinging @DrewAPicture to have a look at the filter too. It feels a little weird to either pass null or a WP_Site instance, but that might be all we have.

[jrf]

@jeremyfelt

If you reverse the variable logic, please do so consistently:
In src/wp-cron.php you need to change true to false in the following line:

if ( true === apply_filters( 'spawn_cron_for_site', $spawn_cron, $current_site ) ) {
        die();
}

I also believe that the additional documentation in the filter about when a site is regarded as suspended should be put back in as this may be clear to us, but not automatically so for other people without looking at the actual code.

[flixos90]

Replying to jeremyfelt:

It feels a little weird to either pass null or a WP_Site instance, but that might be all we have.

Looking at this, I think it would probably make sense to only run this filter on a Multisite setup and otherwise always have $spawn_cron set to true. This way we can always pass a WP_Site instance, and on a single site, having a filter like this doesn't sound useful to me (as you could just DISABLE_WP_CRON there).

Replying to jrf:

In src/wp-cron.php you need to change true to false in the following line:

Yep - we all make mistakes. :)

Another minor update needed, let's use get_site() instead of get_blog_details(), now that that function is available.

[jeremyfelt]

I'm going to move this to 4.7-early. This is a long standing bug and we shouldn't be introducing a new filter this late without some more discussion.

[jorbin]

@jeremyfelt Still targetting this for 4.7? If so, what do you need?

[flixos90]

This might be close, but there hasn't been any activity lately, and it still needs some testing. Let's get back to it for the next release.

[stevenkword]

20537.2.diff​ still applies cleanly against trunk as of Jul 10, 2017. However, further user testing is still required. New contributors looking to get involved could provide a much-needed benefit through some more user testing.

@jeremyfelt -- Do you still feel further discussion around this new filter is required?

[peterwilsoncc]

Needs refresh:

• no longer applies cleanly following coding standards fix
• reversed logic noted in comment 16
• to use get_site() or get_blog_details()

@flixos90 and @jeremyfelt is this something you'd still like to get in?

[peterwilsoncc]

Moving this to the 5.7 milestone as it will be fixed as a side-effect of #24160 if that goes in.

[peterwilsoncc]

For #24160, I'm moving alternative wp-cron to run on the wp_loaded hook.

Having reviewed what happens on multisite for regular cron, hitting /site-name/wp-cron.php already shuts down prior to jobs running unless the user is logged in. For loopback requests this is not the case.

Plugins can use the ms_site_check filter to prevent suspended blogs from shutting down, in which case both standard and alternative cron will continue to fire.

Given the option exists (and I am unsure if it did when this ticket was last discussed) I'm moving the hook without considering the edge case above.

[peterwilsoncc]

In 50135:

Cron API: Run alternative wp-cron later, do not run on archived blogs.

Runs cron jobs later on sites using alternative cron, ie the ALTERNATE_WP_CRON constant is true, to more closely match when standard cron jobs are run. Jobs now run on the wp_loaded hook at priority 20. Prior to this change they would run on the init hook. This ensures custom post types and taxonomies are registered prior to the jobs running.

This change also prevents alternative wp-cron from running on archived or suspended multisite blogs as these are shut down prior to the wp_loaded hook from running.

Moves the existing functionality of wp_cron() in to a new private function _wp_cron().

Props flixos90, jeremyfelt, johnbillion, jrf, kurtpayne, nacin, peterwilsoncc, prettyboymp, r-a-y, ryan, stevenkword, swissspidy.
Fixes #20537, #24160.