Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#48623 closed defect (bug) (fixed)

WP 5.3 time problem causes day number shifts in permalinks

Reported by: steevithak's profile steevithak Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.3.1 Priority: normal
Severity: critical Version: 5.3
Component: Date/Time Keywords: has-patch has-unit-tests fixed-major
Focuses: Cc:

Description (last modified by SergeyBiryukov)

I use a "/%year%/%monthnum%/%day%/%postname%/" permalink for my blog posts. I've been doing it this way for many years, though many wordpress versions. And I have many blog posts.

After upgrading to WordPress 5.3, clicking on posts in my blog began sporadically resulting in 404 errors even though the posts were still visible in the wordpress post editor.

I did some research and found the problem was that the day field in the links displayed throughout my wordpress site for the permalinks sometimes have a shifted day field that varies between two days. When the day in the link matches the actual day in the permalink, the page loads normally. When the day in the link mismatches, the page gives a 404 error.

The day shifts occur even in the "view" links within the wordpress editor and the permalink display next to the editor.

Here's an actual example from my blog:

https://www.steevithak.com/2018/07/22/ecotopia-by-ernest-callenbach/

https://www.steevithak.com/2018/07/23/ecotopia-by-ernest-callenbach/

The first link is the actual, real permalink where the post lives, has always lived, and indexed by Google since the post was created.

The second link was copied-and-pasted from the WordPress editor permalink display for that blog post shortly after I upgraded to WP 5.3. It's also the URL WP 5.3 renders as the link to that blog post. It results in a 404 if you click because the day is wrong.

The day in the links throughout the site seems to shift between 23 and 22 depending on the time of day. But the post is only able to be loaded when the day 22 is used. I'm guessing there's a time zone bug in the code that renders the permalinks and when midnight occurs in WP5.3 time relative to midnight in the user's actual zone causes the day to shift.

I've found the easiest way to spot the issue is on the WP admin "All Posts" page. The day number in the "view" link under each post title will exhibit the day shifting problem, while the "Date" value on the far right shows the actual post day. So for the post above, the "Date" field always says day 22. But the day number in the "view" link will be 23 sometimes.

Attachments (2)

Screenshot-from-2019-11-14 18-09-20.png (514.3 KB) - added by steevithak 5 years ago.
screenshot of admin "all posts" page showing mismatch in dates
48623-get-permalink-with-changed-time-zone.patch (2.4 KB) - added by Rarst 5 years ago.

Download all attachments as: .zip

Change History (45)

#1 @SergeyBiryukov
5 years ago

  • Component changed from Permalinks to Date/Time
  • Description modified (diff)
  • Milestone changed from Awaiting Review to 5.3.1

Related: #48625

#2 @SergeyBiryukov
5 years ago

  • Version set to 5.3

#3 @Rarst
5 years ago

Could you please give a specific example including:

  1. What is your time zone in settings
  2. What are the post_date and post_date_gmt values in database
  3. What the old permalink was
  4. What the current permalink is
Last edited 5 years ago by Rarst (previous) (diff)

#4 @Rarst
5 years ago

I cannot reproduce this so far. I suspected it's about summer time getting fixed up through code base, but example is not close to transitions.

I tried to reproduce the post at that time, that timezone (I guessed central time? tried Chicago and UTC -6) with that permalink structure, in local dev install and I don't get it jumping to 23rd.

This might be caused by shift to UTC somewhere (as in possibly related ticket linked), which does get into 23rd for the example, but I don't have an idea where might that break. Especially since it doesn't seem to happen consistently through the site in either ticket.

#5 @steevithak
5 years ago

I can get you the additional data tonight when I get home from work. In the meantime, my guess is perhaps the problem doesn't show for posts created on 5.3? Maybe it uses the same method to generate the static permalinks as it does to build the dynamic link refs? Maybe try creating posts on an earlier version and then upgrade to 5.3 and see if the dynamically built links change relative to the static article permalinks? (I don't know the details of how the code works in that respect so just making wild guesses!)

