WordPress.org

Make WordPress Core

Opened 6 years ago

Last modified 18 months ago

#18877 assigned enhancement

DRY up rewrite rule matching

Reported by: scribu Owned by: ericlewis
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Rewrite Rules Keywords: has-patch has-unit-tests needs-refresh
Focuses: performance 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 6 years ago.
18877.2.diff (7.6 KB) - added by ericlewis 2 years ago.
18877.3.diff (12.1 KB) - added by ericlewis 2 years ago.
18877.4.diff (12.4 KB) - added by ericlewis 20 months ago.
18877.5.diff (13.0 KB) - added by ericlewis 20 months ago.
18877.6.diff (13.4 KB) - added by ericlewis 19 months ago.
36292-after-18877.diff (13.1 KB) - added by rmccue 18 months ago.

Download all attachments as: .zip

Change History (34)

@scribu
6 years ago

#1 @scribu
6 years ago

  • Description modified (diff)

#2 @landwire
6 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
6 years ago

Please stop spamming trac with your question!

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

#4 @chriscct7
3 years ago

  • Keywords good-first-patch added

Needs a refresh

#5 @chriscct7
2 years ago

  • Focuses performance added

@ericlewis
2 years ago

#6 @ericlewis
2 years ago

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

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


2 years ago

#8 @ericlewis
2 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
2 years ago

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

#10 @nacin
2 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
2 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
2 years ago

#12 @ericlewis
2 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.


22 months ago

#14 @chriscct7
22 months ago

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

#15 @chriscct7
22 months 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
22 months 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.


21 months ago

@ericlewis
20 months ago

#18 @ericlewis
20 months ago

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

@ericlewis
20 months ago

#19 @ericlewis
20 months 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.


20 months ago

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


19 months ago

@ericlewis
19 months ago

#22 @ericlewis
19 months 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
18 months 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
18 months ago

  • Owner set to ericlewis

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


18 months ago

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


18 months ago

#27 @ocean90
18 months ago

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

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

Note: See TracTickets for help on using tickets.