HTTP_WebDAV_Server 1.0.0RC5 BC break; path confusion in different environments

From: Date: Tue, 09 Nov 2010 14:46:49 +0000
Subject: HTTP_WebDAV_Server 1.0.0RC5 BC break; path confusion in different environments
Groups: php.pear.dev 
Request: Send a blank email to pear-dev+get-53885@lists.php.net to get a copy of this message
Hi, My colleagues and I have been successfully using HTTP_WebDAV_Server for a long time and it generally works. One of the things we've always had to work around is that versions up to 1.0.0RC4 looked for $_SERVER['PATH_INFO'], which is not defined in many environments and seems to be a rather historic way of doing things; according to http://php.net/manual/en/reserved.variables.server.php it was for cases where you had URLs like: http://www.example.com/php/path_info.php/some/stuff?foo=bar in which case PATH_INFO would be "/some/stuff" In most modern environments many people (including us) just use something like RewriteRule /.* /filesystem/path/to/some_front_controller_maybe_outside_the_docroot.php and handle all the URLs in that front controller, in which case a request for "/some/stuff?foo=bar" typically results in the following server variables (these can also vary according to the use of mod_php vs CGI, reverse proxies etc.; let's ignore that for now): PATH_INFO is not defined REQUEST_URI = /some/stuff?foo=bar SCRIPT_NAME = /some/stuff We always hacked around this by using a derived class of HTTP_WebDAV_Server and setting $this->_SERVER['PATH_INFO'] to $_SERVER['REQUEST_URI'] in the constructor (perhaps it should have been SCRIPT_NAME although we don't use query strings in practice so it doesn't matter). However, HTTP_WebDAV_Server 1.0.0RC5 has changed the way URLs are handled and (on line 169) now does this: $path_info = substr($this->_SERVER["REQUEST_URI"], strlen($this->_SERVER["SCRIPT_NAME"])); It's good that it no longer relies on PATH_INFO, but the new code means that $path_info ends up set to the wrong thing (and doesn't work in at least our environment), because if you take the above example, strlen('/some/stuff') = 11; $path_info = substr('/some/stuff?foo=bar', 11) = '?foo=bar'; // this matches neither the definition of "path info" in the PHP manual nor the previous usage. Even in a situation such as that envisaged by the PHP manual ("/php/path_info.php/some/stuff?foo=bar"), I still don't think it would work correctly, because with the RC5 code, $path_info would end up being "/some/stuff?foo=bar", which also doesn't match the former usage of $_SERVER['PATH_INFO']. Similarly in a simpler case without a query string (e.g. request for "/foo"), $path_info ends up as an empty string (strlen('/foo')=4; substr('/foo',4)='') and is simply set to "/" as a result (line 173). Now, we don't work in an environment where "path info" is used, but looking at how $path_info is used in HTTP_WebDAV_Server and going back to the example URLs of EITHER "/php/path_info.php/some/stuff?foo=bar" or "some/stuff?foo=bar", my colleagues and I think it is *supposed* to be filled with "/some/stuff" (which indeed matches the previous use by HTTP_WebDAV_Server of $_SERVER['PATH_INFO'] in the case of the URL "/php/path_info.php/some/stuff?foo=bar"), but that's not what the new code is doing. Surely then line 169 should be replaced with something like this: if (array_key_exists('PATH_INFO', $this->_SERVER)) { // old-style PATH_INFO handling i.e. /php/path_info.php/some/stuff?foo=bar $path_info = $this->_SERVER['PATH_INFO']; // /some/stuff } else { $path_info = $this->_SERVER['SCRIPT_NAME']; // /some/stuff } ? However, looking at the usage of $_SERVER['SCRIPT_NAME'] in the code, there are several places where it appears that HTTP_WebDAV_Server tries to reconstruct the "old style" paths, such as line 724[RC5]: $href = $this->_mergePaths($this->_SERVER['SCRIPT_NAME'], $path); This makes sense in the "old style" environments (/php/path_info.php/some/stuff) but not for more modern environments. A similar thing applies on lines 164, 177 and 872 [also RC5]. Therefore, it seems like the code needs to establish clear concepts of: 1. "an optional URL prefix" (/php/path_info.php), 2. "the 'real' path, excluding any prefix and the query string" (/some/stuff) 3. "the query string" (?foo=bar). and use these consistently. The problem is that the $_SERVER variables simply don't provide these in a way which is consistent across environments. So, I suggest that they need initially decoding into some consistent variables (handling environmental differences at this stage), and those decoded variables (instead of $this->_SERVER or $_SERVER) are used everywhere. The query string is not used a lot, so to focus on the other two; the original line 169 would then be replaced with something like: if (array_key_exists('PATH_INFO', $this->_SERVER)) { // old-style PATH_INFO handling i.e. /php/path_info.php/some/stuff?foo=bar $path_info = $this->_SERVER['PATH_INFO']; // /some/stuff $path_prefix = $this->_SERVER['SCRIPT_NAME']; // /php/path_info.php } else { $path_info = $this->_SERVER['SCRIPT_NAME']; $path_prefix = ''; } // N.B. other code would need to be changed as well Then, line 724[RC5] would look something like (pseudo-code; ignoring variable scope here): $href = $this->_mergePaths($path_prefix, $path_info); (N.B. line 1551[RC5] looks like it could also be cleaned up as a result) There are some more potential environmental differences which should probably be accounted for, but I thought I'd get some initial feedback. Thanks, Tim

« previous php.pear.dev (#53885) next »