drupal_http_request - case sensitive HTTP header field names

Let's write a simpletest for this?

I wrote a test for this bug. I re-rolled the patch, now it contains the test too.

I rerolled the patch to make it apply without any notices.

The last submitted patch failed testing.

See: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476

The last submitted patch failed testing.

Rerolled against HEAD.

The last submitted patch failed testing.

Drupal treats these field names case sensitive, wget does the opposite. I read the RFC 2616, but it's not clear me that Drupal does or does not follow the RFC in the question. But for practical reasons it should work as wget.

Message header field names are case insensitive (see Message Headers in RFC 2616), so it looks like wget got it right:

HTTP header fields, which include general-header (section 4.5), request-header (section 5.3), response-header (section 6.2), and entity-header (section 7.1) fields, follow the same generic format as that given in Section 3.1 of RFC 822 [9]. Each header field consists of a name followed by a colon (":") and the field value. Field names are case-insensitive. The field value MAY be preceded by any amount of LWS, though a single SP is preferred.

I'm confused about the patch though:

I attached a patch to fix this issue. It basically makes all the header field names lowercase and fix the places in drupal core where it matters.

If we just established that header field names are case insensitive, then why are we changing the case of these field names, especially since the RFC uses capitalized field names itself (for example: If-Modified-Since)? Note: I'm not saying it's the wrong approach, I'm just not sure I fully understand the problem.

Well, you can do the strtolower() at every comparison, but this sucks. I think the right approach is to use it always lower-case in Drupal (except when we issue a request) and use strtolower() only when parsing the http requests.
I have to take a look why the patch failed.

I looked closer at your patch and the test and think I now understand the problem a bit better. I also agree that using strtolower() or strcasecmp() isn't the most elegant solution. I can't shake off the feeling that the proposed patch treats the symptoms rather than the actual problem though, but I understand your reasoning.

And now on to the actual patch:

1.      if (isset($result->headers[$header]) && $header == 'Set-Cookie') {
       // RFC 2109: the Set-Cookie response header comprises the token Set-
       // Cookie:, followed by a comma-separated list of one or more cookies.
-      $result->headers[$header] .= ',' . trim($value);
+      $result->headers[strtolower($header)] .= ',' . trim($value);
     }
   
   I think this can become a bit messy: if I'm not mistaken, we can end up with two values, $result->headers['Set-Cookie'] and $result->headers['set-cookie'] which have different values?
2. If we're forcing the result headers to lowercase, I feel we should at least document this, both at the PHPDoc for drupal_http_request:
   
   *   - headers
*       An array containing the response headers as name/value pairs.
   
   as well as when the actual lowercase conversion happens.

I can't shake off the feeling that the proposed patch treats the symptoms rather than the actual problem though, but I understand your reasoning.

Well, I think the actual problem is that PHP doesn't support an array-like datatype with case-insensitive keys. The strtolower() approach is also used in SimpleTest in DrupalWebTestCase::drupalGetHeaders().

This changes headers that are sent by drupal_http_request(). There is no need to lower-case them. It is only received headers that should be lower-cased.

Updated patch.

The last submitted patch failed testing.

Ahem.

The last submitted patch failed testing.

It's been a while, but here is a reroll.