WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 months ago

#32656 closed feature request (fixed)

Add hooks to allow hijacking cron implementation

Reported by: rmccue Owned by: peterwilsoncc
Milestone: 5.1 Priority: normal
Severity: normal Version:
Component: Cron API Keywords: has-patch has-unit-tests needs-dev-note
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 (9)

32656.diff (8.2 KB) - added by rmccue 4 years ago.
32656-30apr2016.diff (7.3 KB) - added by DavidAnderson 3 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 3 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 16 months ago.
32656.3.diff (19.0 KB) - added by peterwilsoncc 13 months ago.
32656.4.diff (22.1 KB) - added by peterwilsoncc 13 months ago.
32656.5.diff (24.8 KB) - added by peterwilsoncc 13 months ago.
32656.6.diff (23.7 KB) - added by peterwilsoncc 9 months ago.
32656.7.diff (4.7 KB) - added by peterwilsoncc 4 months ago.
Version number updates

Download all attachments as: .zip

Change History (38)

#1 @rmccue
4 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
4 years ago

#2 @rmccue
4 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.


4 years ago

#4 @johnbillion
4 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 expected 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?

Version 0, edited 4 years ago by johnbillion (next)

#5 @jrf
3 years ago

Regarding the return value - related ticket: #21072

#6 @rmccue
3 years ago

#36682 was marked as a duplicate.

#7 follow-up: @DavidAnderson
3 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
3 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
3 years ago

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

@DavidAnderson
3 years ago

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

#9 @DavidAnderson
3 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
3 years ago

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

#11 @DavidAnderson
3 years ago

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

#12 @DavidAnderson
2 years 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
17 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
16 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
13 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
13 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
13 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
13 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
13 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
12 months ago

  • Keywords needs-refresh added; commit removed

Needs refresh following [43050].

#21 @peterwilsoncc
9 months ago

  • Keywords has-unit-tests added; needs-refresh removed

In 32656.6.diff:

  • conflicts following [43050] removed
  • added tests for new filters
  • otherwise it's pretty much the same as the .4 patch

#22 @peterwilsoncc
9 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 43540:

Cron: Add hooks and a function to allow hijacking cron implementation.

This allows sites with a large cron option or a custom cron implementation to hijack the cron option to store cron data using custom functionality.

wp_get_scheduled_event() is new function to retrieve the event object for a given event based on the hook name, arguments and timestamp. If no timestamp is specified the next occurence is returned.

Preflight filters are added to all functions that read from or modify the cron option: pre_schedule_event, pre_reschedule_event, pre_unschedule_event, pre_clear_scheduled_hook, pre_unschedule_hook, pre_get_scheduled_event and pre_next_scheduled.

Additionally, the post scheduling hooks next_scheduled and get_schedule to allow plugins to modify an event after retrieving it from WordPress.

Props rmccue, DavidAnderson, ethitter, peterwilsoncc.
Fixes #32656.

#23 @DavidAnderson
9 months ago

Out of interest, is there anyone who's already planning to leverage these hooks to create an "alternative cron back-end" plugin?

#24 @archon810
8 months ago

I'm curious if it will affect https://github.com/humanmade/Cavalcade in a positive way.

#25 @peterwilsoncc
7 months ago

  • Milestone changed from 5.0 to 5.1
  • Resolution fixed deleted
  • Status changed from closed to reopened

Milestone changed to the 5.1 branch, inline docs will need to be updated.

#26 @coffee2code
5 months ago

Related: #45445

(Commits relating to this ticket incorrectly used @see syntax in dockblocks.)

#27 @desrosj
4 months ago

  • Keywords needs-dev-note added

@peterwilsoncc
4 months ago

Version number updates

#28 @peterwilsoncc
4 months ago

In 44375:

Docs: Correct inline documentation versions for Cron API changes.

[43540] introduced changes to allow hijacking the cron implementation.

These changes were slated for 5.0, but have since been moved to the 5.1 release.

---

Added manually as the commit referenced the incorrect ticket number.

#29 @SergeyBiryukov
4 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

comment:25 appears to be addressed in [44375].

Note: See TracTickets for help on using tickets.