WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#11845 closed enhancement (fixed)

mod_rewrite Should Not Be Used to Check Existence of index.php

Reported by: miqrogroove Owned by:
Milestone: 3.0 Priority: normal
Severity: normal Version:
Component: Optimization Keywords:
Focuses: Cc:

Description

There's a great optimization thread going on at http://www.webmasterworld.com/apache/4053973.htm

I doubt all or even most of the recommendations would be universally compatible, but one thing did jump out at me about this:

RewriteCond %{REQUEST_FILENAME} !-f 
RewriteCond %{REQUEST_FILENAME} !-d 
RewriteRule . /index.php [L] 

Needs to be changed to:

RewriteRule ^index\.php$ - [S=1]
RewriteCond %{REQUEST_FILENAME} !-f 
RewriteCond %{REQUEST_FILENAME} !-d 
RewriteRule . /index.php [L] 

Without that change, the internal re-write of index.php after [L] has to run through the !-f and !-d conditions, which are supposedly expensive.

Attachments (3)

11845.diff (585 bytes) - added by miqrogroove 7 years ago.
11845.2.diff (1.0 KB) - added by miqrogroove 7 years ago.
11845.3.diff (669 bytes) - added by miqrogroove 7 years ago.
My mistake, sorry.

Download all attachments as: .zip

Change History (25)

#1 follow-up: @apljdi
7 years ago

I started running the code posted by miqrogroove. Consider it a test.

#2 in reply to: ↑ 1 @Lazy79
7 years ago

  • Priority changed from high to normal
  • Severity changed from major to normal
  • Type changed from defect (bug) to enhancement

Replying to apljdi:

I started running the code posted by miqrogroove. Consider it a test.

running the code since two hours on 9 trunk-blogs with success. working good and ap bench thinks it`s a little bit faster than the default wordpress rules.

in my little opinion it should went into 3.0..

if i made something wrong with trunk-reply - im sorry.. didnt use it usually..

#3 @Denis-de-Bernardy
7 years ago

While we're at it, how about:

RewriteCond %{QUERY_STRING} =https?:// [NC]
RewriteRule ^ - [F,L]

RewriteRule \.(gif|png|jpe?g|css|js|ico)$ - [L]

#5 @miqrogroove
7 years ago

While we're at it, how about:

The "static file skip" rule would break the pretty 404 system. I think that's a whole other can of worms... does it belong in the same ticket?

I can't even figure out what that other rule is for?

#6 @Denis-de-Bernardy
7 years ago

the static file skipe bypasses the WP 404 for images, the favicon ico file, js files, and css files. we could strip js and css from the list. the point, though, is WP should not trigger two dozen queries to try to find a post called favicon on every page load.

the other rule is security related. I added it when I noted a few requests in my logs that went:

/?_SERVER[DOCUMENT_ROOT]=http://evil.com

It may be desirable to add it, considering what some recommend to do in the support forums:

http://wordpress.org/search/DOCUMENT_ROOT?forums=1

#7 @miqrogroove
7 years ago

I'd use something more like this: http://www.forumpostersunion.com/showthread.php?goto=newpost&t=9878

But I don't think server security rules belongs in core either way.

#8 @miqrogroove
7 years ago

the static file skipe bypasses the WP 404 for images

It would be more intuitive and better-optimized if we just excluded the uploads path.

not trigger two dozen queries to try to find a post called favicon

The ticket for that debate is #3426

#9 @miqrogroove
7 years ago

js and css are valid points though. Nobody needs a pretty 404 page for those URLs.

#10 @hakre
7 years ago

+1 for the initial idea. I'm ok with the other suggestions as well, nice optimizations, but I commented that on the other ticket as well.

#12 @nacin
7 years ago

Let's ignore the static file skip for now and go with adding RewriteRule ^index\.php$ - [S=1] ?

#13 @miqrogroove
7 years ago

I was getting a little too abstract in the OP. It should be just:

RewriteRule ^index\.php$ - [L]

... anywhere after the RewriteBase and before the !-f

#14 @Denis-de-Bernardy
7 years ago

alternative approaches:

RewriteCond %{REQUEST_FILENAME} !-f [OR]
RewriteCond %{REQUEST_FILENAME} !-d
RewriteRule ^ - [S=1]
RewriteRule . /index.php [L]
RewriteCond %{REQUEST_FILENAME} !-f [OR]
RewriteCond %{REQUEST_FILENAME} !-d
RewriteRule ^ - [L]
RewriteRule . /index.php [L]

#15 @Denis-de-Bernardy
7 years ago

err... ignore that last comment. :-P

RewriteCond %{REQUEST_FILENAME} -f [OR]
RewriteCond %{REQUEST_FILENAME} -d
RewriteRule ^ - [S=1]
RewriteRule . /index.php [L]
RewriteCond %{REQUEST_FILENAME} -f [OR]
RewriteCond %{REQUEST_FILENAME} -d
RewriteRule ^ - [L]
RewriteRule . /index.php [L]

#16 @nacin
7 years ago

Throwing in a check for index\.php$, as suggested at first, before doing filesystem checks, seems like a performance benefit as well.

#17 @Denis-de-Bernardy
7 years ago

I doubt that, somewhat... a check for index.php would never get hit unless it's explicitly requested, e.g. http://example.com/index.php.

@miqrogroove
7 years ago

@miqrogroove
7 years ago

#18 @nacin
7 years ago

(In [13676]) Don't check for the existence of index.php in the htaccess rewrite rules. props miqrogroove. see #11845

@miqrogroove
7 years ago

My mistake, sorry.

#19 @nacin
7 years ago

(In [13678]) Don't check for the existence of index.php in the htaccess rewrite rules. props miqrogroove. see #11845

#20 @miqrogroove
7 years ago

Here's an interesting mention from the manual:

It has to be kept in mind that conditions follow a short circuit logic in the case of the 'ornext|OR' flag so that certain conditions might not be evaluated at all.

It seems to imply without saying directly that certain other conditions might be evaluated unnecessarily. Vague stuff is vague. :/

#21 @miqrogroove
7 years ago

Sorry for my misinterpretation there. I just realized the vagueness is because that sentence is in the context of a feature that is only triggered if the RewriteCond evaluates to TRUE. If it evaluates to FALSE, then the the feature is off regardless of any other logic. This in no way describes the behavior of conditions that do not have an OR flag, and it is still very safe to assume short circuit logic applies to FALSE AND the same as it does to TRUE OR.

#22 @sivel
7 years ago

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

After a benchmark using curl I have verified that this change did not have any *adverse* effects on load times. I am closing this ticket as fixed.

Note: See TracTickets for help on using tickets.