The current implementation only returns the status message if there is an error and never returns the protocol.
I'm suggesting that we return this info because some times you may want to feed the original response from the server into another program or proxy it on. My philosophy on this is that the client should interpret the data for the implementer but not hide any of it. Ideally, I think this structure is best represented in a class wherein the user could always access the properties like the raw request, but barring that, this serves my purpose and perhaps others as well.
Comment | File | Size | Author |
---|---|---|---|
#36 | drupal_http_request_status-345591-36-D6.patch | 1.5 KB | pwolanin |
#34 | drupal_http_request_status_message-D6.diff | 2.66 KB | c960657 |
#28 | indent-fu-345591-28.patch | 906 bytes | pwolanin |
#24 | fu-return-status-protocol-345591-24.patch | 949 bytes | pwolanin |
#19 | return-status-protocol-follow-up-1.patch | 1.63 KB | c960657 |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedComment #2
pwolanin CreditAttribution: pwolanin commentedComment #4
pwolanin CreditAttribution: pwolanin commentedComment #5
mr.baileysI think this is a nice improvement. Some comments/questions after reviewing this patch:
drupal_http_request
should also be changed as part of this patch (since return values are added/altered)$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?$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...
Comment #6
pwolanin CreditAttribution: pwolanin commentedThe goal was a simple (back-portable to D6) fix - so let's start with just the original change plus doxygen.
Comment #7
pwolanin CreditAttribution: pwolanin commentedAlso, the error may get set with a message before it even attempts a request, so there may be no status message.
Comment #8
pwolanin CreditAttribution: pwolanin commentedComment #9
JacobSingh CreditAttribution: JacobSingh commentedLooks good to me
Comment #10
Dries CreditAttribution: Dries commentedThis looks good to me to, but can we write a quick test for this please? Thanks!
Comment #11
pwolanin CreditAttribution: pwolanin commentedI probably could, but really? This is trivial functionality.
Comment #12
JacobSingh CreditAttribution: JacobSingh commentedI don't see any test worth writing here because the code cannot be excersized cleanly.
For this function to be testable, the library would need to be written in an OO format, or at least broken down into half a dozen functions.
-J
Comment #13
JacobSingh CreditAttribution: JacobSingh commentedComment #14
Dave ReidThe function already has a few tests already written (modules/simpletest/tests/common.test). A test for this feature would be easy:
Four lines. :)
Comment #15
JacobSingh CreditAttribution: JacobSingh commentedSure, I suppose we can add those lines, but what I meant was that this function cannot have a unit test. It can have a system test (which I suppose it has), but a unit test by purist definition should never talk to a database or talk across a network. It should simply exercise the conditionals and code flow. For instance, if network access was disabled in the php testbed this was run on, it would fail with an exception, but it would not be caught, and it would not test that the function can interpret response codes are parse them (which is what this bit of code is doing).
If you introduce a network environment (actually making an HTTP request), then you are not testing just that unit but a larger system which is expected to respond (or not respond) to that request. I'm not suggesting we build a huge test harness (although using an OO HTTP request library would be a lot easier to do this with), but IMHO, I don't think this test adds a ton of value to the implementation.
http://www.artima.com/weblogs/viewpost.jsp?thread=126923 is a nice writeup on unit tests and separation of interface from implementation.
-J
Comment #16
pwolanin CreditAttribution: pwolanin commentedok, 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
Comment #17
Dries CreditAttribution: Dries commentedThanks for the extra test -- those are good enough for now. Committed to CVS HEAD.
Comment #18
pwolanin CreditAttribution: pwolanin commentedI think we should back-port to 6.x, since we're just adding extra information, not changing anything already being used.
Comment #19
c960657 CreditAttribution: c960657 commented+ $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]
isHTTP/1.0
as expected, but the server responds withHTTP/1.1 404 Not found
even ifheader('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.
Comment #20
c960657 CreditAttribution: c960657 commentedComment #21
mcrittenden CreditAttribution: mcrittenden commentedNice catch c960657. Your fix looks good to me although I can't test with PHP 5.2.0.
Comment #22
JacobSingh CreditAttribution: JacobSingh commentedheh, hence my point that this is not a unit test.
;)
Comment #23
Dries CreditAttribution: Dries commentedHow 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.
Comment #24
pwolanin CreditAttribution: pwolanin commentedHow about this - we can just check if something is returned for the protocol, rather than looking for any specific version.
Comment #25
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD.
Comment #26
pwolanin CreditAttribution: pwolanin commentedComment #27
sunWrong indentation.
Comment #28
pwolanin CreditAttribution: pwolanin commentedComment #29
mr.baileysThis is ready to be commited...
Comment #30
webchickCommitted. Thanks!
Comment #31
webchickAlso, looks like documentation needs have been addressed.
Restoring previous status.
Comment #32
Dries CreditAttribution: Dries commentedCommitted. Thanks.
Comment #33
pwolanin CreditAttribution: pwolanin commentedok, now for 6.x
Comment #34
c960657 CreditAttribution: c960657 commentedBackport. Not sure whether this qualifies for inclusion in D6.
Comment #35
pwolanin CreditAttribution: pwolanin commentedSince we are adding information (not removing or changing any currently returned) it seems like a reasonable backport to me.
Comment #36
pwolanin CreditAttribution: pwolanin commentedpatch didn't apply for me - just the doxygen part. Here's a re-roll.
Comment #37
sun.
Comment #38
pwolanin CreditAttribution: pwolanin commentedthis is pretty trivial - ready to commit?
Comment #39
c960657 CreditAttribution: c960657 commentedComment #40
Gábor HojtsyLooks good, committed.