Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#35031 closed defect (bug) (fixed)

wp_old_slug_redirect() in 4.4 redirecting existing posts

Reported by: douglsmith's profile douglsmith Owned by: pento's profile 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 9 years ago.
define order by on redirect_guess_404_permalink
35031.diff (758 bytes) - added by pento 9 years ago.
35031.2.diff (464 bytes) - added by pento 9 years ago.
35031.3.diff (1.1 KB) - added by dd32 9 years ago.

Download all attachments as: .zip

Change History (57)

#1 @SergeyBiryukov
9 years ago

  • Milestone changed from Awaiting Review to 4.4.1

#2 @SergeyBiryukov
9 years ago

#34968 was marked as a duplicate.

@rommelxcastro
9 years ago

define order by on redirect_guess_404_permalink

#3 @rommelxcastro
9 years 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.


9 years ago

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

  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
9 years ago

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

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

Last edited 9 years ago by bobbingwide (previous) (diff)

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

Last edited 9 years ago by SergeyBiryukov (previous) (diff)

#9 @bobbingwide
9 years 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
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 @JureCuhalev
9 years 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 9 years ago by JureCuhalev (previous) (diff)

#12 @nacin
9 years ago

  • Severity changed from normal to critical

#13 @SergeyBiryukov
9 years ago

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

@pento
9 years ago

#14 @pento
9 years ago

  • Keywords needs-unit-tests needs-testing added

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

Version 0, edited 9 years ago by pento (next)

@pento
9 years ago

#15 @pento
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 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
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?

Last edited 9 years ago by jobst (previous) (diff)

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

#18 @jobst
9 years ago

#35114 was marked as a duplicate.

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

#27 @pento
9 years ago

#35012 was marked as a duplicate.

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

Last edited 9 years ago by jobst (previous) (diff)

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

#33 @SergeyBiryukov
9 years ago

#35184 was marked as a duplicate.

#34 @SergeyBiryukov
9 years ago

#35189 was marked as a duplicate.

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

#42 @jorbin
9 years ago

@pento what do you need to get this committable?

@dd32
9 years ago

#43 @dd32
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 @jorbin
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

#47 @pento
9 years 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
9 years ago

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

#49 @pento
9 years 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
9 years ago

#35302 was marked as a duplicate.

#51 follow-up: @cbaldelomar
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 @ocean90
9 years 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.


9 years ago

Note: See TracTickets for help on using tickets.