Make WordPress Core

Opened 12 years ago

Last modified 18 months ago

#18877 assigned enhancement

DRY up rewrite rule matching

Reported by: scribu's profile scribu Owned by: ericlewis's profile ericlewis
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Rewrite Rules Keywords: has-patch has-unit-tests needs-refresh
Focuses: Cc:

Description (last modified by scribu)

Currently, the whole rewrite matching loop is duplicated: once in WP::parse_request() and once in url_to_postid().

Re-posting the patch from #16687 for consideration.

Attachments (7)

18877.diff (5.1 KB) - added by scribu 12 years ago.
18877.2.diff (7.6 KB) - added by ericlewis 8 years ago.
18877.3.diff (12.1 KB) - added by ericlewis 8 years ago.
18877.4.diff (12.4 KB) - added by ericlewis 8 years ago.
18877.5.diff (13.0 KB) - added by ericlewis 8 years ago.
18877.6.diff (13.4 KB) - added by ericlewis 8 years ago.
36292-after-18877.diff (13.1 KB) - added by rmccue 8 years ago.

Download all attachments as: .zip

Change History (36)

@scribu
12 years ago

#1 @scribu
12 years ago

  • Description modified (diff)

#2 @landwire
12 years ago

  • Cc landwire added

Hi Scribu,

kind of lost with the following. Just added myself to 3 trac tickets, which might be addressing this issue, but I am not sure:

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?
Many thanks for any hints,
Sascha

#3 @scribu
12 years ago

Please stop spamming trac with your question!

I already replied here: http://core.trac.wordpress.org/ticket/16687#comment:63

#4 @chriscct7
9 years ago

  • Keywords good-first-patch added

Needs a refresh

#5 @chriscct7
8 years ago

  • Focuses performance added

@ericlewis
8 years ago

#6 @ericlewis
8 years ago

  • Keywords good-first-patch removed
Last edited 8 years ago by ericlewis (previous) (diff)

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


8 years ago

#8 @ericlewis
8 years ago

  • Keywords needs-refresh added; has-patch removed
  • Milestone changed from Awaiting Review to 4.5

This seems generically useful, for example to help with unique slugs. And I don't see us changing this API anytime soon.

@since doc needs to be updated.

#9 @ericlewis
8 years ago

@wonderboymusic @nacin @boonebgorges — does this sound good?

#10 @nacin
8 years ago

The lack of unit tests for this scares me a little. Is this a clean copy-paste? Is the code really identical between the two branches, or are there some differences?

#11 @ericlewis
8 years ago

  • Keywords needs-unit-tests added

Is the code really identical between the two branches, or are there some differences?

Mostly. It looks like commits in #17177 were only made to WP->parse_request() and not url_to_postid(), which makes sense because url_to_postid() didn't need homepage-related improvements.

@ericlewis
8 years ago

#12 @ericlewis
8 years ago

  • Keywords has-patch added; needs-refresh removed

attachment:18877.3.diff is a work in progress on unit tests. Feedback welcome :)

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


8 years ago

#14 @chriscct7
8 years ago

  • Owner set to eric
  • Status changed from new to assigned

#15 @chriscct7
8 years ago

  • Owner eric deleted

@ericlewis did you want to still work on this ticket, or should it be moved out of the milestone?

#16 @ericlewis
8 years ago

  • Milestone changed from 4.5 to Future Release

Let's move this off the 4.5 milestone.

We should think on a few things here:

  • find_match() as it currently stands is more or less a static function. Should we make the function a proper class method and look at registered rewrite rules ($wp_rewrite->wp_rewrite_rules())?
  • The fact that the function requires $request and $request_uri is confusing. I wonder if we can boil the function input down to $request.

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


8 years ago

@ericlewis
8 years ago

#18 @ericlewis
8 years ago

attachment:18877.4.diff is a refresh after r37356, which answers my question about $request and $request_uri in comment:16.

@ericlewis
8 years ago

#19 @ericlewis
8 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

attachment:18877.5.diff has unit tests with basic coverage. Any other cases we should cover?

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


8 years ago

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


8 years ago

@ericlewis
8 years ago

#22 @ericlewis
8 years ago

  • Milestone changed from Future Release to 4.6

In attachment:18877.6.diff,

  • Rename the method find_match() to get_rewrite_rule_for_request() to document itself articulately.
  • The method operates on the rewrite rules of the WP_Rewrite instance, rather than passing in an array of rewrite rules.

Feedback and review welcome. Moving this to the 4.6 milestone, and will commit soon if there no objections come up.

cc @rmccue

#23 @rmccue
8 years ago

@ericlewis Looks good to me. I've been adapting my #36292 patch to incorporate this. Couple of notes:

  • We're calling $wp_rewrite->wp_rewrite_rules() twice, once in a check in WP::parse_request() and once in the new WP_Rewrite::get_rewrite_rule_for_request(). Probably a micro-optimisation to remove that (since it stores in an option anyway), but if we could avoid it, might be nice.
  • With #36292 in mind, I've moved the query string parsing back to WP::parse_request, as it sets other variables (such as the new $wp->perform_main_query). This means returning the rule object *and* the matches, which means we're now returning three items. Bit annoying.

I'll upload a patch that incorporates my #36292 changes as a POC (not for commit), maybe there's a better way to solve the latter issue.

The patch itself looks great, +1 on commit if others are happy with it.

#24 @chriscct7
8 years ago

  • Owner set to ericlewis

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


8 years ago

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


8 years ago

#27 @ocean90
8 years ago

  • Keywords needs-refresh added
  • Milestone changed from 4.6 to Future Release

No traction over the last 3 weeks, punting for now.

#28 @desrosj
21 months ago

  • Milestone set to Future Release

Re-adding a milestone.

There's still a fair amount of repetition, but the latest patch needs a refresh.

#29 @mukesh27
18 months ago

  • Focuses performance removed

This ticket does not improve the performance so removing the tag. I agree with the proposed change.

Note: See TracTickets for help on using tickets.