FWIW, the link above is exhibiting the described problem now (around 08:15am central time in Dallas, TX). Not sure what time zone my server is in. It's a cloud vm, so probably UTC. I also have the archive list widget on my home page, which makes links that only include the year and month. In the archive link for July 2018, you can see the example post from above. But if you click on the post title on the archive page to get directly to the post page, you get a 404 (which my theme redirects to the index page).

I have hundreds of posts and only currently see this issue on a small number, so maybe the post has to made within the span of time between midnight in the two time zones to exhibit the problem? Or maybe it only manifests when the original post time is near midnight in the whatever zone WP 5.3 is using - so only 1/12 of my posts would shot the issue at any given time? I post at very random times, so more likely to hit a few of mine than someone who posts at the same time each day I guess.

Last edited 5 years ago by steevithak (previous) (diff)

#6 @Rarst
5 years ago

To my knowledge we hadn't done anything with permalinks regarding Date/Time in 5.3.

It happening to a fraction of posts is confusing as well, we need to figure out what those posts have in common and which API calls exactly fail for them (or possibly which API calls are not broken in the way they were broken to produce certain output before).

#7 @archon810
5 years ago

We are having exactly the same issue except with some of our posts, we end up with endless redirects, possibly because of some custom code. But the root cause is the original link ends up a 404 now instead.

So this definitely has to do with time zones, right? That would explain why only some posts are suffering from this issue.

#8 @Rarst
5 years ago

The jump in day is likely date/time related, but it's not clear what is going wrong and why only on certain posts. We hadn't done anything with permalinks themselves, so something is going wrong with reading and transforming data OR something was wrong before and getting it fixed caused inconsistency with old state.

As above, specific examples with database and time zone information would be helpful! :)

#9 @maciejmackowiak
5 years ago

@Rarst

  1. time zone Los Angeles

2.

post_date 2019-03-18 19:57:43
post_date_gmt 2019-03-19 02:57:43

3 and 4 I'm not sure which one is old and which one is new

But current permalink shown in post editor is:

/2019/03/19/test/

and current working one is:

/2019/03/18/test/

Also when I go to localhost/?p={post_id}
I'm redirected to the not working one /2019/03/19/test/

This ticket was mentioned in Slack in #forums by joyously. View the logs.


5 years ago

#11 @steevithak
5 years ago

I've been browsing the code repo and I'm betting the bug is caused by a change in link-template.php. In 5.2 and older versions you generated the permalinks using date(), which renders the permalink in the user's local time zone but starting in 5.3 you're using gmdate(), which seems wrong as it renders the link in UTC, which in my case means the permalinks are 6 hours off from where the post actually is, and occasionally slips a day boundary. This causes any link created with get_permalink($post->ID) to potentially be wrong if the time zones differ (and assuming the post was originally created on the non-broken older version and at a time close enough to midnight in that time zone to trigger the bug after the upgrade).

Line 215 of
https://core.trac.wordpress.org/browser/branches/5.3/src/wp-includes/link-template.php

$date = explode( ' ', gmdate( 'Y m d H i s', $unixtime ) );

vs

Line 215
https://core.trac.wordpress.org/browser/branches/5.2/src/wp-includes/link-template.php

$date = explode( ' ', date( 'Y m d H i s', $unixtime ) );

#12 @Rarst
5 years ago

In 5.2 and older versions you generated the permalinks using date(), which renders the permalink in the user's local time zone but starting in 5.3 you're using gmdate(), which seems wrong as it renders the link in UTC

This does not sound right, WP core sets PHP time zone to UTC on boot, so date() does not and should not operate with local time (that's why we replaced it with gmdate() which is equivalent for UTC, but does not get broken if something goes and changes time zone).

I will review the logic on fresh head tomorrow, thank you. :)

#13 @steevithak
5 years ago

I can now confirm that swapping gmdate() back to date() at line 215 in link-template.php fixes all the 404s due to the off-by-one-day problem in the blog post links.

