Make WordPress Core

Opened 13 years ago

Closed 11 years ago

#20328 closed defect (bug) (fixed)

get_date_from_gmt assumes current gmt_offset is appropriate

Reported by: scottconnerly's profile scottconnerly Owned by: nacin's profile nacin
Milestone: 3.6 Priority: normal
Severity: normal Version:
Component: Date/Time Keywords: has-patch
Focuses: Cc:

Description

For locations that have daylight savings time, the gmt_offset changes over time. Yet get_date_from_gmt assumes the offset that is in effect now is always the right offset. But I might be trying to display date/times that are in a different timezone transition than the current one.

Proposed fix is to use timezone_transitions_get() as wp-admin/options-general.php does in order to translate those date/times accurately.

Attachments (2)

date_fixes.diff (4.0 KB) - added by scholesmafia 12 years ago.
Updated diff based on upstream changes
ConvertDateTest.diff (1.7 KB) - added by scholesmafia 12 years ago.
New test case for respecting DST

Download all attachments as: .zip

Change History (25)

#1 @nacin
13 years ago

This already happens deeper in the code.

If the option timezone_string is set, then we use it to set the GMT offset, which means it is DST-aware.

Take a look at wp_timezone_override_offset() which overrides get_option('gmt_offset') by hooking into pre_option_gmt_offset.

#2 @solarissmoke
13 years ago

  • Keywords close added

#3 @nacin
13 years ago

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

#4 @scholesmafia
13 years ago

  • Keywords 2nd-opinion added; close removed
  • Resolution invalid deleted
  • Status changed from closed to reopened

Related: #20398

I'm not sure the the bug is addressed with this hook. This still uses an offset which is computed based on the current date, and that offset is not valid for all dates in that timezone.

Example: Timezone is Europe/London. Date is 2012-04-09, so we're in DST. gmt_offset should therefore be 1. Now I want to find the local time of the UTC 2012-01-01 12:00:00 using my timezone. The low-level code and the hook both add 1 hour to that time, which is incorrect.

#5 @SergeyBiryukov
13 years ago

  • Milestone set to Awaiting Review

#6 @scholesmafia
13 years ago

Attached a patch which unifies the behaviour of get_gmt_from_date and get_date_from_gmt.

#7 @SergeyBiryukov
13 years ago

  • Keywords has-patch added

#8 @bradt
12 years ago

  • Cc brad@… added

#9 @axelseaa
12 years ago

  • Cc aaron@… added

fwiw I can confirm that the patch for get_date_from_gmt works - as I am having this same issue with a plugin and will be implementing this manually for now.

#10 @nacin
12 years ago

  • Keywords needs-unit-tests added; 2nd-opinion removed

Still not sure there is a bug here. Needs a test case.

@scholesmafia
12 years ago

Updated diff based on upstream changes

@scholesmafia
12 years ago

New test case for respecting DST

#11 @scholesmafia
12 years ago

  • Keywords needs-unit-tests removed

Test case added.

The original phpdoc for the function hints at the bug: "Simply adds the value of gmt_offset." This is not sufficient to be DST aware. The point of DST is that gmt_offset changes throughout the year, so simply adding the offset to any date will be incorrect for ~50% of dates.

The dual function get_gmt_from_date uses the timezone_string option and the DateTime class to properly respect the timezone, as alluded to in #20398. This is why the second test test_dst_respected_getting_gmt_date passes.

The patch to get_date_from_gmt adds the same functionality, so that the timezone_string is used to convert the GMT date into a date local to that timezone, which respects DST where relevant. The patch only alters get_gmt_from_date to unify the logic between both functions and use DateTime properly.

Note that both functions fall back to the naïve approach of adding or subtracting gmt_offset where the timezone_string option is not available.

Version 1, edited 12 years ago by scholesmafia (previous) (next) (diff)

#12 @nacin
12 years ago

In 1233/tests:

Add tests for get_date_from_gmt() and get_gmt_from_date().

These tests demonstrate that get_date_from_gmt() is ignorant of DST settings.

props scholesmafia.
see #20328.

#13 @nacin
12 years ago

Thank you! I have committed those tests.

So, clearly any usage of get_option( 'gmt_offset' ) is improper except when dealing with the current time. I can't say we currently use it often in core, however, so that's good.

#14 @nacin
12 years ago

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

In 23618:

Properly handle timezones in get_date_from_gmt() rather than relying on the implicit gmt_offset. This offset is only good for the current time, rather than the passed time, which causes problems when converting a DST date when DST is not in effect, or vice versa.

Update get_gmt_from_date() to make these functions match in formatting, as they are complementary and just reverse a few operations.

props scholesmafia
Tests: [1233/tests]

fixes #20328.

#15 @nacin
12 years ago

  • Milestone changed from Awaiting Review to 3.6

#16 @nacin
12 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

iso8601_to_datetime() and wp-mail.php both appear to be affected. We are already parsing out the date/time pieces using regex in both of those cases, so we should be able to just re-construct a date and pass it directly to get_date_from_gmt/get_gmt_from_date.

_walk_bookmarks() is also affected, but that is not a big deal.

#17 @alexkingorg
12 years ago

  • Cc public@… added

#18 @ryan
12 years ago

  • Milestone changed from 3.6 to Future Release

#19 @mbijon
11 years ago

  • Cc mike@… added

#20 @nacin
11 years ago

  • Milestone changed from Future Release to 3.6

Moving back to 3.6 as this had a commit that nearly fixed it entirely.

#21 @nacin
11 years ago

  • Keywords 3.7-early added
  • Milestone changed from 3.6 to Future Release

Moving to Future for final review and tweaking.

#22 @wonderboymusic
11 years ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

#23 @nacin
11 years ago

  • Keywords 3.7-early removed
  • Milestone changed from 3.7 to 3.6
  • Resolution set to fixed
  • Status changed from reopened to closed

See #25399 for iso8601_to_datetime() and wp-mail.

Note: See TracTickets for help on using tickets.