drupal_http_request should return the original status message and protocol

I think this is a nice improvement. Some comments/questions after reviewing this patch:

1. The doxygen comment preceding drupal_http_request should also be changed as part of this patch (since return values are added/altered)
2. Having the status message in both $result->error and $result->status_message (in case of an error) seems a bit confusing to me. What about making $result->error a boolean, and adjusting documentation to point people towards $result->code and $result->status_message for error information?
3. While we're at it, wouldn't it make sense to rename $result->code to $result->status_code so we've got $result->status_code and $result->status_message?

I realize #2 and #3 might have a big impact on (read: break) a lot of code, but it does seem more self-explanatory to me, so I'm interested to hear what others think about this... Setting back to CNW for #1. Tagged "Needs documentation" to make sure the api docs are adjusted after this change...

The goal was a simple (back-portable to D6) fix - so let's start with just the original change plus doxygen.

Also, the error may get set with a message before it even attempts a request, so there may be no status message.

This looks good to me to, but can we write a quick test for this please? Thanks!

I probably could, but really? This is trivial functionality.

The function already has a few tests already written (modules/simpletest/tests/common.test). A test for this feature would be easy:

  $request = drupal_http_request(url('pagedoesnotexist', array('absolute' => TRUE)));
  $this->assertEqual($request->protocol, 'HTTP/1.0');
  $this->assertEqual($request->code, '404');
  $this->assertEqual($request->status_message, 'Not Found');

Four lines. :)

ok, added tests based on the suggestion.

Absent the patch to common.inc, this one set of tests gives: 20 passes, 2 fails, and 2 exceptions

+ patch: 22 passes, 0 fails, and 0 exceptions

Thanks for the extra test -- those are good enough for now. Committed to CVS HEAD.

I think we should back-port to 6.x, since we're just adding extra information, not changing anything already being used.

+ $this->assertEqual($result->protocol, 'HTTP/1.0', t('Result protocol is set as HTTP/1.0'));
This tests fails on my box (Debian Etch with PHP 5.2.0 and Apache 2.2.3) with PHP running as Apache module as well as CGI.

The web server responds in HTTP/1.1 even if the request made by drupal_http_request() is HTTP/1.0. $_SERVER[SERVER_PROTOCOL] is HTTP/1.0 as expected, but the server responds with HTTP/1.1 404 Not found even if header('HTTP/1.0 404 Not found') is called from PHP.

This is due to a bug in PHP that was fixed in PHP 5.2.1. I suggest we just skip the test for PHP 5.2.0.

Nice catch c960657. Your fix looks good to me although I can't test with PHP 5.2.0.

How about we just remove that one line from the tests -- or we comment it out for the time being. Adding a PHP version check feels like one step too far.

How about this - we can just check if something is returned for the protocol, rather than looking for any specific version.

Committed to CVS HEAD.

  *       An integer containing the response status code, or the error code if
  *       an error occurred.
+ *   - protocol
+ *      The response protocol (e.g. HTTP/1.1 or HTTP/1.0).
+ *   - status_message
+ *      The status message from the response, if a response was received.
  *   - redirect_code
  *       If redirected, an integer containing the initial response status code.

Wrong indentation.

This is ready to be commited...

Committed. Thanks!

Also, looks like documentation needs have been addressed.

Restoring previous status.

Committed. Thanks.

ok, now for 6.x

Backport. Not sure whether this qualifies for inclusion in D6.

Since we are adding information (not removing or changing any currently returned) it seems like a reasonable backport to me.

patch didn't apply for me - just the doxygen part. Here's a re-roll.

.

this is pretty trivial - ready to commit?

Looks good, committed.