WordPress.org

Make WordPress Core

Opened 9 years ago

Closed 7 years ago

Last modified 7 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 Owned by: nacin
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.1
Component: Rewrite Rules Keywords: has-patch commit
Focuses: Cc:
PR Number:

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 9 years ago.
Added 'wp_parse_request' hook
wp_parse_request_hook-v2.diff (1.1 KB) - added by mikeschinkel 9 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 9 years ago.
Changed from an action to a filter.
16692.diff (450 bytes) - added by andy 8 years ago.
Allow plugins to cancel core wp::parse_request()

Download all attachments as: .zip

Change History (34)

@mikeschinkel
9 years ago

Added 'wp_parse_request' hook

#1 @mikeschinkel
9 years ago

  • Type changed from defect (bug) to enhancement

#2 follow-up: @scribu
9 years ago

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

About your patch: #16661

#3 @scribu
9 years ago

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

#4 @scribu
9 years ago

  • Owner scribu deleted

#5 in reply to: ↑ 2 @mikeschinkel
9 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
9 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
9 years ago

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

#7 in reply to: ↑ 6 @mikeschinkel
9 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
9 years ago

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

#9 @nacin
9 years ago

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

#10 follow-up: @scribu
9 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
9 years ago

Replying to scribu:

Yeah, I think your approach allows the most flexibility,

Well, it's cumbersome. I 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.

Version 0, edited 9 years ago by mikeschinkel (next)

@mikeschinkel
9 years ago

Changed from an action to a filter.

#12 @mikeschinkel
9 years ago

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

#13 follow-up: @scribu
9 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
9 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 doesn't "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.

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

#15 follow-up: @scribu
9 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
9 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
8 years ago

Allow plugins to cancel core wp::parse_request()

#18 @andy
8 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
8 years ago

  • Cc mpretty@… added

#20 @landwire
8 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
8 years ago

  • Cc sirzooro added

#23 @mikeschinkel
8 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
7 years ago

  • Cc mike@… added

#25 @nacin
7 years ago

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

16692.diff looks good. Any objections?

#26 @scribu
7 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
7 years ago

  • Cc ben@… added

#28 follow-up: @nacin
7 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
7 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
7 years ago

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