Make WordPress Core

Opened 13 years ago

Closed 4 months ago

#17185 closed enhancement (invalid)

Optimize verbose attachment rules

Reported by: duck_'s profile duck_ Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Rewrite Rules Keywords: needs-patch dev-feedback needs-unit-tests
Focuses: performance Cc:


Looking at the rules created for verbose pages it seems that there are a large number of redundant rules created specifically for attachments.

For example:

page-slug/attachment-slug/attachment/([^/]+)/?$ => index.php?attachment=$matches[1]
another-page/another-attachment/attachment/([^/]+)/?$ => index.php?attachment=$matches[1]
page-slug/attachment/([^/]+)/?$ => index.php?attachment=$matches[1]
another-page/attachment/([^/]+)/?$ => index.php?attachment=$matches[1]

As well as the associated trackback, feed and comment page rules for each.

I think we could get rid of specific attachment rewrite rules for pages and their attachments and replace them with a catch-all rewrite for page attachments:

.+?/attachment/([^/]+)/?$ => index.php?attachment=$matches[1]

I also set the flag to include paged requests (slug/page/xx/) to false for pages and page attachments for verbose rules. I think think this could also be applied to post rewrite rules and non-verbose page rules since neither are paged, though they have pages (slug/xx/), but I will open another ticket for that. This is the part I am less sure about since myself and others have been confused by the paged rules before (page/xx/), so correct me if I'm wrong here (or anywhere else!).

For the /%postname%/ structure the attached patch cuts the number of rewrites rules for 1297 pages from 14361 to 6562. This translated to a drop from 0.42s to 0.31s average response time in testing (tested using siege with one concurrent user for 30 seconds) on a single post permalink so near the bottom of the rewrite stack for verbose page rules in trunk.

I have run this patch against the current tests for WP_Query and WP_Rewrite (test_query.php in the unit-tests SVN) using several different verbose rewrite structures, including /%postname%/ and /%category%/%postname%/. The only 'fail' was for the test of paged rewrites for pages which I mentioned above (there were a couple of other tests that showed up as fails initially but further investigation proved to be problems with conflicting page and post slugs). This gives me confidence in the patch but even more rigorous testing is definitely required.

I also wrote a patch which utilised a new parameter for generate_rewrite_rules() to disable adding the attachment rules, but this didn't seem so clean.

Related: #16687 which is for %postname% in particular rather than all verbose structures

Attachments (1)

optimize-verbose-page.diff (3.7 KB) - added by duck_ 13 years ago.

Download all attachments as: .zip

Change History (14)

#1 @scribu
13 years ago

  • Type changed from defect (bug) to enhancement

Need to take a closer look, but seems like a good idea.

#2 @duck_
13 years ago

One change I've noticed is that for /%postname%/ the patch will allow /any/path/at/all/attachment/att-slug/ (first part is made up, att-slug is an existing attachment). Whereas on current trunk this will 404 since it obviously doesn't match any of the huge number of individually generated rules.

NB: the behaviour of such a request is already rather inconsistent. For example it works for both 'Month and name' and /%category%/%postname%/, though redirect_canonical kicks in for the latter.

#3 @mikeschinkel
13 years ago

  • Cc mikeschinkel@… added

#4 @nacin
13 years ago

  • Milestone changed from Awaiting Review to 3.3

#5 @duck_
13 years ago

  • Milestone 3.3 deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Invalidated by the awesomeness of eliminating verbose rules in [18541].

#6 @duck_
12 years ago

We might want to considering revisiting the attachments catch-all rule to help sites that have a large number of custom post types each of which gets an individual attachments rule.

Version 0, edited 12 years ago by duck_ (next)

#7 @wonderboymusic
12 years ago

  • Keywords dev-feedback added
  • Resolution duplicate deleted
  • Status changed from closed to reopened

+1 for Attachment endpoint masks - these get regenerated for every custom post type, yet all match the same end-of-url pattern, can be rolled up into one rule each, versus O(n) generation

#8 @wonderboymusic
12 years ago

  • Keywords dev-feedback removed

#9 @SergeyBiryukov
12 years ago

  • Milestone set to Awaiting Review

#10 @nacin
12 years ago

  • Summary changed from Optimize verbose page rules to Optimize verbose attachment rules

#11 @chriscct7
9 years ago

  • Keywords needs-patch dev-feedback added; has-patch removed

Patch is for the original ticket issue. Needs patch for the catchall endpoint

#12 @johnbillion
9 years ago

  • Focuses performance added
  • Keywords needs-unit-tests added

#13 @pbearne
4 months ago

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

This ticket is too old to add this code now as the code was refactor in a new class

Note: See TracTickets for help on using tickets.