Hello,

There is a slight bug in drupal_http_request().
The function uses fsockopen(); in http://au.php.net/manual/en/function.fsockopen.php you read:

errno
If provided, holds the system level error number that occurred in the system-level connect() call.
If the value returned in errno is 0 and the function returned FALSE, it is an indication that the error occurred before the
connect() call. This is most likely due to a problem initializing the socket.

The second sentence is where the problem lies. If you use drupal_http_request() on a NON-EXISTING URL, it will fail - but errno will be 0 and errstr will be empty (the socked wasn't initialized). As a result, for any calling function, $result->error will be '0' (FALSE). No error!

Basically, the code needs to check the condition where fp is NULL *and* errno is 0 - in which case, it needs to create an error "artificially".

This is exactly what my patch does.

It's a 3 liner - it would be great if it were included in 6.x

Bye,

Merc.

CommentFileSizeAuthor
#17 193073-drupal_http_request.patch1.35 KBpol
#14 193073-drupal_http_request.patch.txt1.35 KBpol
#11 193073-drupal_http_request.patch1.33 KBbrianV
#8 193073-8-drupal_http_request.patch717 bytespasqualle
#6 193073-6-drupal_http_request.patch737 bytespasqualle
patch_merc.diff422 bytesmercmobily

Comments

mercmobily’s picture

mercmobily commented

Hi,

...anybody?

Merc.

mercmobily’s picture

mercmobily commented

Hi,

I realise this is not a very important bug, as it doesn't affect many users. However, I do think it has _some_ importance...

Bye,

Merc.

mercmobily’s picture

mercmobily commented
Priority: Normal » Critical

Hi,

I don't like raising a bug's importance.
However, this bug has been sitting here for more than 3 months now. Not a comment, nothing.

Can somebody please look at this? If an URL doesn't work, anybody calling drupal_http_request() on a non-existing URL won't be able to tell that there was a problem at all.

I don't think it's urgent. However, after 3 months, I think it has somehow *become* urgent.

Bye,

Merc.

drumm’s picture

drumm
he/him
Location NY, US
commented
Version: 5.3 » 7.x-dev
Priority: Critical » Normal
Status: Active » Needs work

Patches won't get much review if they are not marked as such.

This looks okay on initial inspection, but needs more spaces to match http://drupal.org/coding-standards

Patches are usually best made against the current development version and then back-ported as necessary.

mercmobily’s picture

mercmobily commented

Hi,

I don't have a Drupal 7 right now.
However, the patch should be exactly the same, since the code has the same bug.

It's easy to just add the 3 lines by hand, in fact.

Please let me know if you will go forwards with this. Since it's such a tiny patch, backporting it to D6 and D5 would be grand!

Merc.

pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented
Title: Error setting errno in drupal_http_request (common.inc) » drupal_http_request - socket not initialized
Status: Needs work » Needs review
StatusFileSize
new 193073-6-drupal_http_request.patch737 bytes

Status: Needs review » Needs work

The last submitted patch, 193073-6-drupal_http_request.patch, failed testing.

pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented
StatusFileSize
new 193073-8-drupal_http_request.patch717 bytes
pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented
Status: Needs work » Needs review
mercmobily’s picture

mercmobily commented

Any luck with this patch?

brianV’s picture

brianV commented
Version: 7.x-dev » 8.x-dev
StatusFileSize
new 193073-drupal_http_request.patch1.33 KB

Bumping to D8. I updated the patch a bit to match current coding standards. As $errno can be 0 if the socket is not initialized, and -0 is still 0, we assign a specific error code of -1004.

mikeytown2’s picture

mikeytown2 commented

If your looking to use this right now, I've added this patch to #1320222-1: Bring in other drupal patches. I'm expecting to commit it to httprl sometime tomorrow.

If you know of any other issues that deal with drupal_http_request() let me know!

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Needs work
Issue tags: +Novice

Tagging as novice for the task of rerolling the Drupal 8.x patch on account of #22336: Move all core Drupal files under a /core folder to improve usability and upgrades.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

pol’s picture

pol
Location Brussels
commented
StatusFileSize
new 193073-drupal_http_request.patch.txt1.35 KB

Rerolled the patch.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs work » Needs review
Issue tags: -Novice
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Needs work

Er, we need to strip .txt from the filename so testbot will test it. :)

pol’s picture

pol
Location Brussels
commented
StatusFileSize
new 193073-drupal_http_request.patch1.35 KB

Ouch, my bad sorry.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs work » Needs review

And setting to NR so testbot tests it. :)

mercmobily’s picture

mercmobily commented

Hi,

Wow, I submitted this patch some 4 years ago... (!).

@mikeytown2: did this end up getting committed?

Merc.

mikeytown2’s picture

mikeytown2 commented

@mercmobily
This patch did get committed to HTTPRL

mercmobily’s picture

mercmobily commented

Hi Mike,

Ah, I see. But it hasn't been included in Core yet, right?

Merc.

mikeytown2’s picture

mikeytown2 commented
Version: 8.x-dev » 7.x-dev
Status: Needs review » Needs work

Moving this to D7 as D8 has guzzle now.

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.