Make WordPress Core

Opened 14 years ago

Closed 12 years ago

Last modified 12 years ago

#16692 closed enhancement (fixed)

Add hook to allow plugins to implement custom $wp->parse_request() logic to support arbitrary custom URLs

Reported by: mikeschinkel's profile mikeschinkel Owned by: nacin's profile nacin
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.1
Component: Rewrite Rules Keywords: has-patch commit
Focuses: Cc:

Description

This ticket addresses the issues raised in the following two tickets:

The first ticket (#12935) was an exploration and proposed "boiling the ocean"; frankly not a smart approach in hindsight.

The second ticket (#16687) proposes to implement a hack to resolve one very comment use-case that is not well handled as of v3.1 while unfortunately ignoring the subsequent most common use-cases that will arise that are also not well handled.

This ticket actually aims to support the second ticket by providing a generic way that a plugin can front-end the parse_request() of the global $wp variable which will then allow core to generically address the issue raised on #16687 but also allow plugins to add support for the subsequent common use-cases that arise after ticket #16687 is resolved.

As a validation of the need for this here is a question on WordPress Answers that I answered by showing how to front-end `$wp-parse_request()` by subclassing the `WP` class, but clearly only one plugin can use that technique at a time so it's not a technique that should be used except by a site owner. This ticket aims to give safe plugins the ability to extend $wp->parse_request() so that plugins don't subclass WP to the detriment of users who need to use two plugins that use the same technique.

What the patch for this ticket does is add a 'wp_parse_request' hook and uses it to call $wp->parse_request() instead of the currently hard-coded call to $wp->parse_request(). This will allow a plugin to remove the action and add its own, and if the plugin does not find an appropriate URL match then it can be free to delegate matching back to $wp->parse_request(). It would be a technique that could be used by skilled plugin developers to really enhance the ability to route URLs in WordPress without negatively affecting those who are not using it in any way.

Attachments (4)

wp_parse_request_hook.diff (1.1 KB) - added by mikeschinkel 14 years ago.
Added 'wp_parse_request' hook
wp_parse_request_hook-v2.diff (1.1 KB) - added by mikeschinkel 14 years ago.
Changes call to do_action_ref_array() with do_action(), props @scribu
wp_parse_request_hook-v3.diff (499 bytes) - added by mikeschinkel 14 years ago.
Changed from an action to a filter.
16692.diff (450 bytes) - added by andy 13 years ago.
Allow plugins to cancel core wp::parse_request()

Download all attachments as: .zip

Change History (34)

@mikeschinkel
14 years ago

Added 'wp_parse_request' hook

#1 @mikeschinkel
14 years ago

  • Type changed from defect (bug) to enhancement

#2 follow-up: @scribu
14 years ago

  • Owner set to scribu
  • Status changed from new to reviewing

About your patch: #16661

#3 @scribu
14 years ago

(I did not mean to set myself as owner and I can't find the Owner field anywhere either)

#4 @scribu
14 years ago

  • Owner scribu deleted

#5 in reply to: ↑ 2 @mikeschinkel
14 years ago

Replying to scribu:

About your patch: #16661

Thanks!

Can I presume then that you are suggesting I should change this:

do_action_ref_array('wp_parse_request', array(&$this,$query_args));

To this?

do_action( 'wp_parse_request', &$this, $query_args );

#6 follow-up: @scribu
14 years ago

Yes, and you can also replace &$this with just $this.

I was going to ask why you can't just use the 'request' filter, but then I realized you want to prevent the processing done in WP_Rewrite altogether.

@mikeschinkel
14 years ago

Changes call to do_action_ref_array() with do_action(), props @scribu

#7 in reply to: ↑ 6 @mikeschinkel
14 years ago

  • Cc mikeschinkel@… added

Replying to scribu:

Yes, and you can also replace &$this with just $this.

Thanks. New patch uploaded.

I was going to ask why you can't just use the 'request' filter, but then I realized you want to prevent the processing done in WP_Rewrite altogether.

Exactly. Actually, I do want to optionally run the processing done in WP_Rewrite but only after I get a chance to decide if I want to handle it instead.

My patch is probably not be the cleanest way to enable allowing someone to insert processing before $wp->parse_request() with the option of clling $wp->parse_request() if no custom URL matches, but I implemented the way I did because it was the smallest change I could envision making that could work. I figured that if the idea gets traction then others would be able to suggest a more preferable approach.

#8 @scribu
14 years ago

Yeah, I think your approach allows the most flexibility, even though it's a blasphemy in the OOP paradigm. :)

#9 @nacin
14 years ago

You could always just extend the WP class here and override parse_query().

#10 follow-up: @scribu
14 years ago

Yes, but as he already mentioned in the description, this can only be done once, so you couldn't have multiple plugins doing pre-processing.

#11 in reply to: ↑ 10 @mikeschinkel
14 years ago

Replying to scribu:

Yeah, I think your approach allows the most flexibility,

Well, it's cumbersome. It could be made less cumbersome by making it a filter like so:

// This would go in /wp-includes/class-wp.php
if ( !apply_filters( 'wp_parse_request', false, $this, $query_args ) )
   $this->parse_request( $query_args );  

If we used the above then we would not need to add the action in /wp-settings.php. I'm liking this approach more as I write about it. You?

even though it's a blasphemy in the OOP paradigm. :)

heh. :) It's basically a really loose form of multiple inheritance.

But I think that anyone who really sees it as OOP blasphemy (not you, of course :) hasn't learned much since OOP became semi-mainstream in the mid 90s. For example they probably haven't read and/or groked the seminal work Design Patterns. This approach has aspects of each of the following patterns, and it enables the Interpreter Pattern:

WordPress already follows many of those patterns brilliantly in other areas.

Replying to scribu:

Yes, this can only be done once, so you couldn't have multiple plugins doing pre-processing.

Exactly.

Last edited 14 years ago by mikeschinkel (previous) (diff)

@mikeschinkel
14 years ago

Changed from an action to a filter.

#12 @mikeschinkel
14 years ago

Maybe we could use a similar strategy as here? @scribu, would you mind attempting to apply it?

#13 follow-up: @scribu
14 years ago

I've thought about it and it's just not the same thing.

I think the reason all these approaches feel clunky is because the part you're interested in, the rewrite logic, should be in WP_Rewrite in the first place.

So, a better approach would be to create a match_rule() method in WP_Rewrite and then hook that to a 'match_rule' action called from parse_request().

#14 in reply to: ↑ 13 @mikeschinkel
14 years ago

Replying to scribu:

I think the reason all these approaches feel clunky is because the part you're interested in, the rewrite logic, should be in WP_Rewrite in the first place.

I very much disagree with that. The current WP_Rewrite system is probably the clunkiest aspect of WordPress which I'm hoping we can get deprecated by 4.0 by proving via a plugin that there is a much better way.

The current WP_Rewrite system is overly complex because there is an impedance mismatch between the WP_Rewrite system (a list of regexes with almost no metadata) and the underlying use-case it is trying to model (a tree of nodes and leafs where each node and/or leaf potentially needs unique metadata based on it's parent path.)

The current WP_Rewrite system just does not "match" the underlying use-case (pun intended) and as such just gets more and more complicated as requirements get added to WordPress. Isn't it time to think different?

A great analogy is that WP_Rewrite is like SOAP-based web services (i.e. overly complex) vs. a URL routing system that is analogous to REST-based web services (simple and elegant.) Implementing a match system within WP_Rewrite would make it harder to prove the value of an alternate approach in a plugin vs. embracing a straight replacement of $wp->parse_request().

FYI this weekend I started working on an alternate URL routing system that simply replaces parse_request() and is it coming together in an extremely promising manner. Given the complexity of the use-cases that must be matched a $wp_rewrite->match_rule would not enable needed functionality so for what I'm working on it that's a non-starter.

If you will keep an open mind towards this until you have a chance to work with it I think you may change your mind on the need to implement something within the WP_Rewrite system.

So, a better approach would be to create a match_rule() method in WP_Rewrite and then hook that to a 'match_rule' action called from parse_request().

WP_Rewrite has too much baggage and overhead on each page load. Having both systems active when we'll ultimately only need one is overhead that should be avoided.

Of course I can achieve what I need by subclassing WP and if I'm given no other alternative that's what I will do and in it I will implement my own 'wp_parse_request' hook, I just hope that I don't have to default to that approach for 3.2.

Version 1, edited 14 years ago by mikeschinkel (previous) (next) (diff)

#15 follow-up: @scribu
14 years ago

I very much disagree with that. The current WP_Rewrite system is probably the clunkiest aspect of WordPress which I'm hoping we can get deprecated by 4.0 by proving via a plugin that there is a much better way.

Stop right there and read again what I said. I'm actually proposing to make WP_Rewrite more self-contained, so that it's easier to swap out with something else.

#16 in reply to: ↑ 15 @mikeschinkel
14 years ago

Replying to scribu:

I very much disagree with that. The current WP_Rewrite system is probably the clunkiest aspect of WordPress which I'm hoping we can get deprecated by 4.0 by proving via a plugin that there is a much better way.

Stop right there and read again what I said. I'm actually proposing to make WP_Rewrite more self-contained, so that it's easier to swap out with something else.

Okay, my bad. I'll hold my thoughts until I get a chance to see what you mean.

@andy
13 years ago

Allow plugins to cancel core wp::parse_request()

#18 @andy
13 years ago

Mike's rationale for this change is sensible to me. He and I have independently invented the same clunky solution to this problem (extending the wp class) and we both wished for a better way in core.

My patch, 16692.diff, works the same way as Mike's but it is clearer and puts the override in the same function that it overrides. The core function override pattern is already established in places like wp_maybe_load_widgets and even get_option. The cost is virtually nil. This can be added without interrupting any other projects to consolidate rewrite functionality.

#19 @prettyboymp
13 years ago

  • Cc mpretty@… added

#20 @landwire
13 years ago

  • Cc landwire added

Hi,
no idea if I am right here...
I am looking for the possibility to have a permalink of the following:

%role%/%author%/%posttype%/%postname%

Basically adding %role% and %posttype% as structure tags. Is that at all possible?

#22 @sirzooro
13 years ago

  • Cc sirzooro added

#23 @mikeschinkel
13 years ago

I wanted to add a use-case for this ticket for the core developers to consider, and that is that adding this hook would enable custom URL cache functionality.

For example, if someone has a really complex site with lots of custom post types, custom taxonomies and custom permastructs they could have upwards of a 1000 rewrite rules to scan for every page load. Following the 80-20 rule, 20% of the URLs will be called 80% of the time so with this 'wp_parse_request' hook on a cron task a plugin could cache the most recently used ~100 URLs into wp_options as an array where the keys would be the URLs and the values would be the arrays that calling $wp->parse_request() would ultimately set $wp->query_args to.

Clearly such a URL cache would not be compatible with the rare plugin that has side effects in the hooks called within $wp->parse_request() but for a site owner with a complex site and heavy traffic they could avoid or modify those plugins and make a noticeable difference in performance.

#24 @mbijon
12 years ago

  • Cc mike@… added

#25 @nacin
12 years ago

  • Keywords dev-feedback removed
  • Milestone changed from Awaiting Review to 3.5

16692.diff looks good. Any objections?

#26 @scribu
12 years ago

  • Keywords commit added

Even if we move everything that follows to WP_Rewrite, that still seems like the best place for the filter.

#27 @husobj
12 years ago

  • Cc ben@… added

#28 follow-up: @nacin
12 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from reviewing to closed

In [21163]:

Add do_parse_request filter to WP::parse_request() to allow short-circuiting. props andy, mikeschinkel. fixes #16692.

#29 in reply to: ↑ 28 @mikeschinkel
12 years ago

Replying to nacin:

In [21163]:

Add do_parse_request filter to WP::parse_request() to allow short-circuiting. props andy, mikeschinkel. fixes #16692.

Holy crap! Thanks @nacin!!!

#30 @Mamaduka
12 years ago

  • Cc georgemamadashvili@… added
Note: See TracTickets for help on using tickets.