Make WordPress Core

Opened 8 years ago

Closed 5 years ago

#35968 closed defect (bug) (fixed)

wp_reschedule_event timestamp overridden when set in the future

Reported by: svovaf's profile svovaf Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 5.1 Priority: normal
Severity: normal Version: 4.4.2
Component: Cron API Keywords: good-first-bug has-patch
Focuses: docs Cc:

Description

I'm doing some work with WP cron and I noticed two issues in the wp_reschedule_event() code (wp-includes/cron.php).

The first problematic logic is the following:

<?php
        if ( $timestamp >= $now ) {
                $timestamp = $now + $interval;
        } else {
                $timestamp = $now + ( $interval - ( ( $now - $timestamp ) % $interval ) );
        }

Based on the function documentation, $timestamp is the Timestamp for when to run the event.
Let's say that I want to set the first run of the rescheduled event in one hour from now. This if statement overrides the $timestamp based on the interval, practically ignoring my initial $timestamp. If the logic is correct, then something in the documentation should be changed because it's not clear at all.

Btw. I searched trac and found this:
https://core.trac.wordpress.org/changeset/10969

The second issue I see is the last line in that same function:

<?php
        wp_schedule_event( $timestamp, $recurrence, $hook, $args );

Inspecting the code of wp_schedule_event the cron is scheduled based on the given timestamp. What it means is if the originally scheduled task execution time does not match to the new rescheduled $timestamp, it will not override the cron but will add a new one.

Maybe be there's some "dark magic" behind the scenes that makes it all work, but from reading the code + documentation, things doesn't make sense.

Attachments (3)

35968.patch (597 bytes) - added by Dharm1025 6 years ago.
doc updated for $timestamp
42897.diff (99.7 KB) - added by peterwilsoncc 5 years ago.
35968.diff (1.2 KB) - added by peterwilsoncc 5 years ago.

Download all attachments as: .zip

Change History (15)

#1 @jrf
8 years ago

Hi @svovaf,

Thanks for reporting this. I've just had a look at this and while I understand what you're saying, I believe this is not a bug.

If you want to schedule a recurring event, you should use the wp_schedule_event() function and set the $recurrence parameter. The $timestamp you provide will be respected.

wp_reschedule_event() is - more than anything - intended for internal use. It is used during the running of a cron job to automatically reschedule a recurring event. In other words, it's called at approximately the time as set in $timestamp when the event was scheduled and used to schedule the next time this event should run.
With that usage in mind, there's nothing really wrong with the code, though I agree the function description could be enhanced a little to highlight the intended use of the function.

#2 @svovaf
8 years ago

Gotcha. It would be great if you guys could update the documentation of the function.

This ticket was mentioned in Slack in #docs by morganestes. View the logs.


7 years ago

#4 @DrewAPicture
6 years ago

  • Keywords needs-patch good-first-bug added

@svovaf @jrf Would either of you like to create a patch for improving the documentation?

@Dharm1025
6 years ago

doc updated for $timestamp

#5 @Dharm1025
6 years ago

  • Keywords has-patch added; needs-patch removed

#6 @DrewAPicture
6 years ago

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

Assigning to mark the good-first-bug as "claimed".

@peterwilsoncc
5 years ago

#7 @peterwilsoncc
5 years ago

  • Focuses performance removed
  • Milestone changed from Awaiting Review to 5.0
  • Owner changed from DrewAPicture to peterwilsoncc
  • Status changed from reviewing to accepted

42897.diff refreshes the existing patch against trunk and expands the function's documentation to include some of @jrf's explanation above.

#8 follow-up: @jrf
5 years ago

@peterwilsoncc I get the feeling you may have just uploaded the wrong patch to this ticket ?

@peterwilsoncc
5 years ago

#9 in reply to: ↑ 8 @peterwilsoncc
5 years ago

Replying to jrf:

@peterwilsoncc I get the feeling you may have just uploaded the wrong patch to this ticket ?

So do i...

35968.diff refreshes the existing patch against trunk and expands the function's documentation to include some of your explanation above. ;P

#10 @peterwilsoncc
5 years ago

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

In 43608:

Cron API: Clarify documentation for wp_reschedule_event().

Expands documentation to indicate wp_schedule_event() ought to be used for rescheduling an upcoming event, while wp_reschedule_event() is used for internally rescheduling a recurring event after it runs.

Props Dharm1025, jrf.
Fixes #35968.

#11 @peterwilsoncc
5 years 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.

#12 @pento
5 years ago

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

Inline docs don't need updating for this ticket.

Note: See TracTickets for help on using tickets.