Make WordPress Core

Opened 11 years ago

Closed 6 years ago

#25365 closed defect (bug) (fixed)

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

Reported by: biranit's profile biranit Owned by: adamsilverstein's profile adamsilverstein
Milestone: 4.9 Priority: normal
Severity: normal Version: 3.6.1
Component: Revisions Keywords: has-patch commit
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 (5)

25365.diff (1.4 KB) - added by adamsilverstein 10 years ago.
enforce UTC in strtotime calls on gmt values in revisions
25365.2.diff (574 bytes) - added by adamsilverstein 10 years ago.
only revisions this time
25365.3.diff (575 bytes) - added by adamsilverstein 9 years ago.
refresh against trunk
25365.4.diff (575 bytes) - added by adamsilverstein 8 years ago.
25365.5.diff (633 bytes) - added by adamsilverstein 6 years ago.

Download all attachments as: .zip

Change History (32)

#1 @SergeyBiryukov
11 years ago

  • Component changed from Date/Time to Revisions

#2 follow-up: @biranit
11 years 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

#3 @biranit
11 years 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

#4 @nacin
10 years 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.

#5 in reply to: ↑ 2 @adamsilverstein
10 years 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 10 years ago by SergeyBiryukov (previous) (diff)

#6 @SergeyBiryukov
10 years ago

  • Milestone Awaiting Review deleted

#7 @buley
10 years 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.

#8 follow-up: @Biranit
10 years 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 10 years ago by Biranit (previous) (diff)

#9 @buley
10 years 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 10 years ago by buley (previous) (diff)

#10 in reply to: ↑ 8 @adamsilverstein
10 years 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...

#11 follow-up: @buley
10 years 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 10 years ago by buley (previous) (diff)

#12 in reply to: ↑ 11 @adamsilverstein
10 years 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.

#13 @SergeyBiryukov
10 years ago

  • Milestone set to Awaiting Review

#14 @Biranit
10 years 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

#15 @buley
10 years 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 10 years ago by buley (previous) (diff)

@adamsilverstein
10 years ago

enforce UTC in strtotime calls on gmt values in revisions

#16 @adamsilverstein
10 years 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'

@adamsilverstein
10 years ago

only revisions this time

#17 @adamsilverstein
10 years ago

correction, 25365.2.diff is limited to revisions

#18 follow-up: @Biranit
10 years ago

Works perfect, adamsilverstein :)

Many thanks!

#19 in reply to: ↑ 18 @adamsilverstein
10 years 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!

@adamsilverstein
9 years ago

refresh against trunk

#20 @adamsilverstein
9 years ago

In 25365.3.diff: refresh against trunk.

#21 @adamsilverstein
8 years ago

  • Owner set to adamsilverstein
  • Status changed from reopened to assigned

25365.4.diff refresh against trunk

#22 @adamsilverstein
8 years ago

#36829 was marked as a duplicate.

#23 follow-up: @Biranit
7 years ago

I would like to revive this ticket. On a completely clean install, with no intervention in WordPress's timezone management, the revisions continue to show the wrong time (adding 3 hours to them).

Time on server is UTC+3
Timezone set in WordPress options is Jerusalem.

No other time is set in functions.php or elsehwhere.

And yet a revision from 15 minutes ago displays as being from 3 hours and 15 minutes ago; a revision from 1 hour ago shows as being from 4 hours ago, etc.

Can't this be fixed? it's such a simple fix and it's been this way for four years now.

#24 in reply to: ↑ 23 @adamsilverstein
7 years ago

  • Milestone changed from Awaiting Review to 4.9

Replying to Biranit:

Can't this be fixed? it's such a simple fix and it's been this way for four years now.

Let's do it!

#25 @adamsilverstein
6 years ago

  • Keywords commit added

Checking in core, there are several places where we handle gmt dates and add ' +0000'. 25365.5.diff switches to this app[roach, which should be the same as using UTC.

#26 @adamsilverstein
6 years ago

  • Keywords dev-feedback removed

#27 @adamsilverstein
6 years ago

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

In 41559:

Revisions: correct a timezone display issue.

When preparing the data for the revisions screen, add ' +0000' to the gmt date string before passing it thru strtotime.

Props biranit, nacin, buley.
Fixes #25365.

Note: See TracTickets for help on using tickets.