Some web servers, notably IIS, omit REQUEST_URI from the $_SERVER[] array. Others provide a value, but it is different from what drupal expects. In particular, any query string (the '?' and any text following it) are missing from $_SERVER['REQUEST_URI'] on these systems. Web servers reported to be affected by this include:
The attached patch addresses the issue by adding a comparision between $_SERVER['REQUEST_URI'] and $_SERVER['QUERY_STRING']. If QUERY_STRING contains a value and that value is not present in REQUEST_URI, an alternative method of determining request_uri()'s value is used.
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | request_uri_4.patch.txt | 1.7 KB | owen barton |
| #11 | request_uri_3.patch.txt | 1.25 KB | owen barton |
| #8 | request_uri_2.patch.txt | 1.26 KB | owen barton |
| #7 | request_uri_1.patch.txt | 1.26 KB | owen barton |
| #3 | request_uri_patch_5.x.txt | 571 bytes | dfaulkner |
Comments
Comment #1
chx commentedWe write a space after each comma. Also, I would put the return value in a static var because strpos is a bit costly for my taste.
Comment #2
dfaulkner commentedAdjusted formatting as suggested.
pulled strpos() out into a static variable; not sure about naming conventions for the static var?
This patch for 4.7. Stay tuned for a similar patch against 5.x HEAD (only differs in line numbers).
Comment #3
dfaulkner commentedforgot to change issue status.
Comment #4
dfaulkner commentedAlso see the 5.x version of the patch
Comment #5
Steven commentedCode does not conform to code style and is undocumented.
Can you give an example of such bad values of these variables?
It also seems weird how you cache this in a static. Shouldn't we simply do the entire request_uri() code only once, and cache that result instead?
Comment #6
Steven commentedSetting to critical and 5.x. Should really be fixed, so 5.0 runs on as many systems as possible. Plus, the fix is not difficult, it just needs to be tested well.
Comment #7
owen barton commentedHere is a patch that works as Steven describes.
I ditch $stored_querystring_check altogether and simply cache $uri as a static instead, which has the same memory cost, but saves more logic for each call. I also added some comments.
I added a check for !empty($_SERVER['QUERY_STRING']), because it was coming in as an empty string on the homepage which broke strpos.
I have not tested this very much, but it looks like it works fine on Debian Apache. I have not tested at all with the systems mentioned.
Comment #8
owen barton commentedHere is one with punctuation for the comments!
Comment #9
neclimdulTested on apache 2 with php 5.2 without any problems. Nitpick: I'd prefer and most static variable caches in Drupal seem to work more like:
Comment #10
webchickIndeed. isset is faster than is_null iirc.
Comment #11
owen barton commentedYeah - that was inherited from the previous patch. Here is one using !isset() (maybe empty() makes more sense - I guess you could never have a valid, empty request uri right?). I haven't had a chance to test it yet (we should make sure the static is actually working right!).
Comment #12
dries commentedI'd like to tweak the code comments some more.
If we have a REQUEST_URI and the query string is present in it, we use the value directly.Sure, but why? I'd try to provide a little bit more context (i.e. IE compatibility) so we can grok the code 6 months from now.
Comment #13
dfaulkner commentedSteven asked for examples of bad request_uri values. I can only offer the one that lead me to offer the original patch. I don't know why I didn't reference my original forum thread in describing my original patch, but here it is: http://drupal.org/node/86764 It gets interesting right around http://drupal.org/node/86764#comment-159508
If it will help in understanding, I am happy to un-patch my codebase and let you all hit it to see for yourselves.
My web site is running on a university server, which is sun-based due to some LDAP requirements elsewhere (it's the university's equivalent of shared hosting, so I'm limited in my options). Here are some stats from phpinfo():
PHP Version 5.1.4
System: SunOS [redacted] 5.10 [redacted]
Server API: CGI/FastCGI
PHP API 20041225
PHP Extension 20050922
Zend Extension 220051025
_SERVER["SERVER_SOFTWARE"]: iPlanet-WebServer-Enterprise/6.0
For some reason, this combination results in the QUERY_STRING not being present in the REQUEST_URI. I've worked with our system administrator, and he thinks that it's an issue particular to iPlanet, thus the patch. It seems everyone get the purpose of the patch, but to be clear, it's simply to be sure that $uri has the data from REQUEST_URI and QUERY_STRING when it's returned from the request_uri() function. The purpose of the strpos() check is so that we know if we need to do any additional work.
WRT Dries' concern about comments, how about updating the main comment:
All this said, does it really make sense to cache the entire value in a static? If we do, then the static $uri will change on every call to request_uri(), since the QUERY_STRING will be different, not to mention the REQUEST_URI.
What really needs to be cached is the knowledge of whether the server properly(?) includes QUERY_STRING in REQUEST_URI, which is what my original check does. Is there a more efficient way than strpos() to check for the inclusion of QUERY_STRING in REQUEST_URI, or have I missed the point of the newer patch?
Comment #14
Steven commentedTo dfaulkner: Caching in a static only affects multiple calls to request_uri() for the same request URI. Calls across requests each have their own instance of PHP and hence $uri. The function is used enough in modules and form API to warrant a static variable cache IMO.
Comment #15
dfaulkner commentedI guess I did miss the point. I thought the PHP environment was more persistent than that, at least for non-cgi environments.
Please excuse me and continue on fixing my problem! :)
Comment #16
dries commentedThe proposed help-text improvement is clean and simple and should make its way into the patch.
(I wouldn't mind to have us extend it a little, so it includes some practical examples. Extending the help text is certainly not a requirement but it might help. Why? Because this is like the 10th subtle change we make to this function. Whatever we do, it breaks certain platforms in subtle ways. I wish we could solve the root of this problem so we can stop chasing these subtle differences as well as stop trying to be smart about them.)
Either way, patch looks OK. Thanks faulkner. :)
Comment #17
owen barton commentedHere is a patch with clearer documentation about what we are doing and why
Comment #18
dries commentedLooks good. Committed to CVS HEAD. Thanks guys.
Comment #19
seanrThis patch broke the clean URL test on apache. Needs to be rolled back and reevaluated. More info here: http://drupal.org/node/102232
Comment #20
chx commentedI will link the other issue here. No need to have two open criticals for one thing.
Comment #21
dries commentedThis patch has been rolled back.
Comment #22
dries commentedGoing to lower the priority to 'normal'. This only seem to affect a minority of the Drupal users (only one known user).
Comment #23
webchickSERVER_SOFTWARE is an undefined index on command line-driven scripts, such as Drush, and causes its SimpleTest integration to return errors unrelated to the test it's attempting to run. Still an issue in 6.x.
Comment #24
dpearcefl commentedConsidering the time elapsed between now and the last comment plus the fact that D5 is no longer supported, I am closing this ticket.