Make WordPress Core

Opened 17 years ago

Closed 17 years ago

Last modified 17 years ago

#5766 closed defect (bug) (wontfix)

endless redirect loop

Reported by: pixline's profile pixline Owned by: markjaquith's profile markjaquith
Milestone: Priority: normal
Severity: normal Version: 2.3.2
Component: General Keywords: has-patch needs-testing
Focuses: Cc:

Description

Gengo plugins used to add language codes to URLs (like /page/2/en/ ) but actually it triggers in WP 2.3.2 (and trunk) an endless loop, the same way of http://trac.wordpress.org/ticket/5203

Adding || is_paged() to canonical.php, line #7, stops this endless loop and make paged archives work again.

You can test this issue using http://gengo-wp23.googlecode.com/files/gengo-091a8.zip , I could not find a test case without a plugin, sorry.

Attachments (1)

canonical.php.diff (646 bytes) - added by pixline 17 years ago.
patch for canonical.php (trunk version)

Download all attachments as: .zip

Change History (11)

#1 @lloydbudd
17 years ago

  • Keywords needs-patch added
  • Milestone changed from 2.6 to 2.5

@pixline
17 years ago

patch for canonical.php (trunk version)

#2 @pixline
17 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

#3 @jhodgdon
17 years ago

Just a comment. In the Language Switcher (another multilingual plugin) I came across some issues like this, and got around it by adding language portions of the URL *before* the /page/2/. So the URLs look like this:

(siteurl)/langswitch_lang/es/page/2/

The issue I was working around was that some of the code for figuring out page/# assumed that the page/# was at the end of the URL, and it seemed to be pretty fundamental to the code rather than something that could be fixed with an easy patch.

So my recommendation would be to try getting your language suffixes in before the /page/N part of the URL. It's a bit more complex, programming-wise, but it does work better.

#4 @pixline
17 years ago

I'll try to find a way to do this, but this would break a lot existing websites who have this permalink structure, and this would probably require a whole rewriting of some functions I'd prefer to leave untouched :-)

Can this patch leave in standby some time?

#5 @ryan
17 years ago

  • Owner changed from anonymous to markjaquith

#6 @jhodgdon
17 years ago

Just a little more on this.

The problem that Pixline identified comes up in wp-includes/canonical.php, where the function redirect_canonical gets called to turn a possibly badly-formatted URL into a "canonical" URL. This function rewrites the URL so that everything is "canonical".

The issue is that if you have a url like (site url)/page/2/(suffix), that rewrite function screws it up, because the code that handles paging tacitly assumes that the page section is at the end of the URL. The offending line of code is here:

$paged_redirect['path'] = 
   preg_replace('|/page/[0-9]+?(/+)?$|', '/', 
   $paged_redirect['path']); // strip off any existing paging

The regex used on that line only strips off /page/N when it is at the end of the URL, and then lower down in the function, it adds in /page/N at the end of the URL, and you end up with a URL like (site url)/page/2/(suffix)/page/2, which is no good at all.

At some point recently, when one of my Language Switcher users pointed out that the URL suffixes my plugin was appending were doing this, I tried patching it so that it would work with page/N not at the end of the URL, but I ran into other problems, which I think had to do with rewrite rules (even though I had rewrite rules in Language Switcher that would handle pages not being at the end of the URL, something didn't work correctly). My vague recollection was that fixing this issue at the WP level would have involved a large patch, and waiting for a new version to come out, so I elected to redo Language Switcher so that the URLs were in a more "canonical" form (i.e. LS puts URLs into format lang/en/page/2 instead of page/2/lang/en).

So my guess is that Pixline's patch might break something else, and recommend it be tested extensively before committing it.

By the way, URLs like page/2/xyz/pdq used to work in older versions of WordPress -- not sure which -- the change so they didn't work may have been introduced in [5974], but I am not certain about that.

Pixline - contact me off-line (or download Language Switcher from poplarware.com) if you want code samples. LS is GPL-licensed, so feel free to reuse its URL rewriting mechanism.

#7 @markjaquith
17 years ago

  • Status changed from new to assigned

I've rearranged some code that will allow you to use the redirect_canonical filter (which gets passed the redirect URL and the request URL) to cancel a canonical redirect by returning FALSE.

So you could just look for /page/N/en original requests and return FALSE. Commit coming.

#8 @markjaquith
17 years ago

(In [6743]) Allow redirect_canonical filter to cancel a redirect (just return FALSE). see #5766

#9 @markjaquith
17 years ago

  • Resolution set to wontfix
  • Status changed from assigned to closed

#10 @Nazgul
17 years ago

  • Milestone 2.5 deleted
Note: See TracTickets for help on using tickets.