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:

  • Sun One, AKA iPlanet (verified for Enterprise/6.0 by the patch author)
  • thttpd (see references)
  • Xitami (see references)

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.

References

CommentFileSizeAuthor
#17 request_uri_4.patch.txt1.7 KBOwen Barton
#11 request_uri_3.patch.txt1.25 KBOwen Barton
#8 request_uri_2.patch.txt1.26 KBOwen Barton
#7 request_uri_1.patch.txt1.26 KBOwen Barton
#3 request_uri_patch_5.x.txt571 bytesdfaulkner
#2 request_uri_patch_4.7.txt559 bytesdfaulkner
request_uri.patch.txt360 bytesdfaulkner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Needs work

We 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.

dfaulkner’s picture

dfaulkner CreditAttribution: dfaulkner commented
FileSize
request_uri_patch_4.7.txt559 bytes

Adjusted 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).

dfaulkner’s picture

dfaulkner CreditAttribution: dfaulkner commented
Status: Needs work » Needs review
FileSize
request_uri_patch_5.x.txt571 bytes

forgot to change issue status.

dfaulkner’s picture

dfaulkner CreditAttribution: dfaulkner commented

Also see the 5.x version of the patch

Steven’s picture

Steven CreditAttribution: Steven commented
Status: Needs review » Needs work

Code 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?

Steven’s picture

Steven CreditAttribution: Steven commented
Version: 4.7.3 » 5.x-dev
Priority: Normal » Critical

Setting 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.

Owen Barton’s picture

Owen Barton CreditAttribution: Owen Barton commented
Status: Needs work » Needs review
FileSize
request_uri_1.patch.txt1.26 KB

Here 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.

Owen Barton’s picture

Owen Barton CreditAttribution: Owen Barton commented
FileSize
request_uri_2.patch.txt1.26 KB

Here is one with punctuation for the comments!

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented

Tested 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:

  static $uri;
  if (isset($uri)) {
    ...
  }
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Needs review » Needs work

Indeed. isset is faster than is_null iirc.

Owen Barton’s picture

Owen Barton CreditAttribution: Owen Barton commented
FileSize
request_uri_3.patch.txt1.25 KB

Yeah - 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!).

Dries’s picture

Dries CreditAttribution: Dries commented

I'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.

dfaulkner’s picture

dfaulkner CreditAttribution: dfaulkner commented

Steven 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:

Because the value of $_SERVER['REQUEST_URI'] is inconsistent between various web servers (Apache, IIS, iPlanet, etc.),  we generate an equivalent using other environment variables. For those cases where REQUEST_URI is provided, we inspect it to be sure that it contains both the actual REQUEST_URI as well as the QUERY_STRING.

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?

Steven’s picture

Steven CreditAttribution: Steven commented

To 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.

dfaulkner’s picture

dfaulkner CreditAttribution: dfaulkner commented

I 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! :)

Dries’s picture

Dries CreditAttribution: Dries commented

The 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. :)

Owen Barton’s picture

Owen Barton CreditAttribution: Owen Barton commented
Status: Needs work » Needs review
FileSize
request_uri_4.patch.txt1.7 KB

Here is a patch with clearer documentation about what we are doing and why

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Fixed

Looks good. Committed to CVS HEAD. Thanks guys.

seanr’s picture

seanr
he/him
CreditAttribution: seanr commented
Status: Fixed » Needs work

This patch broke the clean URL test on apache. Needs to be rolled back and reevaluated. More info here: http://drupal.org/node/102232

chx’s picture

chx CreditAttribution: chx commented
Status: Needs work » Fixed

I will link the other issue here. No need to have two open criticals for one thing.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Fixed » Active

This patch has been rolled back.

Dries’s picture

Dries CreditAttribution: Dries commented
Priority: Critical » Normal

Going to lower the priority to 'normal'. This only seem to affect a minority of the Drupal users (only one known user).

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

SERVER_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.

dpearcefl’s picture

dpearcefl CreditAttribution: dpearcefl commented
Status: Active » Closed (won't fix)

Considering the time elapsed between now and the last comment plus the fact that D5 is no longer supported, I am closing this ticket.