Make WordPress Core

Opened 18 years ago

Closed 18 years ago

#3964 closed defect (bug) (fixed)

Aggressive matching in rewrite.php leads to "nothing matches" errors with custom rewrite endpoints

Reported by: jhodgdon's profile jhodgdon Owned by: westi's profile westi
Milestone: 2.2 Priority: normal
Severity: normal Version: 2.1.2
Component: General Keywords: rewrite reporter-feedback has-patch 2nd-opinion
Focuses: Cc:

Description (last modified by westi)

In a plugin, I call add_rewrite_endpoint to add a custom endpoint to the rewrite rules, and then cause a refresh of the rules by calling $wp_rewrite->flush_rules(). I add a suffix endpoint called "foo" to the rewrite rules, for a variety of situations, including post permalinks (in my case, "foo" is actually a language choosing parameter).

My permalink structure is set to /%category%/%postname%/

So, it's working fine on a post, category, page, etc. -- my suffixes are recognized and I can set the language and then find the post. But when I go to the home page and add suffixes, to get a permalink like:

http://www.example.com/blogdir/foo/param

the query fails, because the above it matches the post-with-page-suffix rewrite rule:

(.+?)/([^/]+)(/[0-9]+)?/?$

and WordPress thinks it is the post in category "foo" with slug "param" (which doesn't exist).

It's also a problem on feeds, where

http://www.example.com/blogdir/feed/foo/param

matches the same matching rule, and WordPress thinks it is the post in category "feed/foo", with slug "param". That .+ at the beginning of the rule is pretty permissive.

I do have a rule:

foo(/(.*))?/?$

but it is farther down in the list than the rule above, and so the rule above takes precidence.

I am not sure how to fix this... any ideas? The API doesn't allow for upping the precedence of rules. For now I have just made my plugin use a GET instead of permalink endpoint for the blog's home page and for feeds, but it is rather annoying.

Attachments (2)

endpoints.txt (5.5 KB) - added by jhodgdon 18 years ago.
Sample code and printout of the rewrite rules generated
fix_ep_root_rewrite.diff (1.2 KB) - added by westi 18 years ago.
Fix EP_ROOT by moving the endpoint code outside of the check for any sub directories.

Download all attachments as: .zip

Change History (22)

#1 @jhodgdon
18 years ago

  • Component changed from Administration to General
  • Priority changed from low to normal
  • Version set to 2.1.2

Forgot to mention, tested in 2.1.1 release, as well as [5012].

#2 @jhodgdon
18 years ago

  • Keywords rewrite added

#3 @foolswisdom
18 years ago

  • Milestone changed from 2.3 to 2.2

#4 @westi
18 years ago

  • Owner changed from anonymous to westi
  • Status changed from new to assigned

Could you attach some example code and/or a print_r of the generated rules for me to review what you are trying to achieve.

Reading through the rewrite code the permalink rules should come last in the array and so the extra endpoints should be matched earlier (or so i thought).

#5 @westi
18 years ago

  • Description modified (diff)

#6 @majelbstoat
18 years ago

It's not a solution to the underlying problem you describe, but you can use the rewrite_rules_array filter and append the permalinks that way. That was the approach I took with Gengo, which was written before the new endpoint stuff was around. You can control the order of the rules that you return, as well as choosing which ones to append. It's probably not quite as easy as using the new API, but it's been working for a long time...

@jhodgdon
18 years ago

Sample code and printout of the rewrite rules generated

#7 @jhodgdon
18 years ago

Actually, I was wrong, and I don't think my endpoints created a matching rule that would match my special endpoint when it is appended directly to the blog home URL.

#8 @jhodgdon
18 years ago

Shouldn't they have created a rule though?

#9 follow-up: @jhodgdon
18 years ago

One furthere note. Even with the call to add_rewrite_endpoints changed to

add_rewrite_endpoint( $langSwitchPermalink, 4095 );

(which should add all possible endpoints) I still do not get a matching rule that will match

http://www.example.com/blogdir/foo/param

(except for the erroneous match on category/postslug) So maybe that is all there is to the bug, that the endpoints generated are not sufficient to match the main blog URL or the feed with my endpoint suffixes. Should I just add them manually with a different function call? Should I have to?

#10 in reply to: ↑ 9 @westi
18 years ago

Replying to jhodgdon:

One furthere note. Even with the call to add_rewrite_endpoints changed to

add_rewrite_endpoint( $langSwitchPermalink, 4095 );

(which should add all possible endpoints) I still do not get a matching rule that will match

http://www.example.com/blogdir/foo/param

(except for the erroneous match on category/postslug) So maybe that is all there is to the bug, that the endpoints generated are not sufficient to match the main blog URL or the feed with my endpoint suffixes. Should I just add them manually with a different function call? Should I have to?

Looking through your generated links this looks like a definete bug.

If I read right, it seems the only rules generated for EP_ROOT are:

feed/(feed|rdf|rss|rss2|atom)/?$
(feed|rdf|rss|rss2|atom)/?$
page/?([0-9]{1,})/?$

Which is definetly wrong from the example code you attached.

I will have a look into working out what is going wrong in generate_rewrite_rules.

#11 @jhodgdon
18 years ago

Now that we have narrowed it down to this (thanks!), I can try to patch it myself. Assign it to me if you like; if not I will assume you prefer to do it.

#12 @westi
18 years ago

  • Keywords reporter-feedback 2nd-opinion added

Right I've had a look at this and EP_ROOT is definetly broken.

However, I think there is a mismatch between what is available with the addition of extra endpoints and what you expect to happen.

From reading the code I believe the following is true:

  1. The are a number of builtin endpoints = feeds, trackback, paging
  2. You can add you own endpoints at the same level.
  3. These endpoints are then added to the places you specify e.g. EP_ROOT - Therefore you would only get the following rules with EP_ROOT fixed:
feed/(feed|rdf|rss|rss2|atom)/?$
(feed|rdf|rss|rss2|atom)/?$
page/?([0-9]{1,})/?$
new_endpoint(/(.*))?/?$

I am not sure that a new endpoint is what you want to use in order to be able to add language tags to every url.

Do you not want a new rewrite tag? - e.g. %lang% which maps to ?lang= and is used in the permalink structure. This would then get added for all permalink urls before the endpoints.

Otherwise you need to be able to add endpoints to endpoints e.g. we would need EP_FEED, EP_PAGING and the ability to extend EP_ for new endpoints that wanted to support sub endpoints.

@westi
18 years ago

Fix EP_ROOT by moving the endpoint code outside of the check for any sub directories.

#13 @westi
18 years ago

  • Keywords has-patch added

The above patch fixes EP_ROOT to do what it should do.

Test Plugin:

<?php
/*
Plugin Name: Rewrite 
Description: Tests rewrite code.
Version: 0.1
*/
function test_add_endpoint()
{
    global $wp_rewrite;
    add_rewrite_endpoint('test',EP_ALL);
    ?><!--<?php
    print_r($wp_rewrite->generate_rewrite_rules($wp_rewrite->root . '/', EP_ROOT));
    print_r($wp_rewrite->generate_rewrite_rules($wp_rewrite->permalink_structure, EP_PERMALINK));
    ?>--><?php
}

add_action('init', 'test_add_endpoint');
?>

Output:

(
    [feed/(feed|rdf|rss|rss2|atom)/?$] => index.php?&feed=$1
    [(feed|rdf|rss|rss2|atom)/?$] => index.php?&feed=$1
    [page/?([0-9]{1,})/?$] => index.php?&paged=$1
    [test(/(.*))?/?$] => index.php?&test=$2
)
Array
(
    [(.+?)/([0-9]{4})/([^/]+)/trackback/?$] => index.php?category_name=$1&year=$2&name=$3&tb=1
    [(.+?)/([0-9]{4})/([^/]+)/feed/(feed|rdf|rss|rss2|atom)/?$] => index.php?category_name=$1&year=$2&name=$3&feed=$4
    [(.+?)/([0-9]{4})/([^/]+)/(feed|rdf|rss|rss2|atom)/?$] => index.php?category_name=$1&year=$2&name=$3&feed=$4
    [(.+?)/([0-9]{4})/([^/]+)/page/?([0-9]{1,})/?$] => index.php?category_name=$1&year=$2&name=$3&paged=$4
    [(.+?)/([0-9]{4})/([^/]+)/test(/(.*))?/?$] => index.php?category_name=$1&year=$2&name=$3&test=$5
    [.+?/[0-9]{4}/[^/]+/([^/]+)/test(/(.*))?/?$] => index.php?attachment=$1?&test=$2
    [.+?/[0-9]{4}/[^/]+/attachment/([^/]+)/test(/(.*))?/?$] => index.php?attachment=$1?&test=$2
    [(.+?)/([0-9]{4})/([^/]+)(/[0-9]+)?/?$] => index.php?category_name=$1&year=$2&name=$3&page=$4
    [.+?/[0-9]{4}/[^/]+/([^/]+)/?$] => index.php?attachment=$1
    [.+?/[0-9]{4}/[^/]+/([^/]+)/trackback/?$] => index.php?attachment=$1&tb=1
    [.+?/[0-9]{4}/[^/]+/([^/]+)/feed/(feed|rdf|rss|rss2|atom)/?$] => index.php?attachment=$1&feed=$2
    [.+?/[0-9]{4}/[^/]+/([^/]+)/(feed|rdf|rss|rss2|atom)/?$] => index.php?attachment=$1&feed=$2
    [.+?/[0-9]{4}/[^/]+/attachment/([^/]+)/?$] => index.php?attachment=$1
    [.+?/[0-9]{4}/[^/]+/attachment/([^/]+)/trackback/?$] => index.php?attachment=$1&tb=1
    [.+?/[0-9]{4}/[^/]+/attachment/([^/]+)/feed/(feed|rdf|rss|rss2|atom)/?$] => index.php?attachment=$1&feed=$2
    [.+?/[0-9]{4}/[^/]+/attachment/([^/]+)/(feed|rdf|rss|rss2|atom)/?$] => index.php?attachment=$1&feed=$2
    [(.+?)/([0-9]{4})/feed/(feed|rdf|rss|rss2|atom)/?$] => index.php?category_name=$1&year=$2&feed=$3
    [(.+?)/([0-9]{4})/(feed|rdf|rss|rss2|atom)/?$] => index.php?category_name=$1&year=$2&feed=$3
    [(.+?)/([0-9]{4})/page/?([0-9]{1,})/?$] => index.php?category_name=$1&year=$2&paged=$3
    [(.+?)/([0-9]{4})/test(/(.*))?/?$] => index.php?category_name=$1&year=$2&test=$4
    [(.+?)/([0-9]{4})/?$] => index.php?category_name=$1&year=$2
    [(.+?)/feed/(feed|rdf|rss|rss2|atom)/?$] => index.php?category_name=$1&feed=$2
    [(.+?)/(feed|rdf|rss|rss2|atom)/?$] => index.php?category_name=$1&feed=$2
    [(.+?)/page/?([0-9]{1,})/?$] => index.php?category_name=$1&paged=$2
    [(.+?)/test(/(.*))?/?$] => index.php?category_name=$1&test=$3
    [(.+?)/?$] => index.php?category_name=$1
)

#14 @ryan
18 years ago

Looks good offhand. Let me test it a bit.

#15 @jhodgdon
18 years ago

I applied the patch, and it does add the endpoint for the main blog, so www.example.com/foo/param and www.example.com/foo/param work correctly.

It still doesn't add an endpoint to the feed, as noted above. I guess that is not going to be possible, since the feed is implemented as an endpoint as well. Same with pages. But it seems to me that if I ask for an endpoint to be added to everything, that WordPress should add it to everything it generates.

And no, I didn't want to add language to the permalink structure, I really do want endpoints, so that the suffix is added to categories, month archives, etc. As far as I can tell, the permalink structure is used only for blog posts, right?

#16 @foolswisdom
18 years ago

  • Milestone changed from 2.2 to 2.3

#17 @ryan
18 years ago

(In [5130]) Rewrit endpoint fixes from westi. see #3964

#18 follow-up: @jhodgdon
18 years ago

Since the patch (which I had tested, see previous comment) has been applied, I think we ought to close this bug. I don't think it's practical to add endpoints to endpoints, unfortunately, so I think the feed additions will just have to stay broken. Thoughts?

#19 in reply to: ↑ 18 @westi
18 years ago

Replying to jhodgdon:

Since the patch (which I had tested, see previous comment) has been applied, I think we ought to close this bug. I don't think it's practical to add endpoints to endpoints, unfortunately, so I think the feed additions will just have to stay broken. Thoughts?

This sounds sane to me.

If anything we need a new ticket to say that WP_Rewrite needs a rewrite ;-)

#20 @foolswisdom
18 years ago

  • Milestone changed from 2.3 to 2.2
  • Resolution set to fixed
  • Status changed from assigned to closed
Note: See TracTickets for help on using tickets.