WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 6 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 4.0-early
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 (8)

query.php.patch (824 bytes) - added by kitchin 15 months ago.
Minimal fix
post.php.patch (1.7 KB) - added by kitchin 15 months ago.
De-crufting fix for wp_check_for_changed_slugs()
post.php.2.patch (1.7 KB) - added by kitchin 15 months ago.
typo fix to previous patch for wp_check_for_changed_slugs
combined.patch (2.6 KB) - added by kitchin 15 months 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 15 months ago.
maintains data correctly
23074.diff (2.3 KB) - added by MikeHansenMe 10 months ago.
Allow Private status to save old slug. Unit Tests.
23074.2.diff (2.3 KB) - added by MikeHansenMe 10 months ago.
Better Unit test.
23074.3.diff (2.4 KB) - added by MikeHansenMe 10 months ago.
Better Unit test for real this time

Download all attachments as: .zip

Change History (24)

comment:1 @miqrogroove2 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?

comment:2 @SergeyBiryukov2 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?

comment:3 follow-up: @nacin2 years ago

  • Keywords close added

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

comment:4 @c3mdigital20 months 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.

comment:5 in reply to: ↑ 3 @nacin20 months 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.

comment:6 @nacin20 months ago

  • Milestone Awaiting Review deleted

comment:7 @kitchin15 months 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 15 months ago by kitchin (previous) (diff)

@kitchin15 months ago

Minimal fix

@kitchin15 months ago

De-crufting fix for wp_check_for_changed_slugs()

@kitchin15 months ago

typo fix to previous patch for wp_check_for_changed_slugs

comment:8 @kitchin15 months 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 15 months ago by kitchin (previous) (diff)

@kitchin15 months ago

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

@kitchin15 months ago

maintains data correctly

comment:9 @kitchin15 months 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.

comment:10 @samuelsidler15 months ago

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

comment:11 @nacin15 months ago

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

Looks good, kitchin!

comment:12 @samuelsidler13 months ago

  • Keywords 2nd-opinion removed

comment:13 follow-up: @nacin13 months 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.

comment:14 in reply to: ↑ 13 @jtsternberg11 months 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?

comment:15 @MikeHansenMe10 months 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.

@MikeHansenMe10 months ago

Allow Private status to save old slug. Unit Tests.

@MikeHansenMe10 months ago

Better Unit test.

@MikeHansenMe10 months ago

Better Unit test for real this time

comment:16 @jared_smith6 months ago

  • Keywords needs-unit-tests removed

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

Note: See TracTickets for help on using tickets.