WordPress.org

Make WordPress Core

Opened 23 months ago

Last modified 6 months ago

#25365 reopened defect (bug)

timeAgo attribute for revisions produces the wrong result due to missing UTC in strtotime

Reported by: biranit Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.6.1
Component: Revisions Keywords: has-patch dev-feedback
Focuses: Cc:

Description

When looking at revisions, the date for each revision appears correctly, but the human_time_diff is wrong.

For example, on a server with timezone UTC+3, the time is now 13:28 (UTC+3), and looking a revision from 20 Sep 09:41 (UTC+3) shows:

7 hours ago (20 Sep @ 9:41)

Where in fact it should be

4 hours ago (20 Sep @ 9:41)

The post_date for this revision is: 2013-09-20 09:41:08
The post_date_gmt for this revision is: 2013-09-20 06:41:08

(The server timezone is UTC+3, as is the Wordpress option and the php.ini)

The reason for this bug is in wp-admin/includes/revision.php line 117:

$modified_gmt = strtotime( $revision->post_modified_gmt );

strtotime uses the server's timezone and therefore assumes the strong $revision->post_modified_gmt is actually in UTC+3 (the server's timezone). To avoid this and ensure the unix timestap is indeed GMT, the line should change to:

$modified_gmt = strtotime( $revision->post_modified_gmt . " UTC");

Thank you,

Biranit

Attachments (3)

25365.diff (1.4 KB) - added by adamsilverstein 12 months ago.
enforce UTC in strtotime calls on gmt values in revisions
25365.2.diff (574 bytes) - added by adamsilverstein 12 months ago.
only revisions this time
25365.3.diff (575 bytes) - added by adamsilverstein 6 months ago.
refresh against trunk

Download all attachments as: .zip

Change History (23)

comment:1 @SergeyBiryukov23 months ago

  • Component changed from Date/Time to Revisions

comment:2 follow-up: @biranit23 months ago

I went through ALL instances where strtotime is used in Wordpress 3.6.1 and found two - possibly three - instances where this error should also be fixed:

1) In /wp-includes/comment.php line 2047:

if ( time() - strtotime( $posts[0]->post_date_gmt ) > ( $days_old * DAY_IN_SECONDS ) ) {

should be changed to

if ( time() - strtotime( $posts[0]->post_date_gmt . " UTC" ) > ( $days_old * DAY_IN_SECONDS ) ) {

2) In /wp-includes/media.php lines 1661-1662:

'date'        => strtotime( $attachment->post_date_gmt ) * 1000,
'modified'    => strtotime( $attachment->post_modified_gmt ) * 1000,

Should be changed to

'date'        => strtotime( $attachment->post_date_gmt . " UTC") * 1000,
'modified'    => strtotime( $attachment->post_modified_gmt . " UTC") * 1000,

2) In /wp-content/plugins/akismet/akismet.php line 385:

$last_updated = strtotime( $post->post_modified_gmt );

Should be changed to

$last_updated = strtotime( $post->post_modified_gmt . " UTC");

Thank you

comment:3 @biranit23 months ago

  • Summary changed from timeAgo attribute for revisions produces the wrong result due to misuse of strtotime to timeAgo attribute for revisions produces the wrong result due to missing UTC in strtotime

comment:4 @nacin22 months ago

We specifically do date_default_timezone_set( 'UTC' ); in wp-settings.php, which means I'm pretty sure strtotime() should never be GMT+3.

comment:5 in reply to: ↑ 2 @adamsilverstein17 months ago

  • Resolution set to worksforme
  • Status changed from new to closed

Based on @nacin's comment, I'm guessing you were experiencing some sort of server configuration issue - possibly the server time was not set correctly? In all of our testing of this feature we never saw the time difference issue you mention.

I'm going to close this ticket for now because I am unable to reproduce the problem, and it looks like a configuration error. If you are still seeing the issue, feel free to re-open the ticket and please provide some more details - steps to reproduce the issue and screenshots would be helpful.

Thanks for the report, happy to work on fixing this if I can reproduce it.

Replying to biranit:

I went through ALL instances where strtotime is used in Wordpress 3.6.1 and found two - possibly three - instances where this error should also be fixed:

1) In /wp-includes/comment.php line 2047:

