wp_old_slug_redirect() in 4.4 redirecting existing posts

[douglsmith]

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.

[SergeyBiryukov]

#34968 was marked as a duplicate.

[rommelxcastro]

define order by on redirect_guess_404_permalink

[rommelxcastro]

patch added.

ORDER BY post_name ASC

added to query

[SergeyBiryukov]

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:

1. Set the permalink structure to /%postname%/.
2. Create and publish a new post, "My Test".
3. Change its slug to my-test-post.
4. Create and publish a new post, "My Test".
5. The second post redirects to the first one.

[douglsmith]

That makes sense, @SergeyBiryukov. I had originally observed the problem on a custom post type that was non-hierarchical.

[bobbingwide]

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.

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

[bobbingwide]

Steps to reproduce... with month and name permalink structure ( /%year%/%monthnum%/%postname%/ )

1. Add new post title TRAC 35031 and publish
2. 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

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

4. Attempt to view the first post - you get shown the second one.

[bobbingwide]

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

[JureCuhalev]

For now adding back is_404() check fixes the issue for me. I'm not sure if it breaks any of the new functionality of the original patch.

[pento]

35031.diff​ is a first pass at fixing this. It needs some more thought, and tests.

[pento]

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 set is_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 loads template-loader.php, which does the template_redirect action, which wp_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​.

[jobst]

Why is it called "_wp_old_slug", shouldn't it be called "_wp_slug"?

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

[jobst]

#35114 was marked as a duplicate.

[jobst]

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

[pento]

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.

[jobst]

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?

[bobbingwide]

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

[bobbingwide]

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.

[bobbingwide]

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.

[pento]

#35012 was marked as a duplicate.

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

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.

[jobst]

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.

[bobbingwide]

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.

[strategio]

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

[SergeyBiryukov]

#35184 was marked as a duplicate.

[SergeyBiryukov]

#35189 was marked as a duplicate.

[gnotaras]

Hello, as I mentioned in #35189, it might be a good idea to not let wordpress perform permanent redirects on its own.

[pento]

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?

[jobst]

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?

[pento]

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

[jobst]

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

[pento]

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.

[jorbin]

@pento what do you need to get this committable?

[dd32]

@pento could a get_queried_object() check​ be used instead perhaps?

In my limited testing that appears to have the intended effect for me..

[jorbin]

This needs to be in 4.4.1

@pento and @dd32 can you two spearhead this to completion please?

[pento]

In 36128:

Redirects: Prevent redirects if a queried object exists.

After [34659], it became possible to cause an incorrect redirect, by changing the slug of a post, then creating a new post with the old slug. The correct behaviour is to prevent redirecting to the old post.

Props dd32, pento.

Fixes #35031 for trunk.

[pento]

In 36129:

Redirects: Prevent redirects if a queried object exists.

After [34659], it became possible to cause an incorrect redirect, by changing the slug of a post, then creating a new post with the old slug. The correct behaviour is to prevent redirecting to the old post.

Props dd32, pento.

Merge of [36128] to the 4.4 branch.

Fixes #35031.

[Clorith]

#35302 was marked as a duplicate.

[cbaldelomar]

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.

[ocean90]

See #35344.