I'll defer to you guys whether the change from date() to gmdate() was correct in this part of the code, but if it is correct, then what's missing may be a corresponding fix for all the existing permalinks on old posts that were created with the old code. My only worry is that if you change the actual post permalinks, it will cause breakage out on the Internet with search engines and other sites now linking to a permalink that turned out to be a "temporarylink" :) As it stands now, users coming in from Google searches or links on other sites are still getting to my posts correctly. It's only users clicking on internal links created within WordPress by dynamic calls to get_permalink() that are wrong and lead to 404s.

I've got one clue that might offer a solution though. Whatever method you use to calculate the date in Admin -> All Posts, still shows the correct day. So it's clearly using some other method than get_permalink() is and it's still producing consistent dates for me. So maybe get_permalink() needs to use whatever method that is (I haven't tracked down the code that generates that field but may do that tonight if I have time).

#14 @archon810
5 years ago

I'll add this piece of data, and maybe @steevithak can chime in as well.

Our servers are on PST. And at some point during the last 10 years, there were manual calls added to WP theme code that run

date_default_timezone_set(get_option('timezone_string'));

in functions.php.

This option is set to "America/Los_Angeles" in wp_options.

With this setup, as @steevithak noted, the times in the Posts area are correct (in PST).

But under the Revisions list on the edit post page, the "Bla edited N hours ago" list is wrong and has the wrong offsets for us.

Also under wp-admin/options-general.php, the times are incorrect as well:
https://i.imgur.com/nxRfCzy.png

The time right now is actually 12:38 PM Pacific Time.

Version 0, edited 5 years ago by archon810 (next)

#15 @Rarst
5 years ago

I did a quick check and under normal circumstances date() and gmdate() calls in that place are equivalent. As they should be.

If they are not please double check that your default PHP timezone (date_default_timezone_get()) isn't modified from UTC, which WP sets it to on boot.

This might be a case where two bugs compounded to "correct" result, but now that one of them is fixed it's breaking.

@archon810

Ah, and the evidence trickling in. :) Do not use date_default_timezone_set() in WordPress, it's broken and had always been. To be clear the function works fine in normal PHP, but WordPress specifically is coded in a way that massively breaks if it's used. We are dealing with it, which is exactly why some problems is out of the woods right now.

Last edited 5 years ago by Rarst (previous) (diff)

#16 @archon810
5 years ago

Thanks, we'll take a look and report back. So many variables in play.

@steevithak
5 years ago

screenshot of admin "all posts" page showing mismatch in dates

#17 @steevithak
5 years ago

I've attached a screenshot of my admin "all posts" page with the mouse pointer over the "view" link for a blog post that exhibits the problem. In the pictured case, the actual permalink is "2019/09/29" and the date shown in the date column is also 2019/09/29. But the URL (see circled area at bottom of browser) in the "view" link uses 2019/09/30, which is incorrect and generates a 404 error if you click it.

This demonstrates that the algorithm used to parse the post date for get_permalink() is inconsistent with the (correct) algorithm used to display the post date.

#18 @archon810
5 years ago

grep your theme and pluguns for calls to date_default_timezone_set() - I bet you'll find something there that's causing this bug.

#19 @steevithak
5 years ago

Could you please give a specific example including:

What is your time zone in settings

CentOS 7.7.1908
OS Timezone: UTC

PHP 7.3.11-1
via phpinfo():
date/time support enabled
timelib version 2018.02
"Olson" Timezone Database Version 0.system
Timezone Database internal
Default timezone America/Chicago

WordPress
Admin -> Settings -> General Settings -> Timezone: UTC-6
Displayed below the setting is this example timestamp, which is correct:
"Universal time is 2019-11-15 02:04:11. Local time is 2019-11-14 20:04:11"

In answer to @archon810, I found this in the themes functions.php:
date_default_timezone_set("America/Chicago")

What are the post_date and post_date_gmt values in database

I haven't played with MySQL in a while but I think this is what you want:

select post_date, post_date_gmt from wp_posts where ID = 2858;
+---------------------+---------------------+
| post_date           | post_date_gmt       |
+---------------------+---------------------+
| 2018-07-22 21:13:23 | 2018-07-23 03:13:23 |
+---------------------+---------------------+
1 row in set (0.00 sec)

What the old permalink was
What the current permalink is

