Ensuring scheduled posts publish on time.

[hailin]

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.

[ryan]

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

[hailin]

patch

[hailin]

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.

[hailin]

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

[ryan]

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.

[hailin]

revised patch

[hailin]

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

[hailin]

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

[Denis-de-Bernardy]

patch still works against today's trunk

[Denis-de-Bernardy]

patch still applies clean

[westi]

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.

[westi]

Updated title.

[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?

[nacin]

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?

[nacin]

#12610 closed as a duplicate.

[technosailor]

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

[technosailor]

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.

[technosailor]

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.

[technosailor]

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

[technosailor]

Adheres to the WP curly brace style.

[westi]

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

Will then commit.

[technosailor]

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)

[nacin]

Cross referencing #13173.

[solarissmoke]

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.

[solarissmoke]

... 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 );
}

[solarissmoke]

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

[solarissmoke]

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.

[technosailor]

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.

[solarissmoke]

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?

[technosailor]

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

[solarissmoke]

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.

[technosailor]

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.

[solarissmoke]

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.

[solarissmoke]

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.

[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().

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.

[technosailor]

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.

[solarissmoke]

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.

[westi]

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.

[westi]

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.

[westi]

(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.

[westi]

We're at least missing a strtotime here.

Still can't get the tests to pass though

[westi]

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

[westi]

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.

[ryan]

Fixed?

[westi]

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