Opened 5 years ago
Closed 5 years ago
#48384 closed defect (bug) (fixed)
get_post_time returns wrong value after timezone switch
Reported by: | david.binda | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | 5.3 | Priority: | normal |
Severity: | normal | Version: | 5.3 |
Component: | Date/Time | Keywords: | has-patch has-unit-tests |
Focuses: | Cc: |
Description
When testing the r46154 , I've run into a regression, when the returned time from get_post_time
is incorrect after a timezone setting swith.
I know that the timezone is not expected to be changed often, but it still may happen.
In case a post is created with a timezone set, for instance, to UTC+0, and then later changed to a different one, for instance UTC+2, the get_post_time
function with the $gmt
param set to true
, i.e. expecting UTC+0, would return an original post time with the new offset added/substracted (expected behaviour would be the same time, as the post was created under the UTC+0 timezone).
The issue is visible on post listing in wp-admin/edit.php, where post created during the UTC+0 timezone being set, are marked as 2 hours ago, after changing the timezone to UTC+2.
I've created a phpunit test demostrating the issue. The test is passing in 5.2.4, and even on 5.3 RC1 after reverting the r46154, but not when r46154 is being applied.
Checking the updated code of get_post_time
, get_post_modified_time
and of a newly added get_post_datetime
, as well as get_post_timestamp
, it feels like those functions should default to work with the gmt version of the date (stored in post_date_gmt/post_modified_gmt) and should recalculate the GMT datetime to currently set timezone, rather than changing the post_date
to UTC, as the code does not know, what timezone was used for storing the data.
Attachments (4)
Change History (36)
#2
in reply to:
↑ description
@
5 years ago
Replying to david.binda:
The issue is visible on post listing in wp-admin/edit.php, where post created during the UTC+0 timezone being set, are marked as 2 hours ago, after changing the timezone to UTC+2.
I might be missing something, but this is the behavior I would expect here, rather than what 5.2.4 did.
#3
@
5 years ago
I might be missing something, but this is the behavior I would expect here, rather than what 5.2.4 did.
No, I think the report is correct about that being wrong. The relative time passed is irrelevant to time zone. If post was created a minute ago then that was a minute ago regardless of time zone. I don't know if it related to UTC time though, I'll look through things on fresh head.
I think what actually regressed is explicitly retrieving UTC time is now slipping (as per unit test). In the past the local time slipped with time zone changed. Right now I think both are slipping, because we instantiate from local time.
I'll probably add an argument to get_post_datetime()
do we want to instantiate from local or UTC time. It's kind of stupid because there should be no difference if it wasn't broken, but we'll be able to stop UTC from slipping at least (so fix regression, if not underlying issue).
#4
@
5 years ago
- Keywords has-patch has-unit-tests added
Added parameter to instance get_post_datetime()
from local or UTC time and explanation in inline documentation.
Fixed up legacy logic paths from UTC time slipping on time zone change.
This ticket was mentioned in Slack in #core-datetime by rarst. View the logs.
5 years ago
#6
@
5 years ago
- Keywords commit dev-feedback added
- Milestone changed from Awaiting Review to 5.3
Thanks for the patch! Marking for a second committer's review.
This ticket was mentioned in Slack in #core by azaozz. View the logs.
5 years ago
#8
@
5 years ago
In 48384.2.diff:
- Include the unit test from 48384.diff (slightly adapted for consistency).
- Document the default values for
$field
and$source
parameters ofget_post_datetime()
. - Synchronize the
$field
parameter description forget_post_datetime()
andget_post_timestamp()
.
#9
@
5 years ago
I think testing gmt_offset
case is redundant. There is no functional difference between time zone modes for the issue.
#11
@
5 years ago
Actually, scratch that. Better to get to the bottom of the "human time diff" on the Posts screen not working well with the patch. Then add this for RC3.
#12
follow-up:
↓ 13
@
5 years ago
Better to get to the bottom of the "human time diff" on the Posts screen not working well with the patch.
Could you elaborate what is still the issue?
The regression demonstrated by suggested test is fixed by patch.
#13
in reply to:
↑ 12
;
follow-up:
↓ 14
@
5 years ago
Replying to Rarst:
Better to get to the bottom of the "human time diff" on the Posts screen not working well with the patch.
Could you elaborate what is still the issue?
Sure :) The unit tests pass, but we were testing the scenario from the ticket description:
The issue is visible on post listing in wp-admin/edit.php, where post created during the UTC+0 timezone being set, are marked as 2 hours ago, after changing the timezone to UTC+2.
and noticed some weirdness (Slack thread for reference).
My steps:
- Set the timezone to UTC+0.
- Create a draft post.
- Set the timezone to UTC+2.
- View the Date column in the Posts list.
Without the patch, the Date column shows:
Last Modified 6 mins ago
before the timezone change.Last Modified 2 hours ago
after the timezone change.
With the patch, the Date column shows:
Last Modified 2019/10/23
both before and after the timezone change.
I've noticed that get_post_time() returns false there, and the output defaults to mysql2date( __( 'Y/m/d' ), $m_time )
, instead of human_time_diff( $time )
, but haven't yet tracked it down further.
#14
in reply to:
↑ 13
@
5 years ago
Replying to SergeyBiryukov:
I've noticed that get_post_time() returns false there, and the output defaults to
mysql2date( __( 'Y/m/d' ), $m_time )
, instead ofhuman_time_diff( $time )
, but haven't yet tracked it down further.
Ah, it's because for drafts post_date
is the creation date, but with the patch we're now looking at post_date_gmt
, which is 0000-00-00 00:00:00
, which in turn causes get_post_datetime()
to return false.
#15
@
5 years ago
Ok, so there are two issues:
- GMT slipping with time zone change, fixed by standing patch
column_date()
logic behaving differently becauseget_post_time()
failure mode changed in new code from internal implementation changes
Looking at 5.2 get_post_time()
actually returns gibberish negative number value for drafts (probably a negative timestamp for "year zero" time or something like that). So anything producing meaningful result there is pretty much implementation accident, the logic is long broken.
So I suggest:
- We proceed with the standing patch, since it fixes clear testable regression (and unfortunately forces our hand to have to choose how to instantiate objects for backwards compat)
- Stuff a check for valid
get_post_time()
return in thecolumn_date()
and skip the relative time branch, since we can't calculate it with bogus input
#16
@
5 years ago
As an option we could use the new get_post_timestamp()
there, but then we get time slipping with time zone change again because it uses local time as basis right now, not GMT one. Local time is slipping everywhere anyway...
This ticket was mentioned in Slack in #core by azaozz. View the logs.
5 years ago
#18
@
5 years ago
- Milestone changed from 5.3 to 5.3.1
Seems this would need a context, to be aware of the post type, draft vs. published, or perhaps even better to handle both cases with and without GMT date in the db. May also affect some CPTs depending on their settings. Perhaps a wrapper function with some logic along these lines?
Thinking this would be better handled for 5.3.1 with enough time to perhaps explore different approaches.
#19
@
5 years ago
Seems this would need a context, to be aware of the post type, draft vs. published
This logic absolutely does not belong in date retrieval. The purpose of this branch of functions is to take a database value and turn it into workable representation.
Treating presence or absence or magical values of dates is sprinkled all through the core and would take another year to fix.
The new API didn't break anything here, it just highlights things that had always been broken by bringing clear implementations and meaningful error handling into the old messed up code.
We need new API to (gradually) fix the mess, not the other way around.
#20
@
5 years ago
I'd also like to point out that holding up the patch from 5.3 means that effectively get_post_datetime()
will not ship with that signature in it and then get the signature change in a patch immediately, which would be very unwanted from developer experience perspective.
#21
@
5 years ago
Added patch to adjust column date implementation. Went with get_post_timestamp()
since it's most semantically fitting for the task, though it will still suffer from time zone slipping on change, which is waaay too large of a problem to address here).
Effectively there are two standing patches for two issues:
- Enabling
get_post_datetime()
to replicate legacy behavior of instancing from local or UTC time in database - Fixing handling
column_date()
logic
Local time changing with time zone change is a larger major issue in core and there is the #38774 ticket for it.
#22
@
5 years ago
- Milestone changed from 5.3.1 to 5.3
Confirmed that 48384-adjusted-column-date-implementation.patch fixes the issue mentioned in comment:13 and preserves the current behavior with 48384-fixed-utc-time-slipping-on-time-zone-change.patch applied.
Let's get this in for RC3 to fix the get_post_time()
regression.
#24
@
5 years ago
- Keywords dev-reviewed added; dev-feedback removed
- Owner set to SergeyBiryukov
- Status changed from new to accepted
#28
@
5 years ago
- Keywords commit dev-reviewed removed
- Resolution fixed deleted
- Status changed from closed to reopened
The unit test fails on PHP 5.6, reopening for investigation.
#29
follow-up:
↓ 30
@
5 years ago
From a quick look (hadn't run yet) it's likely that you need to explicitly delete timezone_string
if you want to test gmt_offset
, otherwise current value of timezone_string
will always override whatever you set gmt_offset
to.
#30
in reply to:
↑ 29
@
5 years ago
Replying to Rarst:
From a quick look (hadn't run yet) it's likely that you need to explicitly delete
timezone_string
if you want to testgmt_offset
, otherwise current value oftimezone_string
will always override whatever you setgmt_offset
to.
Thanks! I thought I removed that test as per comment:9, but apparently confused my checkouts :)
The time zone change is messed up altogether, see #38774 for general issue.
I'll take a look at GMT regression, but essentially at the moment we are just tweaking how it's broken since we can't fix it.
I prioritized UTC time in my external lib, but went with local time for base in core since that is existing behavior and there are weird logic bits in some places that depend on UTC time being or not being set in database.
But basically if you change WP time zone you break time on the site. :\