I'm unsure about new vs old in this context, so how about these, which you can try out on the live site:

The working permalink URL that the blog post was created with at the time of its creation and can be used to view the post in a web browser:

https://www.steevithak.com/2018/07/22/ecotopia-by-ernest-callenbach/

The non-working permalink URL that get_permalink() returns as of WP 5.3 and that results in a 404 error:

https://www.steevithak.com/2018/07/23/ecotopia-by-ernest-callenbach/

Last edited 5 years ago by steevithak (previous) (diff)

#20 follow-ups: @Rarst
5 years ago

@steevithak

In answer to @archon810, I found this in the themes functions.php:
date_default_timezone_set("America/Chicago")

Yep, likely same thing. Please try dropping any and all date_default_timezone_set() calls (not counting WordPress core boot itself).

I will look into patching get_permalink() to be more resilient about this (we are slowly doing this through the whole core), but in general changing PHP time zone in WP breaks things and isn't normal mode of operation.

@archon810 are you good now?

@maciejmackowiak could you please check for same issue, any date_default_timezone_set() in your code?

#21 in reply to: ↑ 20 @archon810
5 years ago

Replying to Rarst:

@archon810 are you good now?

@maciejmackowiak could you please check for same issue, any date_default_timezone_set() in your code?

@maciejmackowiak works for me and he'll get back to you hopefully tomorrow with how things went with the fix. For now, 5.3 is only on the dev server, and we're sticking to 5.2.4 on prod.

#22 @Rarst
5 years ago

  • Keywords has-patch has-unit-tests added

Added patch that makes get_permalink() resilient against PHP time zone changes and unit test.

This ticket was mentioned in Slack in #core-datetime by rarst. View the logs.


5 years ago

#24 follow-up: @maciejmackowiak
5 years ago

Looks like deleting date_default_timezone_set fixes the issue.
@Rarst thanks for help.

#25 in reply to: ↑ 20 @steevithak
5 years ago

Replying to Rarst:

Yep, likely same thing. Please try dropping any and all date_default_timezone_set() calls (not counting WordPress core boot itself).

Ok, thanks! I'll remove it tonight and report back. Should I expect any other side-effects from removing it that I'll need to clean up? Or is the removal expected to fully fix the time shift problem?

#26 @Rarst
5 years ago

Should I expect any other side-effects from removing it that I'll need to clean up?

Hard to tell, WordPress core was really written to work exclusively with time zone set to UTC for a very long time. There isn't really a clear list of what are the effects on core operation, since no one is supposed to be doing this in first place.

Day shift should probably just go away, since in this case existing permalinks are "correct" even though they were generated in "incorrect" environment (which is now breaking how links are generated).

#27 in reply to: ↑ 24 @Ov3rfly
5 years ago

Looks like deleting date_default_timezone_set fixes the issue.

Seems quite some plugins and also a few themes in repository and probably more outside of repository are using this, not sure if relevant to this issue though.

#28 @steevithak
5 years ago

I've commented out the call to date_default_timezone_set() and so far, all the URLs coming from get_permalink() seem to match the actual blog posts dates and URLs again. I'll continue checking and report back if I find anything wrong. Thanks for your help diagnosing this!

I wonder if it might make sense to add a check for this problem to Tools -> Site Health? A simple shell command like 'ack date_default_timezone_set' or 'grep -r date_default_timezone_set *' in the doc root directory would do it.

Interestingly, I found that the twitter plugin I use, "WP to Twitter", also calls this function but with the argument 'UTC' so I think it's safe to leave that one.

#29 @Rarst
5 years ago

Good idea about adding to Site Health I opened a ticket #48692

Note that we don't even need to scan the source, as long as date_default_timezone_get() still returns UTC it should be fine. Some code can change time zone for its purposes, but then (correctly) restore the UTC state. Core itself used to do this, I got rid of it by now.

#30 @Cybr
5 years ago

  • Severity changed from major to critical

I think the recent change also causes canonical URL issues <link rel=canonical href=... />. Whether that's outputted via WordPress itself, or via an SEO plugin that doesn't rectify this, this canonical URL causes 404 responses and can take down sites from search engines. Therefore, I'm marking this ticket as critical.

Now, this isn't necessarily a plugin conflict problem, because even when date_default_timezone_get() isn't used, PHP still has this configuration setting: date.timezone.

When either is set to anything but UTC, gmdate() will return offsetted dates. But, the timestamp used in get_permalink() ($post->post_date) is always UTC (instigated in touch_time() via current_time()).

Before 5.3, in get_permalink(), WordPress used date() instead of gmdate(), which worked perfectly. (source)

Reverting that change should fix the issue. I haven't found the time to create a proof of concept. So, please do test this if you can.

Alternatively, you could add the known PHP offset to the $unixtime variable, and then keep using the gmdate() function as shown below. However, I think this is redundant and unfavorable to the revert.

$unixtime = strtotime( $post->post_date ) + date( 'Z' );

#31 @Rarst
5 years ago

Now, this isn't necessarily a plugin conflict problem, because even when date_default_timezone_get() isn't used, PHP still has this configuration setting: date.timezone.

Ini setting is irrelevant to WordPress because it explicitly sets default time zone to UTC during core load.

This is absolutely the issue with third party code using date_default_timezone_set(), which was never supported by WordPress due to the way it originally implemented Date/Time calculations. Unfortunately this is very old and quite obscure issue, developers are rarely well familiar with.

We are not reverting gmdate() change because core-wide it's meant exactly to prevent PHP time zone change breaking things all over. That work is just far from complete due to enormous amount of ancient Date/Time code in core.

I have already suggested the patch for get_permalink() issue. Please let me know if you discover it's lacking to address the problem in some way.

We wouldn't go with any manipulation of timestamp because the current course is to remove and deprecate operating with sums of timestamp and offsets in core.

#32 @Cybr
5 years ago

I understand! I had to test the claims on my part, thank you for clarifying.

I got to it, and yes, the patch you provided works for me.

#34 in reply to: ↑ description @scvleon
5 years ago

Replying to steevithak:
I am in Los Angeles and am having exactly the same problem with 5.3 on our 2 WordPress sites (scvtv.com and scvnews.com). One uses an old Yamidoo theme and the other uses a customized version of 2013 (I think -- I didn't build the sites.)

#35 @miette49
5 years ago

Hi-- we have the same bug on our own site. Commenting out calls to date_default_timezone_set() (which we were using to retrieve posts within the past 24 hours, and the past 7 days, to send daily/weekly email updates) fixes it, though now we need to rewrite those functions.

In every case, the posts affected have a discrepancy between post_date and post_date_gmt.

This ticket was mentioned in Slack in #core by cybr. View the logs.


5 years ago

#37 follow-up: @SergeyBiryukov
5 years ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

This ticket was mentioned in Slack in #core by francina. View the logs.


5 years ago

#39 in reply to: ↑ 37 @scvleon
5 years ago

Replying to SergeyBiryukov: Hi! How is the solution coming along? Our local news sites are suffering. Thanks!

#40 @SergeyBiryukov
5 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 46795:

Date/Time: Make get_permalink() more resilient against PHP timezone changes.

Overriding default PHP timezone with date_default_timezone_set(), while not recommended, should not inadvertently result in changing existing permalinks.

Add a unit test.

Props Rarst, steevithak, archon810, maciejmackowiak, Ov3rfly, Cybr, hometowntrailers, scvleon, miette49.
Fixes #48623.

#41 @SergeyBiryukov
5 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 5.3.1.

#42 @SergeyBiryukov
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 46796:

Date/Time: Make get_permalink() more resilient against PHP timezone changes.

Overriding default PHP timezone with date_default_timezone_set(), while not recommended, should not inadvertently result in changing existing permalinks.

Add a unit test.

Props Rarst, steevithak, archon810, maciejmackowiak, Ov3rfly, Cybr, hometowntrailers, scvleon, miette49.
Merges [46795] to the 5.3 branch.
Fixes #48623.

#43 @SergeyBiryukov
5 years ago

#48923 was marked as a duplicate.

Note: See TracTickets for help on using tickets.