WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 12 months ago

#23074 reopened defect (bug)

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

Reported by: 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 3 years ago.
Minimal fix
post.php.patch (1.7 KB) - added by kitchin 3 years ago.
De-crufting fix for wp_check_for_changed_slugs()
post.php.2.patch (1.7 KB) - added by kitchin 3 years ago.
typo fix to previous patch for wp_check_for_changed_slugs
combined.patch (2.6 KB) - added by kitchin 3 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 3 years ago.
maintains data correctly
23074.diff (2.3 KB) - added by MikeHansenMe 2 years ago.
Allow Private status to save old slug. Unit Tests.
23074.2.diff (2.3 KB) - added by MikeHansenMe 2 years ago.
Better Unit test.
23074.3.diff (2.4 KB) - added by MikeHansenMe 2 years ago.
Better Unit test for real this time
23074.4.diff (2.1 KB) - added by MikeHansenMe 13 months ago.
23074.5.diff (2.1 KB) - added by jared_smith 12 months ago.
Updated patch with code changes and unit tests

Download all attachments as: .zip

Change History (30)

#1 @miqrogroove
4 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
4 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
4 years ago

  • Keywords close added

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

#4 @c3mdigital
3 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
3 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
3 years ago

  • Milestone Awaiting Review deleted

#7 @kitchin
3 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 3 years ago by kitchin (previous) (diff)

@kitchin
3 years ago

Minimal fix

@kitchin
3 years ago

De-crufting fix for wp_check_for_changed_slugs()

@kitchin
3 years ago

typo fix to previous patch for wp_check_for_changed_slugs

#8 @kitchin
3 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 3 years ago by kitchin (previous) (diff)

@kitchin
3 years ago

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

@kitchin
3 years ago

maintains data correctly

#9 @kitchin
3 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
3 years ago

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

#11 @nacin
3 years ago

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

Looks good, kitchin!

#12 @samuelsidler
3 years ago

  • Keywords 2nd-opinion removed

#13 follow-up: @nacin
3 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
3 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 @MikeHansenMe
2 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
2 years ago

Allow Private status to save old slug. Unit Tests.

@MikeHansenMe
2 years ago

Better Unit test.

@MikeHansenMe
2 years ago

Better Unit test for real this time

#16 @jared_smith
2 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
13 months ago

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

#18 @johnbillion
13 months ago

  • Severity changed from normal to major

#19 @MikeHansenMe
13 months ago

  • Keywords needs-refresh removed

#20 @jared_smith
12 months 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
12 months ago

Updated patch with code changes and unit tests

Note: See TracTickets for help on using tickets.