Permalink
Browse files

Changed URI auto-detection to try PATH_INFO first

(thanks to @sourcejedi, PR #1326)

Up until PHP 5.2.4 (which is our new lowest requirement),
there was a bug related to PATH_INFO which made REQUEST_URI
a more reliable choice. This is now no longer the case,
see https://bugs.php.net/bug.php?id=31892 for more details.

Also removed ORIG_PATH_INFO from the suggested alternatives
for uri_protocol in application/config/config.php as it will
not exist in most of PHP's recent versions and is pointless
when you can use PATH_INFO anyway.
  • Loading branch information...
1 parent ea6688b commit 3b72eb58e61581b7e92012a322be48e216491d7c @narfbg narfbg committed Oct 31, 2012
Showing with 8 additions and 10 deletions.
  1. +1 −2 application/config/config.php
  2. +6 −8 system/core/URI.php
  3. +1 −0 user_guide_src/source/changelog.rst
@@ -64,10 +64,9 @@
|
| 'AUTO' Default - auto detects
| 'CLI' or 'argv' Uses $_SERVER['argv'] (for php-cli only)
-| 'REQUEST_URI' Uses $_SERVER['REQUEST_URI']
| 'PATH_INFO' Uses $_SERVER['PATH_INFO']
+| 'REQUEST_URI' Uses $_SERVER['REQUEST_URI']
| 'QUERY_STRING' Uses $_SERVER['QUERY_STRING']
-| 'ORIG_PATH_INFO' Uses $_SERVER['ORIG_PATH_INFO']
|
*/
$config['uri_protocol'] = 'AUTO';
View
@@ -102,23 +102,21 @@ public function _fetch_uri_string()
return;
}
- // Let's try the REQUEST_URI first, this will work in most situations
- if (($uri = $this->_parse_request_uri()) !== '')
+ // Is there a PATH_INFO variable? This should be the easiest solution.
+ if (isset($_SERVER['PATH_INFO']))
@sourcejedi
sourcejedi Nov 1, 2012

Nitpick - you didn't take the getenv() cleanups elsewhere, but you're not using getenv() here.

@narfbg
narfbg Nov 1, 2012 collaborator

The only case where $_SERVER['PATH_INFO'] wouldn't exist is when php.ini's variables_order doesn't include an S and if that's the case - your whole application will be broken, not just URI detection. I'm sure you know this and I assume this is the reason that you proposed all getenv() usage to me removed.
My approach is only a bit different - don't use getenv() in new code, but keep it in old chunks as long as it doesn't cause problems. :)

@sourcejedi
sourcejedi Nov 1, 2012

My guess was a work-around for one of PHP's PATH_INFO bugs, which was then applied to all the other variable accesses.

I wasn't aware of variable_order. It sounds like a nasty scenario to debug, if half of the URI methods could appear to work still (due to getenv())...

I guess there might also be a problem if a CGI script (e.g. bash or perl) started a CodeIgniter app as a sub-command.

@narfbg
narfbg Nov 1, 2012 collaborator

... it will then be routed as a CLI request.

{
- $this->_set_uri_string($uri);
+ $this->_set_uri_string($_SERVER['PATH_INFO']);
return;
}
- // Is there a PATH_INFO variable?
- // Note: some servers seem to have trouble with getenv() so we'll test it two ways
- $uri = isset($_SERVER['PATH_INFO']) ? $_SERVER['PATH_INFO'] : @getenv('PATH_INFO');
- if (trim($uri, '/') !== '' && $uri !== '/'.SELF)
+ // Let's try REQUEST_URI then, this will work in most situations
+ if (($uri = $this->_parse_request_uri()) !== '')
{
$this->_set_uri_string($uri);
return;
}
- // No PATH_INFO?... What about QUERY_STRING?
+ // No REQUEST_URI either?... What about QUERY_STRING?
if (($uri = $this->_parse_query_string()) !== '')
{
$this->_set_uri_string($uri);
@@ -236,6 +236,7 @@ Release Date: Not Released
- Renamed internal method ``_detect_uri()`` to ``_parse_request_uri()``.
- Changed ``_parse_request_uri()`` to accept absolute URIs for compatibility with HTTP/1.1 as per `RFC2616 <http://www.ietf.org/rfc/rfc2616.txt>`.
- Added protected method ``_parse_query_string()`` to URI paths in the the **QUERY_STRING** value, like ``_parse_request_uri()`` does.
+ - Changed ``_fetch_uri_string()`` to try the **PATH_INFO** variable first when auto-detecting.
- Removed ``CI_CORE`` boolean constant from *CodeIgniter.php* (no longer Reactor and Core versions).
- :doc:`Loader Library <libraries/loader>` changes include:
- Added method ``get_vars()`` to the Loader to retrieve all variables loaded with ``$this->load->vars()``.

0 comments on commit 3b72eb5

Please sign in to comment.