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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Status: Active » Needs review

This is a patch.

dropcube’s picture

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

Thanks.

oadaeh’s picture

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.

oadaeh’s picture

FileSize
1.02 KB

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.

Dave Reid’s picture

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.

catch’s picture

Status: Needs review » Needs work

Doesn't apply to HEAD any more.

hass’s picture

Subscribe

oadaeh’s picture

Status: Needs work » Needs review
FileSize
1.02 KB

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.

Dave Reid’s picture

Issue tags: +common.inc, +drupal_http_request

Adding tags...

oadaeh’s picture

Status: Needs work » Needs review
FileSize
1.06 KB

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.

superspring’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs work » Needs review
FileSize
1.09 KB

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.

hass’s picture

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

ssm2017 Binder’s picture

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

Berdir’s picture

Issue tags: +Needs tests

I guess it can only go in with test coverage.

barraponto’s picture

Here's a reroll for d7.

barraponto’s picture

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.

ssm2017 Binder’s picture

here is a new patch release for d7

oadaeh’s picture

Status: Needs work » Needs review

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

Berdir’s picture

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.

sun’s picture

Priority: Normal » Critical
Issue summary: View changes
Issue tags: -Needs tests, -common.inc, -drupal_http_request +php-fpm, +phpfpm-testbot, +PHP 5.4
Parent issue: » #2135199: Provide php 5.4 testing on testbots for D8 code without breaking everything else

@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

twistor’s picture

Status: Needs review » Needs work
FileSize
2.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.

Mile23’s picture

Status: Needs work » Needs review
FileSize
4.18 KB

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.

Mile23’s picture

FileSize
4.09 KB

Wow. Left a debug() in there.

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

Jorrit’s picture

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.

sun’s picture

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

Jorrit’s picture

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.

Mile23’s picture

@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

Jorrit’s picture

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.

Mile23’s picture

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.

Jorrit’s picture

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.

Mile23’s picture

FileSize
4.52 KB
1.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.

Mile23’s picture

FileSize
4.29 KB
234 bytes

OK, now with pesky .gitignore changes removed.

Mile23 queued 35: 205969_35.patch for re-testing.

chertzog’s picture

Status: Needs review » Reviewed & tested by the community

Patch still applies to 7.31. Works for me.

The last submitted patch, 24: drupal_http_request-205969-24.patch, failed testing.

David_Rothstein’s picture

Priority: Critical » Normal
Status: Reviewed & tested by the community » Fixed

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

index 522bf10..a38d13b 100644
--- a/includes/common.inc
+++ b/includes/common.inc
@@ -1086,14 +1086,13 @@ function drupal_http_request($url, array $options = array()) {
 }
 
 /**
- * Split an HTTP response status line into components.
+ * Splits an HTTP response status line into components.
  *
- * Refactored out of drupal_http_request() so it can be unit tested.
- *
- * @link http://www.w3.org/Protocols/rfc2616/rfc2616-sec6.html Status line definition @endlink
+ * See the @link http://www.w3.org/Protocols/rfc2616/rfc2616-sec6.html status line definition @endlink
+ * in RFC 2616.
  *
  * @param string $respone
- *   The response status line, e.g. 'HTTP/1.1 500 Internal Server Error'
+ *   The response status line, for example 'HTTP/1.1 500 Internal Server Error'.
  *
  * @return array
  *   Keyed array containing the component parts. If the response is malformed,
diff --git a/modules/simpletest/tests/common.test b/modules/simpletest/tests/common.test
index 0de13df..6e03253 100644
--- a/modules/simpletest/tests/common.test
+++ b/modules/simpletest/tests/common.test
@@ -1084,14 +1084,12 @@ class DrupalHTTPRequestTestCase extends DrupalWebTestCase {
 
 /**
  * Tests parsing of the HTTP response status line.
- *
- * @see _drupal_parse_response_status()
  */
 class DrupalHTTPResponseStatusLineTest extends DrupalUnitTestCase {
   public static function getInfo() {
     return array(
-      'name' => '_drupal_parse_response_status()',
-      'description' => 'Perform unit tests on _drupal_parse_response_status()',
+      'name' => 'Drupal HTTP request response status parsing',
+      'description' => 'Perform unit tests on _drupal_parse_response_status().',
       'group' => 'System',
     );
   }

  • David_Rothstein committed 6b47cbf on 7.x
    Issue #205969 by Mile23, oadaeh, twistor, ssm2017 Binder, barraponto,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.