Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#35012 closed defect (bug) (duplicate)

wp_old_slug_redirect can cause redirect loop

Reported by: vdwijngaert's profile vdwijngaert Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.4
Component: Canonical Keywords: has-patch 2nd-opinion needs-unit-tests
Focuses: Cc:

Description (last modified by ocean90)

At least one known issue arises since [34659].

I noticed this problem when working with the latest version of the WPML Multilingual CMS plugin (v3.3.3) in combination with WP 4.4 (same issues with trunk version).

When the translation of a post had the same slug (example.com/hello-world) as the original post (after duplicating it to the translation) and the slug is then updated to reflect the translation (example.com/fr/bonjour-le-monde), the old slug is saved as _wp_old_slug.

If we then navigate to example.com/hello-world, wp_old_slug_redirect() tries to do a 301 redirect to example.com/hello-world, causing a redirect loop. See this post on the WPML support forums: https://wpml.org/forums/topic/hi-several-posts-on-my-website-return-a-page-too-many-redirects/.

I don't know if what WPML does with the slug system (they are supposed to be unique) is supposed to be possible, but wp_old_slug_redirect() shouldn't be causing a redirect loop nonetheless. I suspect that at this very moment it is causing problems with WPML users worldwide, and possibly other plugins that do something similar as well.

In the past this was handled by the "is_404()" check on line 4733 (now line 4989), but that's no longer there. That's why this bug surfaces now. Shouldn't WordPress be able to detect or even prevent this redirect loop?

Right now I have a temporary fix for the websites I'm running. It checks whether or not the redirect is to the current permalink and returns an empty string if that is the case.

<?php
add_filter('old_slug_redirect_url', function($link) {
        if($link === get_the_permalink()) {
                return '';
        };

        return $link;
});


This issue might possibly be related: #23074.

Thanks for looking in to this! Feel free to ask for more details.

Attachments (3)

35012.diff (455 bytes) - added by vdwijngaert 9 years ago.
Adds check for redirect loop.
query.php.patch (346 bytes) - added by mynamevenu24 9 years ago.
35012.patch (346 bytes) - added by mynamevenu24 9 years ago.

Download all attachments as: .zip

Change History (18)

#1 @ocean90
9 years ago

  • Description modified (diff)
  • Version changed from trunk to 4.4

#2 @SergeyBiryukov
9 years ago

  • Milestone changed from Awaiting Review to 4.4.1

Hi @vdwijngaert, welcome to Trac!

Thanks for the report, seeing a similar issue on support forums. Moving for investigation.

#3 follow-up: @SergeyBiryukov
9 years ago

Also seeing a report about an unexpected redirect (not sure if it's related to [34659] yet):

/sajty-nedvizhimosti-ukrainy-top-samye-populyarnye/sajty-nedvizhimosti-v-2012.

Both are published posts.

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

#4 in reply to: ↑ 3 ; follow-up: @vdwijngaert
9 years ago

Replying to SergeyBiryukov:

Also seeing a report about an unexpected redirect (not sure if it's related to [34659] yet):
/sajty-nedvizhimosti-ukrainy-top-samye-populyarnye/sajty-nedvizhimosti-v-2012.

Doesn't look like a redirect loop. Got a link to the report?

#5 in reply to: ↑ 4 @SergeyBiryukov
9 years ago

Replying to vdwijngaert:

Doesn't look like a redirect loop. Got a link to the report?

It's in Russian :) https://ru.forums.wordpress.org/topic/Редирект-страниц-в-wordpress-44

Redirect loop was mentioned in that thread as well, so I've just added it for reference in case it turns out to be related.

@vdwijngaert
9 years ago

Adds check for redirect loop.

#6 @vdwijngaert
9 years ago

  • Keywords dev-feedback has-patch 2nd-opinion added

I added a patch, it checks whether the $link equals get_post_permalink() to detect the most simple redirect loop: redirecting to the same URL. I'm not sure if get_post_permalink is the best way to get the current URL, but it seems to suit the needs here. Feedback is welcome!

#7 @swissspidy
9 years ago

  • Keywords needs-unit-tests added; dev-feedback removed

#8 @SergeyBiryukov
9 years ago

#35070 was marked as a duplicate.

#9 @apsolut
9 years ago

Amazing.. default cat didnt work, translated ones did.. now both work WP 4.4 & WPML 3.3.3

This ticket was mentioned in Slack in #core by swissspidy. View the logs.


9 years ago

@mynamevenu24
9 years ago

This ticket was mentioned in Slack in #core by mynamvenu24. View the logs.


9 years ago

#12 @pento
9 years ago

Could folks running into this bug please test if 35031.2.diff fixes it for them? It's possible that this is the same bug.

#13 @bobbingwide
9 years ago

See my comments in #35031 suggesting how this problem could be reproduced.

I achieved it just by using the post editor. With two posts.
id 717, title Cycle 1, slug cycle-2-2
id 720, title Cycle 2, slug cycle-2

values for _wp_old_slug post_meta for these two posts
717: cycle-1 and cycle-2
720: cycle-1 and cycle-2-2

visiting 2015/12/cycle-1 gives a redirect loop
visiting 2015/12/cycle-2 does also
visiting 2015/12/cycle-2-2 ditto

The good news is pento's first fix resolves this issue.
The better news is I believe it can be improved to return if there's one result in the query.
Why perform the same query to get the same result?


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

This ticket was mentioned in Slack in #core by bobbingwide. View the logs.


9 years ago

#15 @pento
9 years ago

  • Milestone 4.4.1 deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Cool, let's continue the effort over on #35031.

Note: See TracTickets for help on using tickets.