#15397 closed enhancement (fixed)
redirect_guess_404_permalink() doesn't guess posts with updated dates
Reported by: | archon810 | Owned by: | pento |
---|---|---|---|
Milestone: | 4.9.3 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Canonical | Keywords: | has-patch has-unit-tests fixed-major |
Focuses: | Cc: |
Description
Problem
Here's my post path scheme: http://site.com/YEAR/MONTH/DAY/SLUG. Whenever I have writers working on a post for a while and saving drafts (we're all using Windows Live Writer), they oftentimes publish to the date when the last draft was saved, i.e. several days in the past. Then, they quickly correct the date but the previously tweeted/shared link is now 404 due to the changed date.
I've looked into the source of redirect_guess_404_permalink(), and it purposedly narrows down the query when it sees a post date to that date only. If I understand correctly, this is done to minimize accidental redirects to the wrong post, but has the side effect of not guessing the new link if only the date was changed.
A workaround of removing these lines:
if ( get_query_var('year') ) $where .= $wpdb->prepare(" AND YEAR(post_date) = %d", get_query_var('year')); if ( get_query_var('monthnum') ) $where .= $wpdb->prepare(" AND MONTH(post_date) = %d", get_query_var('monthnum')); if ( get_query_var('day') ) $where .= $wpdb->prepare(" AND DAYOFMONTH(post_date) = %d", get_query_var('day'));
fixes the problem for me.
Can this case be solved in the trunk and the code above removed, or logic improved? My .htaccess file is filled with 301 redirects to correct wrong dates.
Thank you.
Attachments (3)
Change History (36)
#2
@
14 years ago
- Keywords 2nd-opinion added
- Milestone changed from Awaiting Review to Future Release
- Priority changed from normal to lowest
- Type changed from defect (bug) to enhancement
#3
@
14 years ago
Sounds like an acceptable workaround, similar to how _wp_old_slug works for slugs. Alternatively, maybe add a hook into this function, or provide an option we can set in order to disable date checking.
Right now, my core is edited (and it's worked really well), but I hate to see my core files with modifications.
Thanks.
#4
@
14 years ago
I would love to see this function instead hooked in. I was asked a few weeks ago if there was a way to disable exactly this (but keep the rest of canonical), and it required a goofy hack.
#5
follow-up:
↓ 6
@
14 years ago
Removing the specific months/days from the query has been mentioned before, and IMO it'd be nice if it could catch cases like that as well.
But it gets to a point where it may make the redirection more liberate than preferred, As soon as I saw this ticket, I thought that perhaps, we should be returning a Search Results page listing the possible locations which may have been intended..
Another option, is to attempt redirection with the date, and progressively remove refinements until a single post is returned. (ie. 2010/05/04/postname doesnt return, does 2010 + 05 + 04 + postname return? ok, how about 2010 + 05 + postname, 2010+postname? etc)
#6
in reply to:
↑ 5
@
14 years ago
Replying to dd32:
Removing the specific months/days from the query has been mentioned before, and IMO it'd be nice if it could catch cases like that as well.
But it gets to a point where it may make the redirection more liberate than preferred, As soon as I saw this ticket, I thought that perhaps, we should be returning a Search Results page listing the possible locations which may have been intended..
Another option, is to attempt redirection with the date, and progressively remove refinements until a single post is returned. (ie. 2010/05/04/postname doesnt return, does 2010 + 05 + 04 + postname return? ok, how about 2010 + 05 + postname, 2010+postname? etc)
A search page for when the # of results is > 1 sounds acceptable, and so does the latter solution, although my concern is that it might result in more queries (which, in this case, is actually not bad - it would only result in 1 more query if there are no results by the time you get to the date filter).
#7
@
14 years ago
- Keywords has-patch dev-feedback added; 2nd-opinion removed
Attaching is a patch that tries to make the function more lenient without being too liberal:
If only one match for post name is found, it returns that without querying dates. Otherwise it tries to find the best match out of several by factoring in dates, incrementally by year, month, and day. There has to be a clear winner on the date match otherwise it returns false.
#8
follow-up:
↓ 9
@
14 years ago
Good start. What about limiting the number of days that it can drift? I mean, what happens if you delete a post from 2004 with a slug of "my-post" and then subsequently you create a post in 2011 with a slug of "my-post" — you wouldn't want the 2004 URL to now redirect to the 2011 URL. When people change dates on posts, I'd wager that 90% of them change it plus or minus one day. We could restrict the changes to plus or minus 5 days (or so) and probably cover the majority of legitimate changes, without risking false redirection scenarios.
#9
in reply to:
↑ 8
@
14 years ago
Replying to markjaquith:
Good start. What about limiting the number of days that it can drift?
It would only be possible when the permalink structure is /%year%/%monthnum%/%date%/%postname%/
, which is only one of many possible structures. So if the structure is /%year%/%postname%/
then we can only match the year..
#10
@
14 years ago
@mark Unless you make the time period configurable, your solution is highly subjective. Writers can be working on a piece for days, weeks, or even months and then the final URL published by accident. Similarly, a post may be updated and its date changed so that it jumps back onto the front page - thus the date difference could be anything.
#12
@
11 years ago
- Keywords needs-patch needs-unit-tests added; has-patch dev-feedback removed
I like the idea of storing some small number of date changes in post meta, and only performing the redirect if the post has an old matching date. It seems like the least magic option.
We can offer some level of customization by allowing the number of dates store to be configurable (__return_zero
would disable completely)
#14
@
9 years ago
- Summary changed from redirect_guess_404_permalink() purposedly doesn't guess posts with updated dates to redirect_guess_404_permalink() doesn't guess posts with updated dates
#16
@
7 years ago
- Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed
- Milestone changed from Future Release to 4.9.2
- Owner set to pento
- Status changed from new to accepted
#18
@
7 years ago
- Keywords fixed-major added
- Resolution fixed deleted
- Status changed from closed to reopened
#21
@
7 years ago
@archon810: It's currently fixed in trunk (which will be WordPress 5.0). I re-opened it for backporting to 4.9.2, as well.
#22
@
7 years ago
The tests could be improved:
- Use shared fixtures, which makes the tests faster.
- Use
assertSame()
, which checks types (not necessary here, but a good best practice nonetheless). - Avoid using legacy method arguments when creating the attachment fixture (existing tests are often reused when writing others, so let's do it right the first time).
- Do not remove the added filter on
tearDown()
, as this is handled by theWP_UnitTestCase
class (hooks are saved insetUp()
and restored intearDown()
).
This ticket was mentioned in Slack in #core by desrosj. View the logs.
7 years ago
This ticket was mentioned in Slack in #core by desrosj. View the logs.
7 years ago
This ticket was mentioned in Slack in #core by sergey. View the logs.
7 years ago
#31
follow-up:
↓ 32
@
7 years ago
Warning: Looks like the fix is not retroactive so I'll have to keep my redirect function in indefinitely.
#32
in reply to:
↑ 31
;
follow-up:
↓ 33
@
7 years ago
Replying to archon810:
Warning: Looks like the fix is not retroactive so I'll have to keep my redirect function in indefinitely.
Correct, you can also migrate it to store the data the same way core does, to avoid having to run your own variant indefinately.
If you look at the above commits, you'll find it stores the data in _wp_old_date
. I'd suggest inspecting an actual install though.
#33
in reply to:
↑ 32
@
7 years ago
Replying to dd32:
Replying to archon810:
Warning: Looks like the fix is not retroactive so I'll have to keep my redirect function in indefinitely.
Correct, you can also migrate it to store the data the same way core does, to avoid having to run your own variant indefinately.
If you look at the above commits, you'll find it stores the data in
_wp_old_date
. I'd suggest inspecting an actual install though.
The problem is our solution doesn't keep track of dates either, it just detects 404s and attempts to check if the slug is valid for a post, then finds the right date. So I can change the dates all day long, and they'll all still redirect correctly.
Seems to me we'll have to store date changes as well. Otherwise we'll be making far too liberal redirects, which this is designed to prevent.