Make WordPress Core

Opened 4 years ago

Closed 2 years ago

#20328 closed defect (bug) (fixed)

get_date_from_gmt assumes current gmt_offset is appropriate

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


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 3 years ago.
Updated diff based on upstream changes
ConvertDateTest.diff (1.7 KB) - added by scholesmafia 3 years ago.
New test case for respecting DST

Download all attachments as: .zip

Change History (25)

#1 @nacin
4 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
4 years ago

  • Keywords close added

#3 @nacin
4 years ago

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

#4 @scholesmafia
4 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
4 years ago

  • Milestone set to Awaiting Review

#6 @scholesmafia
4 years ago

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

#7 @SergeyBiryukov
4 years ago

  • Keywords has-patch added

#8 @bradt
3 years ago

  • Cc brad@… added

#9 @axelseaa
3 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
3 years ago

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

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

3 years ago

Updated diff based on upstream changes

3 years ago

New test case for respecting DST

#11 @scholesmafia
3 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 3 years ago by scholesmafia (previous) (next) (diff)

#12 @nacin
3 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
3 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
3 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
3 years ago

  • Milestone changed from Awaiting Review to 3.6

#16 @nacin
3 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
3 years ago

  • Cc public@… added

#18 @ryan
3 years ago

  • Milestone changed from 3.6 to Future Release

#19 @mbijon
3 years ago

  • Cc mike@… added

#20 @nacin
3 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
3 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
3 years ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

#23 @nacin
2 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.