WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 8 weeks ago

#32656 assigned feature request

Add hooks to allow hijacking cron implementation

Reported by: rmccue Owned by: peterwilsoncc
Milestone: 5.0 Priority: normal
Severity: normal Version:
Component: Cron API Keywords: has-patch needs-refresh
Focuses: Cc:

Description

Right now, it's partially possible to hijack wp-cron via the schedule_event filter. However, there's a couple of problems with it.

Cannot Skip Default Behaviour

It's not possible to change the default behaviour of wp-cron safely. If you filter on schedule_event and return an empty value, the cron function (wp_schedule_event etc that was originally called) will return false immediately.

While this does cancel the default behaviour, it causes the function to return false rather than the default null. In well-written plugins, this will cause them to think the event wasn't scheduled or an error occurred.

Inconsistent Usage

While wp_schedule_event and wp_schedule_single_event both call out to schedule_event, the other functions (wp_reschedule_event, wp_unschedule_event, wp_clear_scheduled_hook, wp_next_scheduled) don't. This means that you need to filter at a very low level (pre_option_cron) to be able to catch these, at which point you may be missing the data needed to work out what changed.

Proposal

I'd like to introduce a few new hooks here:

  • pre_schedule_event to run at the top of wp_schedule_event and wp_schedule_single_event
  • pre_reschedule_event to run at the top of wp_reschedule_event
  • unschedule_event to run at the top of wp_unschedule_event
  • clear_scheduled_hook to run at the top of wp_clear_scheduled_hook
  • next_scheduled to run at the top of wp_next_scheduled

These would all take false as their first parameter/filterable value, and act in the same way as pre_option_{$option} -- that is, you can return a non-false value, and the function would return that immediately.

This would make the entire system pluggable, and allow swapping out the pseudo-cron for a real jobs system much more easily.

Attachments (7)

32656.diff (8.2 KB) - added by rmccue 3 years ago.
32656-30apr2016.diff (7.3 KB) - added by DavidAnderson 2 years ago.
Update 32656.diff so that it applies to trunk as at 2016-Apr-30 (no other changes yet)
with-return-codes.diff (12.5 KB) - added by DavidAnderson 2 years ago.
New patch incorporating both the suggested filters, and return codes for all relevant functions
32656.2.diff (15.9 KB) - added by peterwilsoncc 6 months ago.
32656.3.diff (19.0 KB) - added by peterwilsoncc 3 months ago.
32656.4.diff (22.1 KB) - added by peterwilsoncc 3 months ago.
32656.5.diff (24.8 KB) - added by peterwilsoncc 3 months ago.

Download all attachments as: .zip

Change History (27)

#1 @rmccue
3 years ago

One alternative here would be to make the system actually pluggable - that is, wrap with function_exists calls. However, if I recall, we're trying to move away from pluggable functions in favour of filters, hence the proposal here.

@rmccue
3 years ago

#2 @rmccue
3 years ago

  • Keywords has-patch added
  • Owner set to rmccue
  • Status changed from new to assigned

Added patch to add the following:

  • pre_schedule_event - Hijack hook for wp_schedule_single_event and wp_schedule_event
  • pre_reschedule_event - Hijack hook for wp_reschedule_event
  • pre_unschedule_event - Hijack hook for wp_unschedule_event
  • pre_clear_scheduled_hook - Hijack hook for wp_clear_scheduled_hook
  • next_scheduled - Filter on return value for wp_next_scheduled
  • get_schedule - Filter on return value for wp_get_schedule

(In the process, I did some minor formatting changes, but no real functional changes.)

This ticket was mentioned in Slack in #core by dd32. View the logs.


3 years ago

#4 @johnbillion
3 years ago

  • Keywords 2nd-opinion added

These filters make sense, but we need to have a think about return values.

If a plugin hooks into, for example, pre_schedule_event and schedules its own cron event, what should its return value be? If it returns null then core's scheduler will still schedule the event. If it returns something other than null then you're returning an unexpected value from wp_schedule_single_event().

Should we think about always returning a more sane value to indicate success, for example true instead of the current null? What might this break?

Last edited 3 years ago by johnbillion (previous) (diff)

#5 @jrf
3 years ago

Regarding the return value - related ticket: #21072

#6 @rmccue
2 years ago

#36682 was marked as a duplicate.

#7 follow-up: @DavidAnderson
2 years ago

