Make WordPress Core

Opened 11 years ago

Last modified 6 weeks ago

#23074 reopened defect (bug)

Changing post's URL and then setting it back causes redirect loop (wp_old_slug_redirect)

Reported by: vbuterin's profile vbuterin Owned by:
Milestone: Future Release Priority: normal
Severity: major Version: 3.5
Component: Canonical Keywords: has-patch needs-refresh has-unit-tests
Focuses: Cc:

Description

This morning, I changed the URL of one of my posts, but then changed my mind and set it back. When you change the URL of your post, WP automatically creates a redirect from the old page to the new page, which is good, but in this case it led to an unintended consequence: after I changed the link from A to B and then back to A, it set up a redirect from A to B and a redirect from B to A, causing anyone trying to access A or B to end up in a redirect loop between the two (or possibly it created a redirect loop from A to A; I'm not sure). It should check for such loops when you update the URL to avoid this problem.

Attachments (10)

query.php.patch (824 bytes) - added by kitchin 10 years ago.
Minimal fix
post.php.patch (1.7 KB) - added by kitchin 10 years ago.
De-crufting fix for wp_check_for_changed_slugs()
post.php.2.patch (1.7 KB) - added by kitchin 10 years ago.
typo fix to previous patch for wp_check_for_changed_slugs
combined.patch (2.6 KB) - added by kitchin 10 years ago.
Combine the minimal patch with the post patch and update the comment (to "legacy data")
combined.2.patch (2.2 KB) - added by kitchin 10 years ago.
maintains data correctly
23074.diff (2.3 KB) - added by MikeHansenMe 10 years ago.
Allow Private status to save old slug. Unit Tests.
23074.2.diff (2.3 KB) - added by MikeHansenMe 10 years ago.
Better Unit test.
23074.3.diff (2.4 KB) - added by MikeHansenMe 10 years ago.
Better Unit test for real this time
23074.4.diff (2.1 KB) - added by MikeHansenMe 8 years ago.
23074.5.diff (2.1 KB) - added by jared_smith 8 years ago.
Updated patch with code changes and unit tests

Download all attachments as: .zip

Change History (34)

#1 @miqrogroove
11 years ago

  • Keywords reporter-feedback added

I couldn't reproduce this problem. I confirmed link A will forward to link B, but after changing the slug back to link A everything worked as expected.

Could you provide an example URL and the exact steps that created a redirect loop?

#2 @SergeyBiryukov
11 years ago

  • Component changed from General to Canonical

Could not reproduce either. _wp_old_slug is properly updated to reflect the changes.

Does this still happen with all plugins deactivated and using Twenty Twelve?

#3 follow-up: @nacin
11 years ago

  • Keywords close added

This sounds like a browser is caching both redirects. Little we could do here.

#4 @c3mdigital
11 years ago

  • Keywords reporter-feedback close removed
  • Resolution set to invalid
  • Status changed from new to closed

I could no reproduce this. Changed url then visited old and new url both worked as expected. changed it back to same exact url as original url, visited both urls again and both worked as expected.

#5 in reply to: ↑ 3 @nacin
11 years ago

  • Resolution changed from invalid to wontfix

Replying to nacin:

This sounds like a browser is caching both redirects. Little we could do here.

Restating that this is the issue.

#6 @nacin
11 years ago

  • Milestone Awaiting Review deleted

#7 @kitchin
10 years ago

  • Keywords has-patch 2nd-opinion added
  • Resolution wontfix deleted
  • Status changed from closed to reopened
  • Summary changed from Changing post's URL and then setting it back causes redirect loop to Changing post's URL and then setting it back causes redirect loop (wp_old_slug_redirect)

Here is how to reproduce. It may seem like a corner case, but I advocate fixing it (patch attached) because

  1. It is hard to debug.
  2. Docs and forum support for redirect loops (on custom post types) are all about calling flush_rewrite_rules() correctly, an unrelated issue.

Steps to reproduce (with permalinks set):

  1. create a post 'foo'
  2. change the permalink to 'foobar'
  3. make the post Private
  4. change the permalink back to 'foo'
  5. open the post in non-logged-in browser
  6. result: redirect loop notice (or blank screen depending on browser)

Minimal fix attached. It's a one line SQL clause to prevent an obvious redirect loop in wp_old_slug_redirect().

Hope it's OK if I change the keywords and resolution here. I can't change the version, but it affects 3.8.1 and trunk has the same code.

Last edited 10 years ago by kitchin (previous) (diff)

@kitchin
10 years ago

Minimal fix

@kitchin
10 years ago

De-crufting fix for wp_check_for_changed_slugs()

@kitchin
10 years ago

typo fix to previous patch for wp_check_for_changed_slugs

#8 @kitchin
10 years ago

The patch for wp_check_for_changed_slugs() in post.php is here if you want it, and it explains the source of the problem. But I would skip it, since it's hard to test and it removes '_old_slug' values in some cases where you might want to keep them. It is better than the current code in preventing cruft, though.

Last edited 10 years ago by kitchin (previous) (diff)

@kitchin
10 years ago

Combine the minimal patch with the post patch and update the comment (to "legacy data")

@kitchin
10 years ago

maintains data correctly

#9 @kitchin
10 years ago

The new combined.2.patch does wp_check_for_changed_slugs() correctly and I think should be landed. Instead of cleaning up the data after all the early returns (current code), it cleans the data first. Compared to my previous patch, it does not delete everything when the post is Private.

#10 @samuelsidler
10 years ago

  • Milestone set to Awaiting Review
  • Severity changed from normal to critical

#11 @nacin
10 years ago

  • Milestone changed from Awaiting Review to 3.9
  • Severity changed from critical to major

Looks good, kitchin!

#12 @samuelsidler
10 years ago

  • Keywords 2nd-opinion removed

#13 follow-up: @nacin
10 years ago

  • Keywords needs-unit-tests 4.0-early added
  • Milestone changed from 3.9 to Future Release

I like combined.2.patch but after studying this a few times over the last few weeks, I've come to the conclusion it needs unit tests.

#14 in reply to: ↑ 13 @jtsternberg
10 years ago

Replying to nacin:

I like combined.2.patch but after studying this a few times over the last few weeks, I've come to the conclusion it needs unit tests.

Tried to write some unit tests (working on my unit-testing chops), but am unsure what I'm testing against. Nacin, do you have a particular logic in mind?

#15 follow-up: @MikeHansenMe
10 years ago

Looking at the code in wp_check_for_changed_slugs I think the problem is that the function returns before it ever gets to add/remove slugs because the post is not publish status. I have a patch for wp_check_for_changed_slug and some unit tests.

@MikeHansenMe
10 years ago

Allow Private status to save old slug. Unit Tests.

@MikeHansenMe
10 years ago

Better Unit test.

@MikeHansenMe
10 years ago

Better Unit test for real this time

#16 @jared_smith
9 years ago

  • Keywords needs-unit-tests removed

Removing the "needs-unit-tests" keywords, as I can see that Mike wrote unit tests for this ticket.

#17 @chriscct7
8 years ago

  • Keywords needs-refresh added; 4.0-early removed
  • Severity changed from major to normal

#18 @johnbillion
8 years ago

  • Severity changed from normal to major

@MikeHansenMe
8 years ago

#19 @MikeHansenMe
8 years ago

  • Keywords needs-refresh removed

#20 @jared_smith
8 years ago

  • Keywords needs-refresh has-unit-tests added

Re-rolled this patch to bring it up to current TRUNK. This patch, however, causes another unit test to fail:

1) Tests_Rewrite_OldSlugRedirect::test_old_slug_redirect_attachment
Failed asserting that null matches expected 'http://example.org/bar-baz/the-attachment/'.

