WordPress.org

Make WordPress Core

Opened 18 months ago

Closed 17 months ago

Last modified 17 months ago

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

35031.patch (719 bytes) - added by rommelxcastro 18 months ago.
define order by on redirect_guess_404_permalink
35031.diff (758 bytes) - added by pento 18 months ago.
35031.2.diff (464 bytes) - added by pento 18 months ago.
35031.3.diff (1.1 KB) - added by dd32 17 months ago.

Download all attachments as: .zip

Change History (57)

#1 @SergeyBiryukov
18 months ago

  • Milestone changed from Awaiting Review to 4.4.1

#2 @SergeyBiryukov
18 months ago

#34968 was marked as a duplicate.

@rommelxcastro
18 months ago

define order by on redirect_guess_404_permalink

#3 @rommelxcastro
18 months ago

  • Keywords has-patch added

patch added.

ORDER BY post_name ASC

added to query

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


18 months ago

#5 in reply to: ↑ description @SergeyBiryukov
18 months 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:

  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.

#6 @douglsmith
18 months ago

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

#7 @bobbingwide
18 months 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.

Last edited 18 months ago by bobbingwide (previous) (diff)

#8 follow-up: @JureCuhalev
18 months 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.

Last edited 18 months ago by SergeyBiryukov (previous) (diff)

#9 @bobbingwide
18 months ago

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

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

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

#10 in reply to: ↑ 8 @bobbingwide
18 months 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 @JureCuhalev
18 months ago

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.

Last edited 18 months ago by JureCuhalev (previous) (diff)

#12 @nacin
18 months ago

  • Severity changed from normal to critical

#13 @SergeyBiryukov
18 months ago

  • Owner set to pento
  • Status changed from new to assigned

@pento
18 months ago

#14 @pento
18 months ago

  • Keywords needs-unit-tests needs-testing added

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

Last edited 18 months ago by pento (previous) (diff)

@pento
18 months ago

#15 @pento
18 months 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 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.

#16 @jobst
18 months ago

Why is it called "_wp_old_slug", shouldn't it be called "_wp_slug"?
I am curious to know, is this written anywhere?

Last edited 18 months ago by jobst (previous) (diff)

#17 follow-up: @pento
18 months 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.

#18 @jobst
18 months ago

#35114 was marked as a duplicate.

#19 in reply to: ↑ 17 @jobst
18 months 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: @pento
18 months 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 @jobst
18 months 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.


18 months ago

#23 in reply to: ↑ 20 @bobbingwide
18 months 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 @bobbingwide
18 months 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.


18 months ago

#26 @bobbingwide
18 months 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.

#27 @pento
18 months ago

#35012 was marked as a duplicate.

#28 follow-ups: @bobbingwide
18 months 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 @jobst
18 months 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.

Last edited 17 months ago by jobst (previous) (diff)

#30 in reply to: ↑ 28 @bobbingwide
18 months 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 @strategio
18 months 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.


18 months ago

#33 @SergeyBiryukov
17 months ago

#35184 was marked as a duplicate.

#34 @SergeyBiryukov
17 months ago

#35189 was marked as a duplicate.

#35 @gnotaras
17 months 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: @pento
17 months 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.


17 months ago

#38 in reply to: ↑ 36 @jobst
17 months 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 @pento
17 months 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 @jobst
17 months 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 @pento
17 months 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.

#42 @jorbin
17 months ago

@pento what do you need to get this committable?

@dd32
17 months ago

#43 @dd32
17 months 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.


17 months ago

#45 @jorbin
17 months 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.


17 months ago

#47 @pento
17 months ago

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

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.

#48 @pento
17 months ago

  • Keywords fixed-major added; needs-unit-tests needs-testing removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

#49 @pento
17 months ago

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

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.

#50 @Clorith
17 months ago

#35302 was marked as a duplicate.

#51 follow-up: @cbaldelomar
17 months 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 @ocean90
17 months ago

  • Keywords dev-feedback removed
  • Resolution set to fixed
  • Status changed from reopened to closed

See #35344.

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


17 months ago

Note: See TracTickets for help on using tickets.