Notice appears when drupal_http_request called with an invalid URL
Aron Novak - July 17, 2008 - 10:37
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
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.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| drupal_http_request_notice.patch | 612 bytes | Ignored | None | None |
#1
Does parse_url() return FALSE in that case? If so, I'd check for FALSE. If not, the patch looks good.
#2
<?phpprint_r(parse_url('foo'));
Result:
Array(
[path] => foo
)
#3
I suggest also checking whether $uri['host'] is set. The code further down assumes it is.
#4
c960657 is right.
#5
I have updated the patch.
#6
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'.
#7
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.
#8
Tested, reviewed and committed to CVS HEAD. I've also backported this to DRUPAL-6. Thanks for the tests! Let's write some more.
#9
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.
#10
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.
#11
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
#12
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.
#13
The exception is unavoidable. I have put an @ operator to that particular function call to suppress the exception. See attached patch.
#14
Patch works good, doesn't affect the test. RTBC.
#15
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
#16
Looks like we are still discussing this.
#17
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.
#18
Automatically closed -- issue fixed for two weeks with no activity.