Some changes for better Nginx compatibility #26

merged 4 commits into from Mar 16, 2012


None yet
2 participants

sloonz commented Feb 28, 2012

The three patches are independent, but I couldn’t find how to make three distinct pull requests in github…

Unit tests: Nginx doesn't accept PUT requests without request body. Because of that, some tests doesn't pass (all tests involving PUT requests). a4d61ad fixes that.

base_uri: In nginx, url rewriting using match/replace (^(.*)$ index.php/$1) is tricky not widely used. The best way to achieve pretty-urls is to use :

try_files $uri $uri/ index.php

Which is equivalent to the (growing popular) apache rule

 RewriteCond %{REQUEST_FILENAME} !-f
 RewriteRule ^(.*)$ index.php [QSA,L]

However, this is not compatible with the current request_uri() implementation. 12955a3 fixes that.

Patch 5eaebb7 : $query_string is used in preference to $request_uri in request_uri(). This raises an important issue : assuming that the desired method by the webmaster/web developer to determine routing is request_uri (which is always the case in the example above), route will work fine whereas will not work. This is really bad IMO. We try to avoid this by not considering query_string being a valid route if it doesn't starts with '/'.

sloonz added some commits Feb 21, 2012

Don't try to use query_string to determine request_uri() if
it doesn't looks like a proper route

Query string can be used to determine routing when used this
way :


However, this method consume GET arguments even if query string
is not intended to be used for routing. To fix that, we don't
even try to use query string for routing if query string doesn't
starts with /.
Allow to use base_uri option for url routing.
Right now, the following methods are used to determine request_uri(),
used in routing :

* u or uri GET params
* PATH_INFO (index.php/uri)
* QUERY_STRING (index.php?/uri)
* REQUEST_URI, which is just the equivalent of the two above methods
  but for webservers which doesn't set PATH_INFO or QUERY_STRING

The problem with this is that none is controllable via user

This patch modify the last method to try to use option('base_uri') in
addition to the internally computed base_path. For example, if
REQUEST_URI is /app/myroute and option('base_uri') is '/app', then
request_uri() (if not other methods are used) will return '/myroute'.

The main usage of this new method will be to allow rewrite rule of
this form :

 RewriteCond %{REQUEST_FILENAME} !-f
 RewriteRule ^(.*)$ index.php [QSA,L]

Which is the preferred method for "pretty"-urls in Nginx.

sofadesign commented Mar 8, 2012

Thanks all for those good patches (I love Nginx!). Before I merge your pull request, can you confirm that tests are still passing ? Is there some changes/side effects that require a note in the README ?


sloonz commented Mar 9, 2012

Yes, the tests are still passing, at least on Nginx (no apache on my server ;))

Obviously no side effect for a4d61ad, or I think we have a serious problem ;)

For 5eaebb7 : prior to the patch, index.php?myroute was a synonym to index.php?/myroute. This is not the case anymore, only the index.php?/myroute form will be accepted as a valid route.

The description of the patch is bad, however. The issue it addresses is not that the query string is consumed, but that in the presence of a non-routing query string, the next part of the code is never called, which make the next patch (routing with request uri + base_uri option) pretty much useless.

For 12955a3 : since, if my understanding is right, this part of the code is rarely used (it just try to determine query_string/path_info from request_uri if the web server doesn't set them, but if that’s the case 1. that’s likely a configuration error, and php may have trouble running 2. php will try to build them itself if cgi.fix_pathinfo is on, which is the case by default), so it shouldn’t have any side-effect :)

For this last patch, note that I ran into an issue a few days ago : since query string is part of request uri, for /foo?bar the route is '/foo?bar' and not '/foo', which obviously leads to a 404 error when the request has a query string. I’ll probably push a fix for this at the begining of next week.


sloonz commented Mar 14, 2012

Like stated in my previous message, i fixed the issue with query string. The patch works fine for me now (it's in all my sites since monday)

sofadesign pushed a commit that referenced this pull request Mar 16, 2012

Fabrice Luraine
Merge pull request #26 from sloonz/master
Some changes for better Nginx compatibility

@sofadesign sofadesign merged commit 2c15fc5 into sofadesign:master Mar 16, 2012


sofadesign commented Mar 16, 2012

I just merged your patches. Thanks a lot for your contribution and your explainations !


sloonz commented Mar 16, 2012

Thanks for the merge :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment