#35031 closed defect (bug) (fixed)
wp_old_slug_redirect() in 4.4 redirecting existing posts
Reported by: | douglsmith | Owned by: | pento |
---|---|---|---|
Milestone: | 4.4.1 | Priority: | highest omg bbq |
Severity: | critical | Version: | 4.4 |
Component: | Query | Keywords: | has-patch fixed-major |
Focuses: | Cc: |
Description
After updating to WordPress 4.4, I noticed that some of my existing pages were redirecting to other pages. The unique thing about these pages is that at one time they had been moved to a new slug and a new page was created and given the old slug.
I have not been able to recreate this condition by adding a new page, changing the slug, then adding a new page at the original slug. Those older pages had a _wp_old_slug
custom field set with the original URL but that is not being created as I move pages now. So this may only be an issue for sites with older page moves.
I believe this behavior is caused by the removal of is_404() in [34659]. It had previously stopped a redirect away from a post / page that currently exists, even if another post had its URL in a _wp_old_slug
custom field.
This is different from, but may be related to #35012.
Attachments (4)
Change History (57)
This ticket was mentioned in Slack in #core by bobbingwide. View the logs.
9 years ago
#5
in reply to:
↑ description
@
9 years ago
Replying to douglsmith:
I have not been able to recreate this condition by adding a new page, changing the slug, then adding a new page at the original slug. Those older pages had a
_wp_old_slug
custom field set with the original URL but that is not being created as I move pages now. So this may only be an issue for sites with older page moves.
_wp_old_slug
is only added for non-hierarchical post types, see wp_check_for_changed_slugs() and #4328.
I was able to reproduce the issue with posts:
- Set the permalink structure to
/%postname%/
. - Create and publish a new post, "My Test".
- Change its slug to
my-test-post
. - Create and publish a new post, "My Test".
- The second post redirects to the first one.
#6
@
9 years ago
That makes sense, @SergeyBiryukov. I had originally observed the problem on a custom post type that was non-hierarchical.
#7
@
9 years ago
The patch does not work for my non-hierarchical post type.
I have 3 'fixture's with the same post title - Huntercombe GC. One for 2014, one for 2015 and the latest for 2016.
IDs and slugs are: 14 huntercombe-gc, 492 huntercombe-gc-2, and 699 huntercombe-gc-3
Only post 699 has post meta _wp_old_slug. The value is 'huntercombe-gc'
The current logic means I can no longer visit /fixture/huntercombe-gc
It redirects to /fixture/huntercombe-gc-3
Workaround: delete the unwanted postmeta row.
Further analysis reveals:
The final post was initially created as "pending".
In that state it had a post_name of 'huntercombe-gc'.
When it was published the _wp_old_slug was set, and the new post_name became huntercombe-gc-3.
#8
follow-up:
↓ 10
@
9 years ago
I can reproduce this on my Wordpress, I also see old _wp_old_slug problem as described by bobbingwide. First patch rommelxcastro doesn't fix this problem in my case.
I ran bisect on commits and the commit that introduced this problem seems to be [34659] - for ticket #33920. Before that, the redirect doesn't happen.
#9
@
9 years ago
Steps to reproduce... with month and name permalink structure ( /%year%/%monthnum%/%postname%/ )
- Add new post title
TRAC 35031
and publish - Add new post title
TRAC 35031
and save as pending
Both the pending and the published post have the same value for post_name: trac-35031
- Publish the pending post
The new post's post_name is changed (to trac-35031-2
) and the _wp_old_slug
created as trac-35031
- Attempt to view the first post - you get shown the second one.
#10
in reply to:
↑ 8
@
9 years ago
Replying to JureCuhalev:
I can reproduce this on my Wordpress, I also see old _wp_old_slug problem as described by bobbingwide. First patch rommelxcastro doesn't fix this problem in my case.
I ran bisect on commits and the commit that introduced this problem seems to be [34659] - for ticket #33920. Before that, the redirect doesn't happen.
I made the same conclusion in slack.
https://wordpress.slack.com/archives/core/p1450203322005811
#11
@
9 years ago
For now adding back is_404()
fixes the issue for me. I'm not sure if it breaks any of the new functionality of the original patch.
#14
@
9 years ago
- Keywords needs-unit-tests needs-testing added
35031.diff is a first pass at fixing this. It needs some more thought, and tests.
#15
@
9 years ago
So, here's a quick order of operations, so that I don't have to keep re-reading all of this:
- From
wp-blog-header.php
,wp()
is called, which calls$wp->main()
. $wp->main()
sets up the query, which looks for the post that matches the slug.$wp->main()
then calls$wp->handle_404()
, which will setis_404()
to true when the page doesn't exist. It *doesn't* set it to true when the URL is a feed, because lol.- We go back to
wp-blog-header.php
, which loadstemplate-loader.php
, which does thetemplate_redirect
action, whichwp_old_slug_redirect()
is hooked into.
Now, the important bit here is that part of the point of [34659] was to handle URL endpoints, like /feed/
. Feeds don't have a 404, though, so we can't just re-add the is_404()
check. Instead, feeds are handled more intelligently inside wp_old_slug_redirect()
. We want to maintain that logic.
Because we know if a post does or doesn't exist (as determined by $wp->handle_404()
), then we know that, after checking is_feed()
, and is_404()
is not true, we're looking at a real post, and we shouldn't redirect.
And so, we have 35031.2.diff.
#16
@
9 years ago
Why is it called "_wp_old_slug", shouldn't it be called "_wp_slug"?
I am curious to know, is this written anywhere?
#17
follow-up:
↓ 19
@
9 years ago
_wp_old_slug
is the slug that the post used to have, before it was changed. If the post slug is never changed, it doesn't have a _wp_old_slug
meta value.
The current post slug is stored in the post_name
.
#19
in reply to:
↑ 17
@
9 years ago
Replying to pento:
_wp_old_slug
is the slug that the post used to have, before it was changed. If the post slug is never changed, it doesn't have a_wp_old_slug
meta value.
The current post slug is stored in the
post_name
.
So why is it necessary to check for the "_wp_old_slug" as the slug is already contained in the post table? I am just trying to get my head around this ...
#20
follow-up:
↓ 23
@
9 years ago
This is so that old links don't break, if the slug is changed.
For example:
- The owner of example.com posts example.com/my-psot/
- That post is linked to from Reddit, giving them lots of traffic.
- They realise there's a typo in the slug! They want to rename it, but they don't want their visitors to get a 404.
- When they rename it, WordPress stores the old slug in
_wp_old_slug
. If people try to visit the old URL, they'll be quietly redirected to the new one.
This behaviour has existed in WordPress since 2.1. The bug in this ticket is if a post slug is changed, then a new post is created with the old slug, it still redirects, instead of stopping at the new post.
#21
@
9 years ago
Applying 35031.diff and 35031.2.diff (via editor) work for me.
Those patches applying the correct logic as the test for an existing post was missing in the code within "wp_old_slug_redirect()".
However, my line numbers are slightly different, how come?
This ticket was mentioned in Slack in #core by dd32. View the logs.
9 years ago
#23
in reply to:
↑ 20
@
9 years ago
Replying to pento:
This is so that old links don't break, if the slug is changed.
The bug in this ticket is if a post slug is changed, then a new post is created with the old slug, it still redirects, instead of stopping at the new post.
Should I be raising a separate ticket where, IMO, the _wp_old_slug should not have been created in the first place when the new post changed from "pending" to "published" ?
#24
@
9 years ago
Comments on 35031.diff. I can't see why you need to run another query (using get_posts()) to check the name of the post that has already been fetched. For me $wp_query already contains the required post.
I would only expect another query to be run if this were not the case.
There is another challenge though... without the new test the next query
SELECT post_id FROM wp_postmeta, wp_posts WHERE ID = post_id AND post_type = 'fixture' AND meta_key = '_wp_old_slug' AND meta_value = 'huntercombe-gc'
can return multiple results.
Since there is no ordering then the ID that's returned could be different each time.
This may lead to #35012.
This ticket was mentioned in Slack in #core by bobbingwide. View the logs.
9 years ago
#26
@
9 years ago
Updated #35012 with a scenario to reproduce the redirect loop problem.
I then reduced the test to
if ( 1 === $wp_query->post_count ) { return; }
This basically says... if you got what you were looking for in the first place then that's fine by me.
But I believe that the next query may still need an order by to prevent it from producing random results.
#28
follow-ups:
↓ 29
↓ 30
@
9 years ago
Note: I'm currently having problems when I introduced "pending" posts with the same slug into the scenario.
The existence of the pending post seems to prevent a visitor from viewing the previously published post.
It gives a 404 when you click on the permalink for the existing published post.
Note: I've not be working in a vanilla environment; it seems WP-members was affecting the results in one.
In another the pending post's slug is already being suffixed. Watch this space.
#29
in reply to:
↑ 28
@
9 years ago
Replying to bobbingwide:
I just checked this on my system and it works correctly after applying 35031.1.diff and 35031.2.diff.
I duplicated a post, then changed the slug to end up with "_wp_old_slug" for that post.
I then duplicated the newly created post again, but left it saved as a draft (pending).
I then searched using phpMyAdmin the table wp_postmeta for "_wp_old_slug" and sorted it in order of post_id, showing the last two post_id's correctly on top.
Both had the same "_wp_old_slug", which is correct as the slug has not been changed.
Clicking on the link for the first post displayed the first post correctly after applying patch 35031.1.diff and 35031.2.diff.
Note: I'm currently having problems when I introduced "pending" posts with the same slug into the scenario.
The existence of the pending post seems to prevent a visitor from viewing the previously published post.
It gives a 404 when you click on the permalink for the existing published post.
Note: I've not be working in a vanilla environment; it seems WP-members was affecting the results in one.
In another the pending post's slug is already being suffixed. Watch this space.
#30
in reply to:
↑ 28
@
9 years ago
Replying to bobbingwide:
Note: I'm currently having problems when I introduced "pending" posts with the same slug into the scenario.
The existence of the pending post seems to prevent a visitor from viewing the previously published post.
It gives a 404 when you click on the permalink for the existing published post.
I've raised #35131 to address this bug, suggesting one solution would be to ensure pending posts don't get the same slug as an existing one.
#31
@
9 years ago
Here's a temporary fix which works for me:
function fix_35012_wp_old_slug() {
global $wp_query;
if ( $wp_query->post_count > 0 ) {
remove_action( 'template_redirect', 'wp_old_slug_redirect' );
}
}
add_action( 'template_redirect', 'fix_35012_wp_old_slug', 5 );
More details here: https://wpml.org/forums/topic/after-updating-wpmlwp-some-posts-automatically-redirect-to-english-version/page/2/#post-771536
This ticket was mentioned in Slack in #core by bobbingwide. View the logs.
9 years ago
#35
@
9 years ago
Hello, as I mentioned in #35189, it might be a good idea to not let wordpress perform permanent redirects on its own.
#36
follow-up:
↓ 38
@
9 years ago
I don't think that checking $wp_query->post_count
is the correct solution. There are cases in WP::handle_404()
where $wp_query->posts
(and by extension, $wp_query->post_count
) can be set, but it will still return a 404. I don't really want to just copy the tests from WP::handle_404()
, because they could change in the future.
I think 35031.2.diff is close. @bobbingwide, you mentioned that it doesn't fix the bug in some situations with WPML - could you please provide steps to reproduce this?
This ticket was mentioned in Slack in #outreach by kovshenin. View the logs.
9 years ago
#38
in reply to:
↑ 36
@
9 years ago
Replying to pento:
I think 35031.2.diff is close. @bobbingwide, you mentioned that it doesn't fix the bug in some situations with WPML - could you please provide steps to reproduce this?
Does this mean you would only include 35031.2.diff?
IMHO you would also need to include 35031.diff or is this incorrect?
#39
@
9 years ago
A variation of 35031.2.diff that catches the WPML bug, yes.
35031.diff was a first pass, it runs an extra query on every page load, so isn't a viable option for the actual fix.
All of the information is there to bring back the old behaviour, we just need to get it in the right order. :-)
#40
@
9 years ago
@pento suggestion for the function wp_old_slug_redirect() while you are changing it:
Currently on the second line of the function the code is:
if ( '' !== $wp_query->query_vars['name'] ) :
which is then closed just before function end 80 lines further down.
To be in line with the other lines of code for that function wouldn't it be better to write that as:
if ( '' === $wp_query->query_vars['name'] ) { return; }
It would be more readable as well and you do not need to scroll down 80 lines to see what happens afterwards.
Just a thought.
#41
@
9 years ago
Yeah, that if
is kind of awkward. I'll have a think about it, but I don't really want to change the indentation for every line, it makes a mess in svn blame
.
#43
@
9 years ago
@pento could a get_queried_object() check be used instead perhaps?
In my limited testing that appears to have the intended effect for me..
This ticket was mentioned in Slack in #core by jorbin. View the logs.
9 years ago
#45
@
9 years ago
- Priority changed from normal to highest omg bbq
This needs to be in 4.4.1
@pento and @dd32 can you two spearhead this to completion please?
This ticket was mentioned in Slack in #core by jorbin. View the logs.
9 years ago
#48
@
9 years ago
- Keywords fixed-major added; needs-unit-tests needs-testing removed
- Resolution fixed deleted
- Status changed from closed to reopened
#51
follow-up:
↓ 52
@
9 years ago
- Keywords dev-feedback added
- Resolution fixed deleted
- Status changed from closed to reopened
The following addition in "includes/query.php"
if ( get_queried_object() ) { return; }
Causes a static home page with pagination links not to work and always redirects to the home page.
Example Permalink: http://dev.wordpress.org/page/2/
This does not work on a static home page that has a posts shortcode inserted that uses paginate_links(), or any post that uses the page links tag <!--nextpage-->
https://codex.wordpress.org/Styling_Page-Links
https://codex.wordpress.org/Function_Reference/paginate_links
Still researching a solution to this problem. But I wanted to make everyone aware of it as this is a critical bug.
#52
in reply to:
↑ 51
@
9 years ago
- Keywords dev-feedback removed
- Resolution set to fixed
- Status changed from reopened to closed
See #35344.
#34968 was marked as a duplicate.