get_post_time returns wrong value after timezone switch

[david.binda]

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.

[Rarst]

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. :\

[SergeyBiryukov]

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.

[Rarst]

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).

[Rarst]

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.

[SergeyBiryukov]

Thanks for the patch! Marking for a second committer's review.

[SergeyBiryukov]

In 48384.2.diff​:

• Include the unit test from 48384.diff​ (slightly adapted for consistency).
• Document the default values for $field and $source parameters of get_post_datetime().
• Synchronize the $field parameter description for get_post_datetime() and get_post_timestamp().

[Rarst]

I think testing gmt_offset case is redundant. There is no functional difference between time zone modes for the issue.

[azaozz]

48384.2.diff looks good, +1 to commit.

[azaozz]

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.

[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?

The regression demonstrated by suggested test is fixed by patch.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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 of human_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.

[Rarst]

Ok, so there are two issues:

1. GMT slipping with time zone change, fixed by standing patch
2. column_date() logic behaving differently because get_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 $gmt timestamp of 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:

1. 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)
2. Stuff a check for valid get_post_time() return in the column_date() and skip the relative time branch, since we can't calculate it with bogus input

[Rarst]

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...

[azaozz]

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.

[Rarst]

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.

[Rarst]

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.

[Rarst]

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:

1. Enabling get_post_datetime() to replicate legacy behavior of instancing from local or UTC time in database
2. 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.

[SergeyBiryukov]

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.

[azaozz]

Confirmed that applying both patches fixes the reported issue, +1 to commit.

[SergeyBiryukov]

In 46578:

Date/Time: Correct the logic in WP_Posts_List_Table::column_date() and WP_Media_List_Table::column_date() to check for a valid post timestamp.

Props Rarst.
Reviewed by azaozz, SergeyBiryukov.
See #48384.

[SergeyBiryukov]

In 46579:

Posts, Post Types: Remove unintended change from [46578].

See #48384.

[SergeyBiryukov]

In 46580:

Date/Time: Make sure get_post_time() keeps UTC time on timezone change.

Add $source parameter to get_post_datetime() to instantiate from local or UTC time in database.

Props Rarst, david.binda.
Reviewed by azaozz, SergeyBiryukov.
Fixes #48384.

[SergeyBiryukov]

The unit test ​fails on PHP 5.6, reopening for investigation.

[Rarst]

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.

[SergeyBiryukov]

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 test gmt_offset, otherwise current value of timezone_string will always override whatever you set gmt_offset to.

Thanks! I thought I removed that test as per comment:9, but apparently confused my checkouts :)

[SergeyBiryukov]

In 46583:

Date/Time: Remove incomplete and redundant test for get_post_time() added in [46580].

There is no functional difference between gmt_offset and timezone_string timezone modes for the issue.

See #48384.

[SergeyBiryukov]

In 46584:

Date/Time: Remove incomplete and redundant test for get_post_time() added in [46580].

There is no functional difference between gmt_offset and timezone_string timezone modes for the issue.

Props Rarst.
Merges [46583] to the 5.3 branch.
Fixes #48384.