WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 7 months 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:

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

Download all attachments as: .zip

Change History (25)

comment:1 nacin2 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.

comment:2 solarissmoke2 years ago

  • Keywords close added

comment:3 nacin2 years ago

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

comment:4 scholesmafia2 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.

comment:5 SergeyBiryukov2 years ago

  • Milestone set to Awaiting Review

comment:6 scholesmafia2 years ago

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

comment:7 SergeyBiryukov2 years ago

  • Keywords has-patch added

comment:8 bradt18 months ago

  • Cc brad@… added

comment:9 axelseaa14 months 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.

comment:10 nacin14 months ago

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

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

scholesmafia14 months ago

Updated diff based on upstream changes

scholesmafia14 months ago

New test case for respecting DST

comment:11 scholesmafia14 months 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 logic in this function is rather nasty and roundabout, however, so the attached patch simplifies it.

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.

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.

Last edited 14 months ago by scholesmafia (previous) (diff)

comment:12 nacin14 months 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.

comment:13 nacin14 months 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.

comment:14 nacin14 months 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.

comment:15 nacin14 months ago

  • Milestone changed from Awaiting Review to 3.6

comment:16 nacin14 months 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.

comment:17 alexkingorg14 months ago

  • Cc public@… added

comment:18 ryan11 months ago

  • Milestone changed from 3.6 to Future Release

comment:19 mbijon11 months ago

  • Cc mike@… added

comment:20 nacin11 months ago

  • Milestone changed from Future Release to 3.6

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

comment:21 nacin10 months ago

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

Moving to Future for final review and tweaking.

comment:22 wonderboymusic9 months ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

comment:23 nacin7 months 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.