Retrieve HTTP response headers

This should be discussed and committed to core before being backported to SimpleTest 6.x-2.x.

I really miss this feature.

It would be useful if headers were returned in a name => value array, e.g. array('Expires' => 'Sun, 19 Nov 1978 05:00:00 GMT', 'Last-Modified' => 'Fri, 07 Nov 2008 16:59:08 GMT', …), like $result->headers from drupal_http_request(), and/or if there was function to get a header with a specific name, e.g. drupalGetHeader($name).

According to RFC 2616, section 4.2 (last paragraph), two headers with the same name may be combined in one by concatenating the values separated by comma. The following two header blocks are equivalent:

Foo: 1
Foo: 2

Foo: 1,2

So in the name => value array I'd combine identical headers using comma.

I would only keep the headers from the last request like drupalGetContent() and assertResponse() do. If a test wishes to inspect the "intermediate" request, I suggest it call curlExec() with an option to do only one request (i.e. CURLOPT_FOLLOWLOCATION = FALSE etc). Not sure whether this is possible with the current version of drupal_web_test_case, though.

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

This patch adds the ability to get response headers and add request headers. It adds a test to ensure the behaviour described in #9.

The patch also updates drupal_http_request() to use the $name/$value wording as discussed in #327269: drupal_page_cache_header() should compare timestamp where the patch was originally posted.

... the patch:

This patch adds access to the response headers from intermediate requests (see the unit test for an example). Will that work?

It is easy enough to override curlHeaderCallback() in your own test class to get the headers and do whatever you need to do with them. There is no need for this feature, given that it will waste memory while only being useful in a small number of test cases. I suggest we won't fix this, there is enough clutter in DWTC already.

This feature would be useful in several other issues: #327269: drupal_page_cache_header() should compare timestamp, #201122: Drupal should support disabling anonymous sessions, #43462: cache_set and cache_get base_url brokenosity. I hope you will reconsider your decision.

Is the increased memory usage more than 1-2 KB total? We don't save the headers from old requests when drupalGet() is called again, and the old headers are garbage collected, so the 1-2 KB don't add up.

Here is what pwolanin is doing in #280934: Use httponly cookie support when available:

+  /**
+   * Implementation of curlHeaderCallback().
+   */
+  protected function curlHeaderCallback($ch, $header) {
+    // Look for a Set-Cookie header.
+    if (preg_match('/^Set-Cookie.+$/i', $header, $matches)) {
+      $this->saved_cookie = $header;
+    }
+
+    return parent::curlHeaderCallback($ch, $header);
+  }

Would that kill you?

Simpletest is already slow and already requires a lot of memory. No need to add clutter to the clutter.

One possible way to address the performance concerns, is to do the heavy lifting in drupalGetHeaders(). Is there a reason the header parsing could not be postponed until drupalGetHeaders() is called?

@Darren Oh: pwolanin has not broken anything. Why would you think that the SimpleTest error headers are "broken"?

With this patch, the headers are parsed on demand in drupalGetHeaders().

To sum up: The patches in #25 and #26 both implement drupalGetHeaders() in largely the same way. In addition, the one in #25 includes a test, adds drupalGetHeader($name) for getting only one header, updates the terminology for header names and values in common.inc to be consistent with than in drupal_web_test_case, and adds the ability to add request headers. Some of this is outside the scope of this bug but was included due to discussion in #327269: drupal_page_cache_header() should compare timestamp where earlier versions of the patch was posted.

Thanks for implementing my suggestion. The patch in #25/#26 gives us the best of both worlds: no performance impact and a clean API to access the headers. Should pwolanin's test referenced in the discussion above be updated

Personally, I think drupalGetHeader($name) is a LOT more useful than drupalGetHeaders().

I'm not sure I understand why $all_requests is useful. The PHPdoc is not really helping me understand the purpose of $all_requests.

It would be good if we could improve the variable name and its explanation. Darren's comment in #31 is already a better explanation than what we have right now.