wordpress-trunk/tests/phpunit/tests/rewrite/oldSlugRedirect.php:130

@jared_smith
8 years ago

Updated patch with code changes and unit tests

#21 @SergeyBiryukov
4 months ago

  • Milestone set to 6.5

#22 @SergeyBiryukov
4 months ago

Found this ticket in a coding session with @poena and @afercia while testing the patch on #59866.

Specifically, comment:15 caught our attention. Moving to the milestone for further review.

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

#23 in reply to: ↑ 15 @afercia
4 months ago

Replying to MikeHansenMe:

Looking at the code in wp_check_for_changed_slugs I think the problem is that the function returns before it ever gets to add/remove slugs because the post is not publish status. I have a patch for wp_check_for_changed_slug and some unit tests.

Kudos to @MikeHansenMe for discovering this. Unfortunately it's 9 years that this needs to be fixed and it's still buggy. To add some more details:

This condition in wp_check_for_changed_slugs() appears to be based on an incorrect assumption:

// We're only concerned with published, non-hierarchical objects.
if ( ! ( 'publish' === $post->post_status || ( 'attachment' === get_post_type( $post ) && 'inherit' === $post->post_status ) ) || is_post_type_hierarchical( $post->post_type ) ) {
	return;
}

Private posts

Private posts can be viewed by logged-in users. As such, redirects need to work for private posts as well. The problem is that a slug change isn't added/removed for private posts. No slug change detection, no redirect.
It is possible that condition dates before the introduction or private posts, not sure. Regardless it needs to be changed to take into account all cases were a post is viewable and redirects are expected to work.

Other cases

I may be missing many things here but:

  • Why attachment pages are excluded? Users can change the attachment page slug in the admin and in that case no redirect will happen. Worth reminding attachment pages can be re-enabled via the filter pre_option_wp_attachment_pages_enabled.
  • Why hierarchical posts (e.g. pages) are excluded? After changing a page slug, no redirect happens.

#24 @swissspidy
6 weeks ago

  • Milestone changed from 6.5 to Future Release
Note: See TracTickets for help on using tickets.