WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 3 months ago

#21072 closed enhancement (fixed)

CRON often returns ambiguous values

Reported by: evansolomon Owned by: peterwilsoncc
Milestone: 5.0 Priority: normal
Severity: normal Version:
Component: Cron API Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

Many of the cron API functions return ambiguous values. For example, wp_cron() returns null (explicitly) when cron doesn't run, but it also returns null (implicitly) every other time. This can make debugging harder.

Whenever possible, I think it's valuable to return meaningful values. The attached patch does that.

Attachments (5)

20172.diff (5.4 KB) - added by evansolomon 6 years ago.
0001-Refresh-of-evansolomon-s-patch-for-issue-20172.patch (9.6 KB) - added by jrf 3 years ago.
Refresh of the earlier patch.
21072.diff (17.3 KB) - added by peterwilsoncc 3 months ago.
21072.2.diff (17.3 KB) - added by peterwilsoncc 3 months ago.
21072.3.diff (17.9 KB) - added by peterwilsoncc 3 months ago.

Download all attachments as: .zip

Change History (17)

@evansolomon
6 years ago

#1 @SergeyBiryukov
6 years ago

  • Keywords has-patch added

#2 @ace_dent
5 years ago

  • Cc ace_dent added

#3 @nacin
4 years ago

  • Milestone changed from Awaiting Review to Future Release

I think this is good. While there are technically return value changes, I'm not too concerned about the back compat.

wp_clear_scheduled_hook() probably just needs to return true (it got cleared) or false, versus an array. It's also possible we change how that function works in the future, #25773 gave us some trouble.

The wp_remote_post() call is non-blocking, so spawn_cron() should probably not have a return value, right? Thus wp_cron() shouldn't have a return value either. I know that was the original bug report — what about true? Is that sufficient?

#4 @chriscct7
3 years ago

  • Keywords needs-refresh added

@jrf
3 years ago

Refresh of the earlier patch.

#5 @jrf
3 years ago

  • Keywords needs-refresh removed

I've refreshed @evansolomon's patch.

(and boyscouted inline control structures in the touched functions).

#6 @ericlewis
3 years ago

#31598 was marked as a duplicate.

#7 @swissspidy
18 months ago

  • Owner set to swissspidy
  • Status changed from new to reviewing

#8 @peterwilsoncc
3 months ago

  • Milestone changed from Future Release to 5.0
  • Owner changed from swissspidy to peterwilsoncc
  • Status changed from reviewing to accepted

This got dragged in to the patch for #32656 as return values are needed for that too.

I'll take into account @nacin's comment re return values as part of that.

#9 @peterwilsoncc
3 months ago

  • Keywords has-unit-tests added

In 21072.diff:

  • wp_schedule_single_event(), wp_schedule_event(), wp_reschedule_event() and wp_unschedule_event(): boolean outcome
  • wp_clear_scheduled_hook(): false if fails to unschedule one or more events, zero if no events need to be unscheduled, or number of events unscheduled. If there are events to unschedule, partial success is possible.
  • wp_unschedule_hook() : false if it fails to unschedule events, zero if no events need to be unscheduled, or number of events unscheduled. If there are events to unschedule, partial success is not possible.
  • spawn_cron() false if no attempt to spawn, outcome of wp_remote_post() if an attempt is made (either an array or WP_Error)
  • wp_cron(): false if one or more events fail to spawn, zero if no events require spawning, integer indicating number of events spawned if all successful
  • _set_cron_array(): boolean outcome of update_option().

Tests updated to include return values.

@peterwilsoncc
3 months ago

#10 @peterwilsoncc
3 months ago

Working on this on a Github PR

#11 @peterwilsoncc
3 months ago

  • Keywords commit added

21072.3.diff is a tidy up of earlier patches:

  • Fixes some CS errors
  • Ensures _set_cron_array is only called when need when unscheduling

I've got a 👌 from Pento on the Github PR so labeling commit for next time I've got vvv runnning.

#12 @peterwilsoncc
3 months ago

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

In 43050:

Cron API: Return meaningful values from cron functions.

Return values added to Cron API functions to indicate outcome:

  • wp_schedule_single_event(), wp_schedule_event(), wp_reschedule_event() and wp_unschedule_event(): boolean indicating success or failure,
  • wp_clear_scheduled_hook(): integer indicating number of jobs cleared (zero or more), false if one or more jobs fail to clear,
  • wp_unschedule_hook(): integer indicating number of jobs cleared (zero or more), false if the jobs fail to clear,
  • spawn_cron(): boolean indicating whether job spawned,
  • wp_cron(): integer indicating number of jobs spawned (zero or more), false if one or more jobs fail to spawned,
  • _set_cron_array(): boolean outcome of update_option().

Props evansolomon, jrf, peterwilsoncc, pento for code review.
Fixes #21072.

Note: See TracTickets for help on using tickets.