Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
When a response from a given URL doesn't return what is expected, a notice is generated:
notice: Undefined offset: 2 in /var/www/drupal6/includes/common.inc on line 511.
The line at 511 is this:
list($protocol, $code, $text) = explode(' ', trim(array_shift($split)), 3);
What happened is I put in a URL that only returned:
HTTP/1.1 200
instead of the anticipated
HTTP/1.1 200 OK
The attached patch provides one way of keeping the error message from showing up.
Comment | File | Size | Author |
---|---|---|---|
#35 | interdiff_34.txt | 234 bytes | Mile23 |
#35 | 205969_35.patch | 4.29 KB | Mile23 |
#34 | interdiff_26.txt | 1.21 KB | Mile23 |
#34 | 205969_34.patch | 4.52 KB | Mile23 |
#26 | 205969_26.patch | 4.09 KB | Mile23 |
Comments
Comment #1
catchThis is a patch.
Comment #2
dropcube CreditAttribution: dropcube commentedPlease, provide more information for testers. Would be good to have a list of steps to reproduce.
Thanks.
Comment #3
oadaeh CreditAttribution: oadaeh commentedDarn. Why am I only getting some e-mails on my issues? I totally missed both responses.
@dropcube, I'll try to spend a little time later today or tomorrow coming up with a concise list of steps to reproduce it.
Comment #4
oadaeh CreditAttribution: oadaeh commentedOkay, I went ahead and took out some time to do this now, in case it is to make it in the final release. Here are the steps to produce it:
The notice shows up (now on line 510).
I've updated the patch to account for the line number change.
Comment #5
Dave ReidMarked #273994: drupal_http_request assumes presence of Reason-Phrase in Status-Line as a duplicate of this issue. Moving to 7.x so it can be fixed there first, then backported.
Comment #6
catchDoesn't apply to HEAD any more.
Comment #7
hass CreditAttribution: hass commentedSubscribe
Comment #8
oadaeh CreditAttribution: oadaeh commentedIt seems as though Goodreads has fixed their feed so that the error no longer happens. I suppose I could contrive a feed that has that error in it, but I haven't the time right now to do that. I have updated the patch, however, so that it applies to HEAD. I'll try to see if I can locate or create a feed that produces the error latter on.
Comment #10
Dave ReidAdding tags...
Comment #11
oadaeh CreditAttribution: oadaeh commentedHere's an updated version of the patch. I still don't have an example to test it against.
Comment #13
superspring CreditAttribution: superspring commentedI am unable to reproduce the problem with the URL given above, but I am sure that there are a few servers out there that don't follow the HTTP protocol properly. Do we want to specifically not cater for these?
Attached is a Drupal 8 patch since the code described above is still there.
Comment #14
hass CreditAttribution: hass commentedSince #1447736: Adopt Guzzle library to replace drupal_http_request() is RTBC, this may never go into D8. May only go into D7/D6.
Comment #15
ssm2017 Binder CreditAttribution: ssm2017 Binder commentedi can read in #14 that this patch would not been applied to d8 but the problem still exists in d7.17 and this patch is working for d7 (i have applied it manually)
so i am changing the assignation of the version back to d7 in the hope that it will be applied soon :)
Comment #16
BerdirI guess it can only go in with test coverage.
Comment #17
barraponto CreditAttribution: barraponto commentedHere's a reroll for d7.
Comment #18
barraponto CreditAttribution: barraponto commentedAccidentally removed the tag.
Comment #20
ssm2017 Binder CreditAttribution: ssm2017 Binder commentedhere is a new patch release for d7
Comment #21
oadaeh CreditAttribution: oadaeh commentedChanging the status, so the bot can test the patch.
Comment #22
BerdirFor writing tests, have a look at the existing drupal_http_request() tests. In 7.x, they are in DrupalHTTPRequestTestCase, in common.test.
There is quite a number of tests in there, that tests invalid schemas, redirects, timeouts and so on. You will need to create a page callback in a system_test.module that returns a response which triggers this bug, then call it in the test.
Comment #23
sun@twistor discovered in #2157017-13: D7: DrupalHTTPRequestTestCase breaks on php-fpm [testbot PHP 5.4 blocker] that PHP on php-fpm does not send a Reason-Phrase when the PHP-native header() function is called with a "Location" header and a custom 307 response status code.
The system_test.module appears to issue such headers for DrupalHTTPRequestTestCase.
http://php.net/manual/en/function.header.php
Comment #24
twistor CreditAttribution: twistor commentedSo, I can't figure out a way to write a test for this. In a page callback, I'm doing this:
With mod_php, the response is
HTTP/1.1 500 Internal Server Error
With fcgi via fpm, the response is
HTTP/1.1 999
So, this will be testable when we switch to using fcgi, but not before.
on both mod_php and fcgi, this is perfectly valid:
Trying to use a valid response code sends a reason message back, except for the case of 307 and fcgi which is why this issue has become blocking.
Short of writing our own HTTP server for this test, I'm not sure how to proceed.
I've tested this on 5.3.28 and 5.4.24.
Comment #25
Mile23I refactored the code in question into a function (
_drupal_parse_response_status()
), and then did some unit testing on the function.drupal_http_request()
is a mere 308 lines after my trim. :-)Some stylistic questions such as what to call the unit test in
getInfo()
. I gave it the function name, since a few other tests do that.Comment #26
Mile23Wow. Left a debug() in there.
Let's try again with a few other changes too.
Comment #27
Jorrit CreditAttribution: Jorrit commentedI got the 'Undefined offset: 2 in drupal_http_request()' error while using the facebook module, as Facebook returns no reason-phrase for their Graph API request. However, the RFC for HTTP/1.1 states that the reason is required but it can be empty.
The status line must be of the form:
HTTP-Version[space]Status-Code[space]Reason-Phrase[CRLF]
This means that the following response is valid:
HTTP/1.1[space]400[space][space][CRLF]
which is what Facebook returns.
The only reason Drupal doesn't accept this is because of the
trim()
in the linelist($protocol, $code, $status_message) = explode(' ', trim(array_shift($response)), 3);
. My suggestion is to just remove the trim, or restrict the trim to remove only\r
and\n
. This is sufficient for HTTP spec conformance.Comment #28
sunThanks @Jorrit. Along with a code comment + tests, that sounds like a sensible solution.
Comment #29
Jorrit CreditAttribution: Jorrit commentedIt is hard to write a test for this, because I can't force the Apache web server to return an empty Reason-Phrase. It will return the default one for the status code. I can only write this test by invoking a known web server with this behavior, like Facebook, but I think that that is not desirable.
Comment #30
Mile23That's why the patch in #26 refactors response line parsing out to another function, and then unit-tests the function for different styles of response code.
It also allows for the condition you mention.
Response code definitions here: http://www.w3.org/Protocols/rfc2616/rfc2616-sec6.html
Comment #31
Jorrit CreditAttribution: Jorrit commentedI understand. I just think the problem can be solved by simply removing the trim(). I what situation did you encounter the notice? (ie. what webserver sent no reason-phrase?)
Regarding your patch:
Perhaps you can assign to $result->protocol and $result->status_message direcly, without first initializing $protocol and $status_message.
By the way, I found a way to test the whole
drupal_http_request()
function without resorting to testing subfunctionality using functions, but I assume it is not acceptable: you can write a small socket server in a separate PHP script that responds to an incoming connection with a fixed response and then exits, and fork that process from the test. It actually works.Comment #32
Mile23IIRC, some analysis showed that these variables are used throughout drupal_http_request(). I wanted to make a patch that worked and illustrated the solution of making a new function and unit testing it, and then see if that got shot down. Sadly, I was met with apathy. :-)
I came at this issue from the #2135199: Provide php 5.4 testing on testbots for D8 code without breaking everything else direction, so I wasn't seeing the error, just trying to contribute to unblocking that issue.
Comment #33
Jorrit CreditAttribution: Jorrit commentedI did only a quick look and I thought only status message was used in another part of the function, and you can reference directly to $result->status_message. But you're right, it doesn't matter much. I too would like to know if any patch is acceptable, given that Drupal 7 is in such a mature state.
Comment #34
Mile23Fixed stuff from #31.
Again, the premise here is that since we can't always test whether the server returns a given response code, we refactor the response code handling out to another function and then unit test that function.
Edit: Oops, my .gitignore made it in.
Comment #35
Mile23OK, now with pesky .gitignore changes removed.
Comment #37
chertzogPatch still applies to 7.31. Works for me.
Comment #39
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! Splitting it to a helper function was definitely a good way to make it testable.
It looks to me like the new code is calling trim() twice?... not a huge deal but perhaps something to fix on a followup.
Since this seems to only be a PHP notice I can't figure out why it's critical. If I'm mistaken feel free to explain why so I can mention this in CHANGELOG.txt as a critical bugfix.
Also, I fixed a few small style issues and such (in the code comments and tests) on commit: