WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 5 years ago

#9285 closed defect (bug) (fixed)

Ensuring scheduled posts publish on time.

Reported by: hailin Owned by: westi
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.8
Component: Date/Time Keywords: has-patch early
Focuses: Cc:

Description

Some imported posts have identical post_date and post_date_gmt,
despite the fact the time zone setting is UTC-5.

This can cause issues when the post is set to publish in a future date, as the dates were wrong to begin with.

I think we should have mechanism to sanity checking these, at various places such as inserting a post, modifying it, and scheduling it for future.

Attachments (9)

9285_postdate.diff (1007 bytes) - added by hailin 6 years ago.
patch
9285_postdate.2.diff (865 bytes) - added by hailin 6 years ago.
revised patch
9285.diff (2.1 KB) - added by technosailor 5 years ago.
Initial proof of concept. This code worked in unit testing outside of WP but fails to calculate DST correctly in WP. Releasing as a POC anyway and will continue to improve.
9285-2.diff (2.2 KB) - added by technosailor 5 years ago.
Revision of patch fixes existing problems. Should be solid as gold.
9285-3.diff (2.2 KB) - added by technosailor 5 years ago.
Adheres to the WP curly brace style.
get_gmt_offset.patch (3.2 KB) - added by solarissmoke 5 years ago.
Change behaviour of get_date_from_gmt and get_gmt_from_date to use a new function get_gmt_offset
9285-4.2.patch (3.1 KB) - added by solarissmoke 5 years ago.
9285-4.patch (4.0 KB) - added by solarissmoke 5 years ago.
9285-5.diff (4.5 KB) - added by technosailor 5 years ago.
9285-3 with the addition of the deprecation of get_date_from_gmt() in favor of get_gmt_from_date() and eating our own dogfood.

Download all attachments as: .zip

Change History (54)

comment:1 @ryan6 years ago

Which importer[s]? Some importers do their own gmt calculations. Are the dates wrong in the source posts?

@hailin6 years ago

patch

comment:2 @hailin6 years ago

The user exported posts from local WordPress installation, and its timezone is set UTC. Before export, the user scheduled some future posts. After export, the XML file has:
<wp:post_date>2009-04-04 10:00:00<wp:post_date>
<wp:post_date_gmt>2009-04-04 10:00:00<wp:post_date_gmt>
This is correct since his timezone is UTC.

Now, the user imports the XML to his live blog, which has timezone set to: UTC-5.
He checks the scheduled dates and saw "scheduled for 2009-04-04 10:00:00".
Of course, this is misleading because internally, we used post_date_gmt to derive the timestamp. So the post ended up being published 5 hours earlier.

My proposed fix is to create post_date_gmt from post_date, to retain user's perception of date while maintaining correct internal logic, during import.

Another alternative is to enforce this rule in wp_insert_post, but that maybe a little too strict.

comment:3 @hailin6 years ago

Joseph Scott encountered another similar case when post is published via XML-RPC.
post_date was set to 2009-03-05 17:00:00, and post_date_gmt was set to 2009-03-06 03:00:00; and it has UTC-5. So user was expecting post to be published at 2009-03-05 17:00:00 (2009-03-05 22:00:00 GMT), but it won't since cron uses post_date_gmt.

one way to prevent this is to force post_date_gmt = post_date - gmt_offset during wp_insert_post

comment:4 @ryan6 years ago

If we are given an explicit post_date_gmt, that is what we should insert. Maybe we when setup the schedule we can subtract offset from post_date. Regardless, this seems pretty edge case.

@hailin6 years ago

revised patch

comment:5 @hailin6 years ago

Revised the patch to tackle it in _future_post_hook.
Tested with the above cases. Should be a safe patch.

comment:6 @Viper007Bond6 years ago

  • Keywords has-patch needs-testing added

comment:7 @hailin6 years ago

This patch has been running at WordPress.com for 6 weeks without any issue.

comment:8 @Denis-de-Bernardy6 years ago

  • Keywords tested added; needs-testing removed
  • Milestone changed from Unassigned to 2.8
  • Version set to 2.8

comment:9 @Denis-de-Bernardy6 years ago

patch still works against today's trunk

comment:10 @Denis-de-Bernardy6 years ago

  • Keywords commit added

comment:11 @Denis-de-Bernardy6 years ago

  • Component changed from General to Date/Time

comment:12 @Denis-de-Bernardy6 years ago

  • Keywords early added
  • Milestone changed from 2.8 to 2.9

comment:13 @Denis-de-Bernardy6 years ago

patch still applies clean

comment:14 @ryan6 years ago

  • Milestone changed from 2.9 to 3.0

comment:15 follow-up: @westi5 years ago

  • Keywords tested commit removed

I have one issue with the current patch.

We have a window around the two non-DST > DST change over points where we will schedule the cron task for the wrong time with this change.

For example if we are currently not in DST and have a post scheduled for 5pm DST we will schedule for 4pm DST as the current offset would be 1 hour less than when in DST.

This will be caught by check_and_publish_future_post and it will reschedule - but it has the old style calculation there based on post_date_gmt so it will schedule based on that which could still be wrong.

Now if we are currently in DST and have a post scheduled for 5pm non-DST we will schedule for 6pm non-DST as the current offset would be 1 hour more than when not in DST.

This means that a post will publish one hour late.

I'm not sure how much of this we can resolve - do we have access to the DST info for our current timezone when using the PHP5 auto-dst code?

Removing commit and tested until we have an updated tested patch.

comment:16 @westi5 years ago

  • Cc westi added
  • Summary changed from making sure post_date and post_date_gmt are correct to Ensuring scheduled posts publish on time.

Updated title.

comment:17 follow-up: @hakre5 years ago

How are posts sheduled? With or w/o DST? Or is assumed that when the user enters a date for sheduling, that this is with DST-offset and it will be sheduled with the UTC time stored as the publish event-time?

comment:18 in reply to: ↑ 17 @nacin5 years ago

Replying to hakre:

How are posts sheduled? With or w/o DST? Or is assumed that when the user enters a date for sheduling, that this is with DST-offset and it will be sheduled with the UTC time stored as the publish event-time?

I think westi explains this above?

comment:19 @nacin5 years ago

#12610 closed as a duplicate.

comment:20 in reply to: ↑ 15 @technosailor5 years ago

Replying to westi:

I'm not sure how much of this we can resolve - do we have access to the DST info for our current timezone when using the PHP5 auto-dst code?

Mayyyyybe.... http://us.php.net/manual/en/datetime.getoffset.php

comment:21 @technosailor5 years ago

