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 | 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)
Change History (25)
#3
@
13 years ago
- Milestone Awaiting Review deleted
- Resolution set to invalid
- Status changed from new to closed
#4
@
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.
#6
@
13 years ago
Attached a patch which unifies the behaviour of get_gmt_from_date and get_date_from_gmt.
#9
@
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
@
12 years ago
- Keywords needs-unit-tests added; 2nd-opinion removed
Still not sure there is a bug here. Needs a test case.
#11
@
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 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.
#12
@
12 years ago
In 1233/tests:
#13
@
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
@
12 years ago
- Owner set to nacin
- Resolution set to fixed
- Status changed from reopened to closed
In 23618:
#16
@
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.
#20
@
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.
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.