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

[vbuterin]

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.

[miqrogroove]

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?

[SergeyBiryukov]

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?

[nacin]

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

[c3mdigital]

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.

[nacin]

Replying to nacin:

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

Restating that this is the issue.

[kitchin]

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

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

[kitchin]

Minimal fix

[kitchin]

De-crufting fix for wp_check_for_changed_slugs()

[kitchin]

typo fix to previous patch for wp_check_for_changed_slugs

[kitchin]

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.

[kitchin]

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

[kitchin]

maintains data correctly

[kitchin]

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.

[nacin]

Looks good, kitchin!

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

[jtsternberg]

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?

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

[MikeHansenMe]

Allow Private status to save old slug. Unit Tests.

[MikeHansenMe]

Better Unit test.

[MikeHansenMe]

Better Unit test for real this time

[jared_smith]

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

[jared_smith]

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]

Updated patch with code changes and unit tests

[SergeyBiryukov]

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.

[afercia]

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.