This error appears when drupal_http_request follows a redirect. Attached patch fixes the error.

CommentFileSizeAuthor
#6 293529-drupal-http-request-documentation.patch2.42 KBdamien tournoud
#3 drupal_http_request_error.patch1.29 KBmustafau
drupal_http_request_error.patch598 bytesmustafau

Comments

dries’s picture

dries commented

This kind of subtle patches could really benefit from a test case. It looks like drupal_http_request() is in need for some tests. Care to write some?

damien tournoud’s picture

damien tournoud commented

That whole drupal_http_request() function will probably benefit for a full rewrite to make it both more robust (error and memory management are *so* bad in that function) and more easily extensible. On the feature list, proxy support has been awaited for a long time (#7881: Add support to drupal_http_request() for proxy servers (http not https)).

mustafau’s picture

mustafau commented
StatusFileSize
new drupal_http_request_error.patch1.29 KB

I was wrong. $result->redirect_code is meant to be set to $result->code. I have put an if statement and updated the function documentation. What do you think about the documentation?

I will write a test case for drupal_http_request.

damien tournoud’s picture

damien tournoud commented

In that case, how could the $result->code be unset? Because this is set on the very last line of drupal_http_request(), the only case I can think of is a redirect to a bad url (invalid, or relative, for example). In that case, we should:

  • Support redirect to relative urls (those are non compliant to the spec, I think, but are common); and/or
  • Set $result->error and pass a meaningful error message back to the caller.
mustafau’s picture

mustafau commented

In case of redirect to a bad URL $result->error is already set. However $result->code is set to 301, 302 or 307. $result->redirect_code and $result->data might not be set.

What I mean by "When a redirect happens you can not rely on the status code returned by this function" is this. For example; aggregator_refresh() supposes that if $result->code is 200, 301, 302 or 307 $result->data should be set. However I'm seeing "Undefined property: stdClass::$data in aggregator.module on line 608" errors in log messages. The caller of drupal_http_request() (in this case aggregator_refresh()) relies on $result->code and does not check for isset($result->data) before using $result->data.

damien tournoud’s picture

damien tournoud commented
StatusFileSize
new 293529-drupal-http-request-documentation.patch2.42 KB

Ok. This is mainly (but not only) a documentation issue.

I suggest we settle on that:

  • code is always the result code of the *last* request
  • redirect_code is set only when a redirect occured. It is the redirect code in that case (301, 302, 307, etc.)

Note that redirect_code is not used anywhere in core (except in our test cases). I also modified aggregator_refresh(), which is the only place in core that actually cares about redirects.

mustafau’s picture

mustafau commented

I'm OK with this. Before setting to RTBC:
- I think we should set redirect_code even if $retry = 0.
- Move,

if (isset($result->redirect_code) && $result->redirect_code == 301) {
  $feed['url'] = $result->redirect_url;
}

after hash compare.
- Incorporate patch at http://drupal.org/node/293532#comment-986376

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Needs work

The last submitted patch failed testing.

kars-t’s picture

kars-t commented
Status: Needs work » Closed (fixed)

I am closing this because the patch seems to be realized in the latest D7 version with all the new API stuff that wasn't there as the patch was written.

The documentation was added and the status code is resolved in drupal_http_request().