I think that returning a boolean indicating whether or not an event was (re-)scheduled (or cancelled, depending on the call), is the right thing to do. I'm willing to adapt the patch if nobody else is working on it - please let me know.

#8 in reply to: ↑ 7 @rmccue
2 years ago

  • Keywords needs-refresh added

Replying to DavidAnderson:

I'm willing to adapt the patch if nobody else is working on it - please let me know.

Go for it :)

@DavidAnderson
2 years ago

Update 32656.diff so that it applies to trunk as at 2016-Apr-30 (no other changes yet)

@DavidAnderson
2 years ago

New patch incorporating both the suggested filters, and return codes for all relevant functions

#9 @DavidAnderson
2 years ago

  • Keywords needs-refresh removed

The new patch - with-return-codes.diff - builds on the previous patch, by now using boolean return codes for cron functions where relevant, and adding defined returned values for other functions. This is adapted from the patch in #21072, with a few minor modifications.

@rmccue - are you able to take a look?

#10 @DavidAnderson
2 years ago

Any likelihood that this patch might get looked at before WP 4.6?

#11 @DavidAnderson
22 months ago

What needs doing to push this forward? It's not hugely complex, but would be very useful, and has a patch.

#12 @DavidAnderson
15 months ago

@dd32 About a year ago, in https://core.trac.wordpress.org/ticket/36682, you asked @rmccue to review this patch (in a previous version), but it looks like he's not got the capacity. Is there another way that this can go forward?

#13 @peterwilsoncc
6 months ago

  • Milestone changed from Awaiting Review to 5.0
  • Owner changed from rmccue to peterwilsoncc

Let's give this a red-hot go for 5.0.

#14 @peterwilsoncc
6 months ago

32656.2.diff is a refresh of with-return-codes.diff with a few changes for coding standards.

Looking over the code, I think the way some of the data is passed to filters could be altered for consistency between each filter.

#15 @peterwilsoncc
3 months ago

Refreshed in 32656.3.diff :

  • data format for passing events to the filters has been made more consistent
  • Added a hook to allow hijacking of rescheduling functions
  • Rescheduling may call _get_cron_array() before the hijack. This will need to change so wp-cron alternatives aren't required to generate in the cron option format (in both Human Made's Cavalcade and Automattic's Cron-Control this requires some ugly db queries)

#16 @ethitter
3 months ago

These hooks would be a great addition! Cron Control does some interesting things to offload event storage to its own table, and these hooks would significantly simplify that. Allowing plugins to completely bypass the cron option would go a long way towards eliminating race conditions that can occur under load.

The issue in wp_reschedule_event() is an interesting one. The cron array is only needed if the interval can't be determined, which should only happen if a schedule is removed but the events that use it remain. Two options come to mind. The first is to introduce a wp_get_scheduled_event() or some such thing, which wraps the call to _get_cron_array() and the attendant parsing out of the event, and which includes a pre_ filter for hijacking. My second thought was to update wp_next_scheduled() to be able to return more than the event's timestamp. A pre_ filter in _get_cron_array() is something I considered, but it won't scale unless the function accepts arguments that could be used to return a subset of scheduled events; with the pre_ filters in all of the functions that use the cron array, I'm inclined to leave the array helper alone.

#17 @peterwilsoncc
3 months ago

In 32656.4.diff :

  • Added wp_get_scheduled_event() per @ethitter's notes above, this should allow custom runners to avoid filtering options
  • wp_get_schedule() altered to use the new function.
  • Tidied up the docs a bit to include recommended return values and some corrections

@johnbillion: From @DavidAnderson's earlier work, each function returns values for plugins to emulate. Plugin authors can still return unexpected values but that's a problem elsewhere in WP so I'm inclined to let it pass.

I've written too much code in this patch to commit it without someone else adding commit keyword.

#18 @rmccue
3 months ago

  • Keywords commit added; 2nd-opinion removed

This looks good to me. The only thing I'd be concerned about is changing the return value from null to false, which could break existing checks of the return value. I doubt that'll have any issues in reality though.

#19 @peterwilsoncc
3 months ago

In 32656.5.diff:

  • Committable version adding @since tags to docblocks
  • Modified return value for wp_cron() failure for consistency
  • tests passing

#20 @peterwilsoncc
8 weeks ago

  • Keywords needs-refresh added; commit removed

Needs refresh following [43050].

Note: See TracTickets for help on using tickets.