WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 5 months ago

#17957 reopened enhancement

wp_reschedule_event & daylight saving

Reported by: MattyRob Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.2
Component: Cron API Keywords: needs-codex
Focuses: Cc:

Description

When a recurring event is established using the WP cron functions the function takes a Unix timestamp and a recurrence interval.

In the situation where daylight saving changes the local time, the timing of an event can change by an hour. So, if a database backup is set to run at midnight when the clocks change this can start to happen at 11pm or 1am instead since the timing of the event is based on an initial static time (Unix timestamps reference point is January 1, 1970) and a static interval period.

An enhancement to the cron functions would be to account for an obey daylight saving changes so that events schedule for midnight actually occur at midnight irrespective of the time of year.

Attachments (1)

17957.diff (1.8 KB) - added by pento 6 months ago.

Download all attachments as: .zip

Change History (17)

comment:1 nacin20 months ago

  • Keywords reporter-feedback added; dev-feedback removed

Events should be scheduled with time(), which is always UTC in WP core. So if you're scheduling midnight UTC, or an offset of that, you're fine.

comment:2 MattyRob20 months ago

  • Keywords reporter-feedback removed
  • Resolution set to invalid
  • Status changed from new to closed

I'd forgotten all about this ticket! :)

I think, on looking at my coded plugins that I must have figured out to use time() at some point and started using that rather than current_time().

This might be more of a documentation fix then:
http://codex.wordpress.org/Function_Reference/wp_schedule_event
This page says to use current_time('timestamp');

http://codex.wordpress.org/Function_Reference/wp_reschedule_event
This page doesn't specify

So, is the PHP time(); function bet or the WordPress current_time('timestamp');?

Last edited 20 months ago by SergeyBiryukov (previous) (diff)

comment:4 helenyhou20 months ago

  • Milestone Awaiting Review deleted

comment:5 MattyRob20 months ago

Just reconsidering the above...

Firstly, perhaps the more appropriate codex pages to consider are:
http://codex.wordpress.org/Function_Reference/wp_schedule_single_event
http://codex.wordpress.org/Function_Reference/wp_schedule_event

The former is using time()+3600 in the example while the latter uses current_time('timestamp').

current_time('timestamp') could be in the future or past by up to 12 hours if I'm understanding the time offsets correctly, so perhaps we should be encouraging use of time() plus an interval to ensure a future date is used.

Also, as this has been marked with 'needs-codex' does it need re-opening so it doesn't get lost?

(I'm quite happy to make codex changes but don't want to move the recommendations contrary to the core developers opinions)

Last edited 20 months ago by SergeyBiryukov (previous) (diff)

comment:6 SergeyBiryukov20 months ago

  • Milestone set to Awaiting Review
  • Resolution invalid deleted
  • Status changed from closed to reopened

Looks like time() was replaced with current_time( 'timestamp' ) there to prevent the confusion when the first occurrence of the event is not in local time.

Reopening until the consensus about the best practice is reached.

comment:7 MattyRob15 months ago

Over the last few months I've tried different appraches to this, I find that these both work:

$time = current_time('timestamp') + $interval;
while ($timestamp < current_time('timestamp')) {
	// if we are trying to set the time in the past increment it forward
	// by the interval period until it is in the future
	$timestamp += $interval;
}

And:

$time = time() + $interval;
while ($timestamp < time()) {
	// if we are trying to set the time in the past increment it forward
	// by the interval period until it is in the future
	$timestamp += $interval;
}

However, I'd have to say that using current_time() seems to me more reliably offset to GMT on local servers and needs less manipulation than the results from time().

comment:8 smerriman12 months ago

I don't quite understand the followup comments to this ticket, but the original point is still valid.

For recurring events, wp_schedule_event is called once with the 'daily' argument. Regardless of how the initial timestamp is calculated, every future calculation is based on adding 86400 to the previous timestamp, so when daylight savings kicks in the event will run at a different hour local time.

I would imagine every use of the 'daily' argument would want the event to run at a specific hour, local time. To achieve that currently you would have to basically ignore the recurring nature of wp_schedule_event and manually reschedule the event each day at the right hour to ensure this didn't happen.

While changing the functionality would require a few 'hacks', it would make more logical sense to me.

comment:9 Ipstenu6 months ago

Small note: This comes up with WP 3.7 because of the auto-updater. All my checks run on the 6s now instead of the 7s. Thus all the hard work put into making sure this works is busted for half the year. Or until we finally riot in a blaze or angooooorrr and rip DST out of our lexicon, like the archaic concept that it is!

Raaaaaar.

Till then, can we get this fixed before 3.7.2 as it's gonna mess up people who schedule things to, say, backup before the upgrade runs.

pento6 months ago

comment:10 pento6 months ago

attachment:17957.diff

So, here's a wildly untested proof of concept that changes cron times when DST comes around. Now let's discuss why it's a terrible idea, and we should forget I ever wrote this code.

  • WP-cron is not, and should not be treated like a reliable cron system. If you want to do things at a specific time, use actual cron.
  • There are probably a bunch of side effects to just changing the cron time by an hour. Not as bad as changing the server time by an hour, but still pretty bad.
  • People who change their server timezone from UTC are Bad People, and deserve to be punished for their transgressions. I briefly considered this as an alternate patch:
Index: src/index.php
===================================================================
--- src/index.php	(revision 25998)
+++ src/index.php	(working copy)
@@ -1,4 +1,6 @@
 <?php
+if ( date_default_timezone_get() !== 'UTC' )
+	exit;
 /**
  * Front to the WordPress application. This file doesn't do anything, but loads
  * wp-blog-header.php which does and tells WordPress to load the theme.

comment:11 pento6 months ago

Oh, I forgot a set_option( 'cron-dst-change', time() ); at the end of _check_cron_for_dst(). But you get the idea.

comment:12 rmccue5 months ago

IMO, this should be handled higher up, in this case by the auto-updater. I don't think WP Cron should be handling this at all.

comment:13 pento5 months ago

I agree, I don't think WP Cron should be caring about DST changes.

I also don't think the auto updater needs to care either, but that's for another ticket if someone wants to open it.

For reference, if people really need something to run just before an auto update occurs, here's a gist for you to copy/paste:

https://gist.github.com/pento/7330005

comment:14 MattyRob5 months ago

This ticket is not about changing the server time nor the WordPress time. It's more about the fact that events are scheduled and re-scheduled using UTC timestamps rather than local timestamps.

These local timestamps are relative to UTC of course but when DST changes occur in a calendar year the result is that currently scheduled events can move forwards or backwards an hour. I'm not sure they would do this is the local time was used as that updates with DST.

I am fully aware that WP Cron is not as reliable as server cron however when an event is schedule at a particular time users will expect it to happen at that time year round, not to move twice a year.

This is already a low priority to fix as it's been open for 2 years but has only resurfaced due to the overlap with the AutoUpdate code.

comment:15 mboynes5 months ago

  • Cc mboynes@… added

comment:16 mboynes5 months ago

As I see it, this isn't a bug per se. There are some recurring tasks that you want to run every n seconds, and there are some tasks that you want to run at specific times. wp_schedule_event is built for the former, but it would be nice to be able to also accommodate the latter, either through a separate function or through a modification to wp_schedule_event. For recurring events that are intended to be run at specific times, DST isn't the only issue; timezone changes (via Settings -> General) would also affect when a task runs.

Note: See TracTickets for help on using tickets.