Opened 13 years ago
Last modified 2 years 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: | Cc: |
Description (last modified by )
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)
Change History (36)
#3
@
13 years ago
Please stop spamming trac with your question!
I already replied here: http://core.trac.wordpress.org/ticket/16687#comment:63
This ticket was mentioned in Slack in #core by eric. View the logs.
9 years ago
#8
@
9 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.
#10
@
9 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
@
9 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.
#12
@
9 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.
9 years ago
#15
@
9 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
@
9 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.
9 years ago
#18
@
9 years ago
attachment:18877.4.diff is a refresh after r37356, which answers my question about $request
and $request_uri
in comment:16.
#19
@
9 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.
9 years ago
This ticket was mentioned in Slack in #core by eric. View the logs.
8 years ago
#22
@
8 years ago
- Milestone changed from Future Release to 4.6
- Rename the method
find_match()
toget_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
@
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 inWP::parse_request()
and once in the newWP_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.
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
@
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.
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