if ( time() - strtotime( $posts[0]->post_date_gmt ) > ( $days_old * DAY_IN_SECONDS ) ) {

should be changed to

if ( time() - strtotime( $posts[0]->post_date_gmt . " UTC" ) > ( $days_old * DAY_IN_SECONDS ) ) {

2) In /wp-includes/media.php lines 1661-1662:

'date'        => strtotime( $attachment->post_date_gmt ) * 1000,
'modified'    => strtotime( $attachment->post_modified_gmt ) * 1000,

Should be changed to

'date'        => strtotime( $attachment->post_date_gmt . " UTC") * 1000,
'modified'    => strtotime( $attachment->post_modified_gmt . " UTC") * 1000,

2) In /wp-content/plugins/akismet/akismet.php line 385:

$last_updated = strtotime( $post->post_modified_gmt );

Should be changed to

$last_updated = strtotime( $post->post_modified_gmt . " UTC");

Thank you

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

comment:6 @SergeyBiryukov16 months ago

  • Milestone Awaiting Review deleted

comment:7 @buley13 months ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

This issue seems unduly dismissed.

https://cloud.githubusercontent.com/assets/86985/3389378/46bb2ddc-fc94-11e3-8979-f95d84377aa5.png

https://cloud.githubusercontent.com/assets/86985/3389354/e0d0b97e-fc93-11e3-9828-e9e906f95f6f.png

In the core code above and on trac https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/revision.php#L172 WordPress is shown to use $modified to calculate the pretty date (dateShort) but $modified_gmt to show the human_time_diff() (timeAgo). This causes apparent time travel.

I'm guessing you were experiencing some sort of server configuration issue

This does not seem to be possible to me given the equivalence between the dates, but I'm interested in hearing how my servers might be misconfigured. Looking for answers here because I see no other way to filter these values, as they are templated on the client side.

comment:8 follow-up: @Biranit13 months ago

I think in my case, the issue is down to the fact that our theme's function file has the line:

date_default_timezone_set('Asia/Jerusalem');

For us, timezone functions are crucial. Everything on the server is set up for this timezone -- including php.ini, OS timezone + clock AND the hardware clock.

Yet the revisions are all off, as they appear in UTC+0 instead of UTC+3 (Asia/Jerusalem).

Personally, I still think this is a bug - or just an oversight - that should be fixed...

Last edited 13 months ago by Biranit (previous) (diff)

comment:9 @buley13 months ago

I can confirm for you that your date_default_timezone_set('Asia/Jerusalem'); is in fact the configuration "issue" that causes this behavior. For me of course it's my own America/New_York, needed for a 3rd party Google Analytics library on which we rely.

In a setup where WordPress is a component in a larger system, using date_default_timezone_set() is commonplace. In fact if you don't set this you'll get the "It is not safe to rely on the system's timezone settings" warning telling you that you should use this, or "the date.timezone setting" which I presume will have a similar effect.

I've found no filters or other way around this that doesn't require core modification or client-side hackery. A proposed "fix" is can be as simple as using $modified for the human_time_diff().

Removing the date_default_timezone_set() call seems necessary but that's unacceptable in my configuration, and unfortunately the difference is somewhat undermining our user confidence.

Last edited 13 months ago by buley (previous) (diff)

comment:10 in reply to: ↑ 8 @adamsilverstein13 months ago

Biranit - thanks for the feedback.

