If someone does this:

drupal_http_request('foo');

a php notice appears and a switch() structure in the function uses a non-existing value.

I'd suggest to check if the URL has scheme before do anything with the scheme.

CommentFileSizeAuthor
#13 test_http_request_exception.patch816 bytesmustafau
#7 parse_url_error.patch758 bytesmustafau
#7 test_http_request.patch8.65 KBmustafau
#5 drupal_http_request_notice.patch717 bytesmustafau
drupal_http_request_notice.patch612 bytesaron novak

Comments

dries’s picture

dries commented

Does parse_url() return FALSE in that case? If so, I'd check for FALSE. If not, the patch looks good.

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented 
<?php
print_r(parse_url('foo'));

Result:

Array
(
    [path] => foo
)
c960657’s picture

c960657 commented

I suggest also checking whether $uri['host'] is set. The code further down assumes it is.

dries’s picture

dries commented
Status: Needs review » Needs work

c960657 is right.

mustafau’s picture

mustafau commented
Status: Needs work » Needs review
StatusFileSize
new drupal_http_request_notice.patch717 bytes

I have updated the patch.

dries’s picture

dries commented
Status: Needs review » Needs work

I've committed this to CVS HEAD and DRUPAL-6. Wouldn't hurt to have some tests for this. Marking it as 'code needs work'.

mustafau’s picture

mustafau commented
Status: Needs work » Needs review
StatusFileSize
new test_http_request.patch8.65 KB
new parse_url_error.patch758 bytes

It seems that when host is not set parse_url returns FALSE so we never get a "missing host" error. Instead of checking for $uri['host'] we should check for FALSE.

A test case is also attached.

dries’s picture

dries commented
Status: Needs review » Fixed

Tested, reviewed and committed to CVS HEAD. I've also backported this to DRUPAL-6. Thanks for the tests! Let's write some more.

damien tournoud’s picture

damien tournoud commented
Priority: Minor » Critical
Status: Fixed » Active

What is the rationale of moving common.test out of includes/tests and to modules/simpletest/tests? Not that I don't support this, but this not seems like the good issue to do that.

mustafau’s picture

mustafau commented

Tests in includes/tests are no longer supported by Simpletest. See this commit: http://drupal.org/cvs?commit=132616

I had to move common.test because there was no other way to make my tests recognized by Simpletest.

catch’s picture

catch
he/him
Primary language English
commented

I get an exception on the drupal_http_request() test:

parse_url(http:///path) [function.parse-url]: Unable to parse URL Warning common.inc 437

c960657’s picture

c960657 commented

Perhaps valid_url() could be utilized for validating the URL.

That method also has its flaws (see e.g. #124492: valid_url() does not support all valid URL characters or #247880: valid_url needs to support more characters), but it is probably better to fix them rather than having two different ways of validating URLs.

mustafau’s picture

mustafau commented
Status: Active » Needs review
StatusFileSize
new test_http_request_exception.patch816 bytes

The exception is unavoidable. I have put an @ operator to that particular function call to suppress the exception. See attached patch.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Patch works good, doesn't affect the test. RTBC.

boombatower’s picture

boombatower commented

Wouldn't it make more sense to but the @ operator on in the actual function instead of in the test?

I have a patch I created before becoming aware of this thread which also has an updated test.

#295564: drupal_http_request triggers error in addition to returning it & test cleanup

dries’s picture

dries commented
Status: Reviewed & tested by the community » Needs review

Looks like we are still discussing this.

catch’s picture

catch
he/him
Primary language English
commented
Priority: Critical » Normal
Status: Needs review » Fixed

Boombatower's patch looks good, and an extra test can't hurt, so back to fixed and we can deal with it in the other issue.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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