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.

Files: 
CommentFileSizeAuthor
#35 interdiff_34.txt234 bytesMile23
#35 205969_35.patch4.29 KBMile23
PASSED: [[SimpleTest]]: [MySQL] 40,768 pass(es).
[ View ]
#34 interdiff_26.txt1.21 KBMile23
#34 205969_34.patch4.52 KBMile23
PASSED: [[SimpleTest]]: [MySQL] 40,724 pass(es).
[ View ]
#26 205969_26.patch4.09 KBMile23
PASSED: [[SimpleTest]]: [MySQL] 40,737 pass(es).
[ View ]
#25 205969_25.patch4.18 KBMile23
PASSED: [[SimpleTest]]: [MySQL] 40,765 pass(es).
[ View ]
#24 drupal_http_request-205969-24.patch2.51 KBtwistor
#20 drupal_http_request-205969-20.patch876 bytesssm2017 Binder
PASSED: [[SimpleTest]]: [MySQL] 39,570 pass(es).
[ View ]
#17 drupal-205969-16-3-http-request-no-status.patch1006 bytesbarraponto
FAILED: [[SimpleTest]]: [MySQL] 39,636 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#13 drupal_http_request-205969-13.patch1.09 KBsuperspring
PASSED: [[SimpleTest]]: [MySQL] 47,590 pass(es).
[ View ]
#11 drupal_http_request.patch1.06 KBoadaeh
FAILED: [[SimpleTest]]: [MySQL] 22,772 pass(es), 11 fail(s), and 260 exception(es).
[ View ]
#8 drupal_http_request.patch1.02 KBoadaeh
Failed: 9614 passes, 7 fails, 51 exceptions
[ View ]
#4 drupal_http_request.patch1.02 KBoadaeh
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_http_request_2.patch.
[ View ]
drupal_http_request.patch1.02 KBoadaeh
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_http_request_1.patch.
[ View ]

Comments

Status:Active» Needs review

This is a patch.

Please, provide more information for testers. Would be good to have a list of steps to reproduce.

Thanks.

Darn. 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.

StatusFileSize
new1.02 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_http_request_2.patch.
[ View ]

Okay, 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:

  1. Install Drupal 6 (BTW, this happens in Drupal 5.x, as well)
  2. Enable the Aggregator module
  3. In Content management, click on Feed aggregator, then Add feed
  4. Put "Goodreads" in the Title field and "http://www.goodreads.com/blog/list_rss" in the URL field
  5. Click Save
  6. Click update items

The notice shows up (now on line 510).

I've updated the patch to account for the line number change.

Title:Undefined offset in drupal_http_request() . . .drupal_http_request() assumes presence of Reason-Phrase in response Status-Line
Version:6.x-dev» 7.x-dev

Marked #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.

Status:Needs review» Needs work

Doesn't apply to HEAD any more.

Subscribe

Status:Needs work» Needs review
StatusFileSize
new1.02 KB
Failed: 9614 passes, 7 fails, 51 exceptions
[ View ]

It 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.

Status:Needs review» Needs work

The last submitted patch failed testing.

Adding tags...

Status:Needs work» Needs review
StatusFileSize
new1.06 KB
FAILED: [[SimpleTest]]: [MySQL] 22,772 pass(es), 11 fail(s), and 260 exception(es).
[ View ]

Here's an updated version of the patch. I still don't have an example to test it against.

Status:Needs review» Needs work

The last submitted patch, drupal_http_request.patch, failed testing.

Version:7.x-dev» 8.x-dev
Status:Needs work» Needs review
StatusFileSize
new1.09 KB
PASSED: [[SimpleTest]]: [MySQL] 47,590 pass(es).
[ View ]

I 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.

Since #1447736: Adopt Guzzle library to replace drupal_http_request() is RTBC, this may never go into D8. May only go into D7/D6.

Version:8.x-dev» 7.x-dev

i 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 :)

Issue tags:+Needs tests

I guess it can only go in with test coverage.

Issue tags:-Needs tests
StatusFileSize
new1006 bytes
FAILED: [[SimpleTest]]: [MySQL] 39,636 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Here's a reroll for d7.

Issue tags:+Needs tests

Accidentally removed the tag.

Status:Needs review» Needs work

The last submitted patch, drupal-205969-16-3-http-request-no-status.patch, failed testing.

StatusFileSize
new876 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,570 pass(es).
[ View ]

here is a new patch release for d7

Status:Needs work» Needs review

Changing the status, so the bot can test the patch.

For 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.

@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

Status:Needs review» Needs work
StatusFileSize
new2.51 KB

So, I can't figure out a way to write a test for this. In a page callback, I'm doing this:

header('HTTP/1.1 999');
exit;

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:

header('HTTP/1.1 999 Ponies');
exit;

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.

Status:Needs work» Needs review
StatusFileSize
new4.18 KB
PASSED: [[SimpleTest]]: [MySQL] 40,765 pass(es).
[ View ]

I 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.

StatusFileSize
new4.09 KB
PASSED: [[SimpleTest]]: [MySQL] 40,737 pass(es).
[ View ]

Wow. Left a debug() in there.

Let's try again with a few other changes too.

I 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 line list($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.

Thanks @Jorrit. Along with a code comment + tests, that sounds like a sensible solution.

It 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.

@Jorrit: It is hard to write a test for this, because I can't force the Apache web server to return an empty Reason-Phrase.

That'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

I 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:

+  $protocol = $response_status_array['http_version'];
+  $code = $response_status_array['response_code'];
+  $status_message = $response_status_array['reason_phrase'];
+
   $result->protocol = $protocol;
   $result->status_message = $status_message;
+  $protocol = $response_status_array['http_version'];
+  $code = $response_status_array['response_code'];
+  $status_message = $response_status_array['reason_phrase'];
+
   $result->protocol = $protocol;
   $result->status_message = $status_message;

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.

Perhaps you can assign to $result->protocol and $result->status_message direcly, without first initializing $protocol and $status_message.

IIRC, 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.

I 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.

StatusFileSize
new4.52 KB
PASSED: [[SimpleTest]]: [MySQL] 40,724 pass(es).
[ View ]
new1.21 KB

Fixed 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.

StatusFileSize
new4.29 KB
PASSED: [[SimpleTest]]: [MySQL] 40,768 pass(es).
[ View ]
new234 bytes

OK, now with pesky .gitignore changes removed.