After digging around, I think the obvious answer here is to patch get_gmt_from_date() (which I'm working on) to use the DateTime stuff from PHP5 if supports or fallback on traditional old-style calculations if PHP4. We're already using PHP5 DateTimeZone objects etc for the auto-dst so it makes sense to integrate that here as well and provide ample fallback if needed.

@technosailor5 years ago

Initial proof of concept. This code worked in unit testing outside of WP but fails to calculate DST correctly in WP. Releasing as a POC anyway and will continue to improve.

@technosailor5 years ago

Revision of patch fixes existing problems. Should be solid as gold.

comment:22 @technosailor5 years ago

  • Owner changed from anonymous to technosailor
  • Status changed from new to accepted

@technosailor5 years ago

Adheres to the WP curly brace style.

comment:23 @westi5 years ago

  • Owner changed from technosailor to westi
  • Status changed from accepted to assigned

Going to review this and also see if I can write some unit tests.

Will then commit.

comment:24 @technosailor5 years ago

Tested successfully:

  • 1:59am EDT Nov 7 2010 => GMT=5:59am UTC Nov 7 2010
  • 2:00am EDT Nov 7 2010 => GMT=6:00am UTC Nov 7 2010
  • 2:01am EST Nov 2010 => GMT=7:01am UTC Nov 7 2010
  • Three tests above, same results when Server TZ is different than WP TZ

Need to test

  • Server TZ = Hawaii (GMT-10 under DST, GMT-9 under non-DST)
  • WP TZ = Hawaii (GMT-10 under DST, GMT-9 under non-DST)

comment:25 @nacin5 years ago

Cross referencing #13173.

comment:26 @solarissmoke5 years ago

From the patch in #13173, I propose that a introducing function like this:

/**
 * Get the gmt_offset at a specified time. This is different from using get_option('gmt_offset') 
 * because it lets you specify a time in the past or future (where the daylight savings may be different
 * from the current time. Defaults to the gmt_offset option if timezone support doesn't exist, or if the
 * timestamp argument is omitted.
 *
 * @since
 * @return float|bool
 */
function get_gmt_offset($timestamp = 'now') {
	// if no timestamp is supplied, or if PHP doesn't support fancy timezone handling, just get the option
	if('now' == $timestamp || !wp_timezone_supported() )
		return get_option('gmt_offset');
	
	return wp_timezone_override_offset($timestamp);
}

Would help solve both these issues.

comment:27 @solarissmoke5 years ago

... where wp_timezone_override_offset has been modified to accept timestamps as follows:

function wp_timezone_override_offset($timestamp = 'now') {
	if ( !wp_timezone_supported() ) {
		return false;
	}
	if ( !$timezone_string = get_option( 'timezone_string' ) ) {
		return false;
	}
	
	if('now' == $timestamp) $timestamp = time();

	$timezone_object = timezone_open( $timezone_string );
	$datetime_object = date_create($timestamp);
	if ( false === $timezone_object || false === $datetime_object ) {
		return false;
	}
	return round( timezone_offset_get( $timezone_object, $datetime_object ) / 3600, 2 );
}

@solarissmoke5 years ago

Change behaviour of get_date_from_gmt and get_gmt_from_date to use a new function get_gmt_offset

comment:28 @solarissmoke5 years ago

Proposing that something like this will solve both this ticket and #13173. The core issue for both is that current mechanisms for getting gmt_offset assume that you want the offset for the present moment, and not for a future time.

comment:29 @technosailor5 years ago

Problem is, gmt_offset only exists when the WP timezone is set with *offsets*.... When it's set by Timezone (PHP 5 functionality), then gmt_offset is not set and instead timezone_string is set. The PHP5 functionality is rock solid and tested. The gmt_offset method is older and more widely supported for PHP4, and might could use some of the juice you're suggesting.

comment:30 @solarissmoke5 years ago

We could maybe go one step further and completely replace the use of get_option('gmt_offset') with get_gmt_offset() (and remove/deprecate the pre_option filter that exists at present).

The flow would then be: attempt to use PHP5 native handling first. If it fails, fall back to to the PHP4/database option. But the option becomes a last resort.

Thoughts?

comment:31 follow-up: @technosailor5 years ago

That's what 9285-3.diff does now without the get_gmt_offset() which I think is superfluous.

comment:32 in reply to: ↑ 31 @solarissmoke5 years ago

Replying to technosailor:

That's what 9285-3.diff does now without the get_gmt_offset() which I think is superfluous.

But there are other instances in the core where the same manipulation is required (as in #13173, when a post is created using xmlrpc). The logic behind having a separate function is so that this manipulation isn't duplicated in multiple locations.

Besides, the method in 9285-3.diff is rather long-winded. The timezone_offset_get() function (used in wp_timezone_override_offset) does it all for you when you supply the the time and timezone for which you want an offset - so I would suggest it is better to use what is already there.

comment:33 follow-up: @technosailor5 years ago

Alright fine, although I'm not sure why we can't use get_date_from_gmt() in xmlrpc too?

The patch doesn't work though.

  • Set Blog to Eastern Time
  • Schedule a post for Nov 7, 2010 at 1:00

-- post_date should be 2010-11-07 01:00:00
-- post_date_gmt should be 2010-11-07 05:00:00
-- They are identical

  • Schedule a post for Nov 7, 2010 at 2:00

-- post_date should be 2010-11-07 02:00:00
-- post_date_gmt should be 2010-11-07 07:00:00
-- They are identical

If you want to refresh the patch and make sure it does what it's supposed to, feel free. Let's move quickly though. Otherwise 9285-3.diff actually does work right now and maybe can be improved on in the future.

comment:34 in reply to: ↑ 33 @solarissmoke5 years ago

Replying to technosailor:

Alright fine, although I'm not sure why we can't use get_date_from_gmt() in xmlrpc too?

I think you're probably right - at any rate the manipulation in xmlrpc seems excessive. Lets leave that for the other ticket then.

If you want to refresh the patch and make sure it does what it's supposed to, feel free. Let's move quickly though. Otherwise 9285-3.diff actually does work right now and maybe can be improved on in the future.

Okay, here is a new patch. I've dropped the new function (maybe after the hallowed day when wordpress drops PHP4 support it can be reconsidered). This patch does what yours did but is simpler. It works for me using the tests you gave - could you confirm this?

As an aside, I think get_date_from_gmt() needs to be changed to match get_gmt_from_date() at some point.

@solarissmoke5 years ago

comment:35 @solarissmoke5 years ago

Actually I've changed get_date_from_gmt() as well to make both functions consistent.

Looking closer I think xmlrpc has to use get_date_from_gmt when a user supplies a date_created in GMT - so it then has to work out what the local time will be for scheduling.

I think this should fix #13173 also.

@solarissmoke5 years ago

comment:36 follow-up: @technosailor5 years ago

The problem still exists. Revision times are proper but the post times itself are not. Agreed on combining get_gmt_from_date() and get-date_from_gmt().

We're on code sprint. Going back to 9285-3.diff and combining the two functions. We need to get this in and if you want to mess around with it after, that's fine.

@technosailor5 years ago

9285-3 with the addition of the deprecation of get_date_from_gmt() in favor of get_gmt_from_date() and eating our own dogfood.

comment:37 in reply to: ↑ 36 @solarissmoke5 years ago

Replying to technosailor:

The problem still exists. Revision times are proper but the post times itself are not. Agreed on combining get_gmt_from_date() and get-date_from_gmt().

No, I think you misunderstood me about the two functions. You can't replace get_date_from_gmt() with get_gmt_from_date() - they do different things. If you look at the original functions you see that one subtracts the gmt offset and the other adds it - what I was trying to do was to get them to do so with the datetime functions. They are not identical and cannot be merged.

Since there is a rush, maybe best to stick with 9285-3 which you had tested and work the rest out later.

comment:38 @westi5 years ago

Ok I'm going to go for 9285-3 for now as this has the most testing.

We can revisit removing duplication of code and re-factoring this overall in a future release.

comment:39 @westi5 years ago

I've been struggling for 45 minutes to get the wordpress-tests to run at all for future published posts.

For some reason the cron entries are not getting setup.

As soon as I resolve that I will get this in.

comment:40 @westi5 years ago

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

(In [14487]) Improve cross DST future post publishing behaviour to try and publish at the correct time if we have PHP5 timezone support available to help. Fixes #9285 props technosailor.

comment:41 @westi5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

We're at least missing a strtotime here.

Still can't get the tests to pass though

comment:42 @westi5 years ago

(In [14507]) Restore the strtotime(). See #9285.

comment:43 @westi5 years ago

Added some tests cases in here:

http://svn.automattic.com/wordpress-tests/wp-testcase/test_includes_post.php

Need to check the Timezone being used for a default wordpress-tests run has a DST covered by those two tests though.

comment:44 @ryan5 years ago

Fixed?

comment:45 @westi5 years ago

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

Re-ran the tests to make sure and I am happy with this.

Note: See TracTickets for help on using tickets.