Opened 2 years ago

Closed 18 months ago

#17177 closed defect (bug) (fixed)

Optimize parse_request for the home page (empty request)

Reported by: duck_ Owned by: ryan
Priority: high Milestone: 3.3
Component: Rewrite Rules Version:
Severity: normal Keywords: has-patch commit 3.3-early
Cc: dnljms

Description

When you visit a site's home page the request (WP::$request) is empty. However we still try to match it against every rewrite rule. By default none of the rules will match. This can be particularly bad when verbose page rules are enabled and WordPress is attempting 700+ regex matches (~50 pages) or more for especially large page oriented sites on an empty string.

The only reason against breaking from the loop like we do for wp-app.php in this case I could think of is if a plugin has added an extra rule to match ^$.[1] If we want to allow this then maybe manually check for a rule == $ and still break early.

[1] This can currently be blocked by #17176 as ()(/[0-9]+)?/?$ matches an empty string

Attachments (6)

17177.diff (618 bytes) - added by duck_ 2 years ago.
17177.2.diff (2.8 KB) - added by duck_ 2 years ago.
17177.3.diff (3.2 KB) - added by ryan 22 months ago.
Lose unnecessary isset() checks.
request.patch (482 bytes) - added by dnljms 19 months ago.
Fixes the check for when $req_uri is empty, but a match is still possible.
request2.patch (948 bytes) - added by dnljms 19 months ago.
I think this is a better patch, but it's a bigger change.
17177.request_match.diff (465 bytes) - added by duck_ 19 months ago.

Download all attachments as: .zip

Change History (22)

duck_2 years ago

Sort of related: #16736

  • Milestone changed from Awaiting Review to 3.2

I really like this. I wouldn't mind checking for ^$ just in case though. I can't see anyone doing that, but still.

duck_2 years ago

A lot more code movement this time so please read and test carefully.

Moved the empty and wp-app.php checks out of the loop, no need to continually run them if they don't pass the first time. Not really sure why but I don't really like the $matches = array(''); we could add a preg_match there just because, thoughts?

Tested with and without:

add_action( 'init', function() { add_rewrite_rule( '$', 'index.php?p=1' ); });

Passes all the rewrite/query tests as well as my random testing. Might want to double check some of the lesser used rewrite structures, e.g. with slug prefix, pathinfo, etc.

  • Keywords commit added

Looks good to me.

comment:6   ryan2 years ago

  • Keywords 3.3-early added
  • Milestone changed from 3.2 to Future Release

Looks good but we waited too long for 3.2. Marking for early 3.3.

  • Component changed from Performance to Rewrite Rules
  • Milestone changed from Future Release to 3.3

Looking good at a glance.

No need to check isset() before unset().

ryan22 months ago

Lose unnecessary isset() checks.

In [18466]:

Optimize parse_request for the home page. Props duck_. see #17177

  • Owner set to ryan
  • Resolution set to fixed
  • Status changed from new to closed

In [18467]:

Explain the empty req_uri branch. Props duck_. fixes #17177

  • Resolution fixed deleted
  • Status changed from closed to reopened

This change breaks when using IIS 6 with IIRF (I know that's a bit odd) as $req_uri is empty for rewritten urls. If you look at the loop, when $req_uri is empty it just uses the existing value of $request_match so there can be a match for empty $req_uri. I'll attach a patch.

Fixes the check for when $req_uri is empty, but a match is still possible.

I think this is a better patch, but it's a bigger change.

  • Cc dnljms added
  • Priority changed from normal to high

Good catch, thanks.

Not sure what happened between my original ticket where I mention WP::$request specifically and the patches which start checking $req_uri. I think we can just replace $req_uri with $request_match in the empty() call in the if-statement. I think this would be clearer than request2.patch (which also unnecessarily sets $request_match on every loop iteration) and, unlike request.patch, use the correct variable name.

duck_19 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In [19280]:

Use correct variable. We preg_match() against $request_match, so that's what should be checked with empty() too. Fixes #17177.

Note: See TracTickets for help on using tickets.