In order to simplify timezone issues, WordPress always keeps track of time internally using UTC. Then, when the time is displayed it's converted the the local time that's defined in the Timezone setting on the General Settings page. The proper place to set your timezone within WordPress is under Settings->General (see http://cl.ly/image/3O3N170m2C3R).

Can you try setting your timezone there and commenting out out or deleting the date_default_timezone_set call in your functions.php file? When I tested this locally, the revision time_ago called returned the proper value.

The UTC/timezone issue has a long history that relates in part to legacy support of PHP4 - see Otto's comments here - http://wordpress.org/support/topic/why-does-wordpress-set-timezone-to-utc?replies=5#post-3749681

There is a long term effort to improve timezone handling in WordPress - if you are interested in helping out with this effort, take a look at these related tickets: #28636, #25555, #20973, #23384

Replying to Biranit:

I think in my case, the issue is down to the fact that our theme's function file has the line:

date_default_timezone_set('Asia/Jerusalem');

For us, timezone functions are crucial. Everything on the server is set up for this timezone -- including php.ini, OS timezone + clock AND the hardware clock.

Yet the revisions are all off, as they appear in UTC+0 instead of UTC+3 (Asia/Jerusalem).

Personally, I still think this is a bug - or just an oversight - that should be fixed...

comment:11 follow-up: @buley13 months ago

I can confirm that in my case timezone is set correctly in WordPress under Settings->General. It's also set correctly in PHP using date_default_timezone_set().

To repeat, the problem is not that WordPress uses UTC to represent dates internally. The problem is that in one single instance feeds human_time_diff() a UTC date for external display of a localized time.

Last edited 13 months ago by buley (previous) (diff)

comment:12 in reply to: ↑ 11 @adamsilverstein13 months ago

Thanks for the feedback -

When using WordPress the timezone should not be set in PHP. WordPress explicitly sets the timezone to UTC in wp-settings.php and calculates all offsets from there - see here -- setting the timezone elsewhere in PHP is confusing WordPress internally.

I tried changing the code from using modified_gmt to modified as suggested above and that does fix the revisions page if I set the timezone in PHP - otherwise the times are all off. Since setting the timezone in PHP is against recommended practice, making this change would likely have a negative affect for everyone who doesn't have this set.

Where and why are you setting the timezone in your PHP with date_default_timezone_set? can you try removing that set statement to see if the issue is resolved? Setting the timezone in WordPress should be sufficient.

If you absolutely have to set the timezone for a block of PHP, try setting it back to UTC at the end of the block, so WordPress internal calculations are not affected.

Hope that helps.

Replying to buley:

I can confirm that in my case timezone is set correctly in WordPress under Settings->General. It's also set correctly in PHP using date_default_timezone_set().

To repeat, the problem is not that WordPress uses UTC to represent dates internally. The problem is that in one single instance feeds human_time_diff() a UTC date for external display of a localized time.

comment:13 @SergeyBiryukov12 months ago

  • Milestone set to Awaiting Review

comment:14 @Biranit12 months ago

I'm really sorry I've not replied before -- it's been a little difficult around here, recently...

I just wanted to say: it's not realistic to prevent PHP settings of timezone. In fact, on a PHP install without setting timezone, PHP now issues a warning. So most sysadmins will likely set up a timezone.

Furthermore, on shared servers, the local directive for PHP timezone is very important: it affects error log files, for example; and other non-WordPress scripts.

WordPress should be self-contained, in my opinion, and this is really a very simple thing to resolve to ensure there is no discrepancy in times. Fact is, this issue does NOT occur anywhere other than revisions. So why not just solve this simply by adding that . " UTC" bit to those functions?

Many thanks,

Bira

comment:15 @buley12 months ago

Those are my exact feelings as well, Bira. And I also apologize for my delay in response.

We use WordPress as the heart of a larger PHP-based platform. It seems like a shame to "force" what seems like WordPress-specific need on the rest of our codebase -- especially since we're talking about a single parameter on a single function. But, following adamsilveerstin's very kind advice, that's what we had to do in order to prevent our revision dates from time-traveling because we are unwilling to hack core even for a revision time.

Last edited 12 months ago by buley (previous) (diff)

@adamsilverstein12 months ago

enforce UTC in strtotime calls on gmt values in revisions

comment:16 @adamsilverstein12 months ago

  • Keywords has-patch dev-feedback added

25365.diff includes the suggested fixes from Bira. Bira, can you see if this patch resolves your issue?

I only applied the UTC fix to the revisions code so we stay within the scope of this ticket; The issue deserves more attention in a new ticket: scanning core I see several other uses of strtotime on gmt values, some with no appended timezone, some with ' GMT', some with '+0000'

@adamsilverstein12 months ago

only revisions this time

comment:17 @adamsilverstein12 months ago

correction, 25365.2.diff is limited to revisions

comment:18 follow-up: @Biranit12 months ago

Works perfect, adamsilverstein :)

Many thanks!

comment:19 in reply to: ↑ 18 @adamsilverstein12 months ago

You are welcome. I will defer to the lead developers to decide if this is a fix that belongs in core.

Thanks for testing and raising the issue!

Replying to Biranit:

Works perfect, adamsilverstein :)

Many thanks!

@adamsilverstein6 months ago

refresh against trunk

comment:20 @adamsilverstein6 months ago

In 25365.3.diff: refresh against trunk.

Note: See TracTickets for help on using tickets.