Second, it looks like we need to grep the existing tests for header parsing and see if we can reuse our new API. Damien referenced some code from pwolanin that might be a candidate for clean-up.

It looks like we should be able to drive this patch home reasonably easily.

Instead of Status-Line I suggest using a key that doesn't give the impression that this is an actual header, e.g. :status. This is not a valid HTTP header name (see the token production in RFC 2616, section 2.2).

+ list($name, $value) = array('Status-Line' => $header);
This is an unusual way of assigning two variables. It took me a few moments to figure out what was going on.

+ $headers[$request][$name] .= ','. trim($value);
Small coding style glitch (misses a space before the period).

+    // Save header to the _headers array.
+    $this->_headers[] = $header;

I don't think the comment adds anything that isn't already obvious.

Now that the headers are accessible through drupalGetHeaders(), I don't think that drupalHead() should put them in drupalGetContent() too.

Some additional comments:
+ * Gets the value of an HTTP header return by the last request.
I assume this should have been "returned".
I suggest the wording "HTTP response header" to emphasize that we are not referring to request headers (not that this is particularly unclear, but it doesn't hurt).

If you are removing CURLOPT_HEADER from drupalHead(), you may want to remove it from the rest of the file as well (originally suggested by mfb).

The :status pseudo-header should be described in the Doxygen comment.

The patch in #25 contains a test that might be relevant.

AFAICT the test is failing due to this line:
+ list($name, $value) = array(':status' => $header);
There are no keys 0 and 1 in that array, so it triggers two notices. Replacing this with $name = ':status'; $value = $header; fixes the problem.

+ list($name, $value) = array(':status', $header);
I still think this is a peculiar way of assigning two variables. The bug mentioned in #40 is a good example of how unusual coding patterns are more likely to hide bugs, because it is harder to see what is going on. But I think it works now. Apart from that the code looks good.

The comments are now much better. Perhaps the handling of duplicate header names using comma deserves mention?

Don't you think a test would be relevant?

Apart from the conflict with the #338403: Use {@inheritdoc} on all class methods (including tests) that prevents the patch for applying, it looks like this wont work, because the cookie is set in an intermediate request:

-    $this->assertTrue(preg_match('/HttpOnly/i', $this->saved_cookie), t('Session cookie is set as HttpOnly.'));
+    $this->assertTrue(preg_match('/HttpOnly/i', $this->drupalGetHeader('Set-Cookie')), t('Session cookie is set as HttpOnly.'));

There is no test for drupalGetContent(), so I don't think we need one for drupalGetHeader() or drupalGetHeaders() either.

drupalGetHeaders() is considerably more complex than drupalGetContent(). Also, the construction of a complete test suite for Drupal is still work in progress, so some functions may not have tests even though they should have (specifically drupalGetContent() is used by almost every other unit test, so testing it directly as well probably isn't that relevant).

simpletest-headers-3.patch contains an example that covers the case where drupalGetHeaders(TRUE) returns headers for several requests. You could also test whether the two Cache-Control headers in drupal_page_header() are properly concatenated using comma. Then all code paths of the new functions would be covered.

The tests may not only prevent future bugs in Drupal itself but also help discover if cURL behaves differently on different platforms (it doesn't seem unlikely that e.g. the header callback has subtle differences in different versions of cURL and/or PHP). The test suite has helped me report several bugs that occur with PHP 5.2.0 but not with newer versions (e.g #310904: Use early fetch and document why).

This looks good. Nice that you did the lowercasing of header names.

Just two nits:

+   * be checked by default. However, if true is passed as the second argument,

true should be TRUE.

+   *   The name of the header to retrieve. Names are case-insensitive (see RFC
+   *   2616 section 4.2.

You missed a ).

this is PHP5, if you are using, there is hardly any need for protected $_headers; the underscore where the underscore is the pseudo-visibility marker that Drupal uses. Just protected $headers will do.

I reviewed this patch and it looks ready. I committed it to CVS HEAD. Thanks all.

Got all passes.

Committed.