Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Background: #574940: Loss of communication - Visit admin/settings/mollom to reestablish communications
The problem boils down to if you pass a naked IP, instead of a properly formed URL to xmlrpc() the xmlrpc_error() function doesn't get a return value.
In _xmlrpc() the following code is called in this instance:
if ($result->code != 200) {
xmlrpc_error($result->code, $result->error);
return FALSE;
}
However $result->code is undefined in this case.
As a consequence xmlprc_error() return is also undefined
function xmlrpc_error($code = NULL, $message = NULL, $reset = FALSE) {
static $xmlrpc_error;
if (isset($code)) {
$xmlrpc_error = new stdClass();
$xmlrpc_error->is_error = TRUE;
$xmlrpc_error->code = $code;
$xmlrpc_error->message = $message;
}
elseif ($reset) {
$xmlrpc_error = NULL;
}
return $xmlrpc_error;
}
which leads to bugs with modules that use something like:
if ($error = xmlrpc_error())) {
I believe the problem is with drupal_http_request() not setting an $result->code when the URL can't be parsed. It only seems to set a $result->error message. This should be an easy fix, IMO.
This bug exists in Drupal 5, Drupal 6 and Drupal 7.
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | drupal_http_request_fixes_drupal5.patch | 1.74 KB | Dries |
| #13 | drupal_http_request_drupal7.patch | 4.47 KB | Dries |
| #10 | drupal_http_request_fixes_drupal6.patch | 767 bytes | Dries |
| #7 | 578470-7-drupal_http_request_error_code_consistency.patch | 838 bytes | jbrauer |
| #6 | 578470-6-drupal_http_request_error_code_consistency.patch | 832 bytes | jbrauer |
Comments
Comment #1
Dave ReidSubscribing to hopefully get a patch out tonight.
Comment #2
jbrauer CreditAttribution: jbrauer commentedThe only question in my mind is what error code should be returned? Seems like a 400 would make sense?
Comment #3
jbrauer CreditAttribution: jbrauer commentedHmm will something this simple work... seems to cover those cases where we were previously not getting a result.
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commentedI agree on the 400 return code.
I don't subscribe, ever :)
Comment #5
Gábor HojtsyThere are three cases when there is no return error code, the patch only covers two. Also, the fsockopen() errors use negative numbers plus we have the HTTP status codes. Not sure we should overload the HTTP status codes, as that makes this error indistinguishable from the other errors when a HTTP request actually took place. My thinking is that we'd rather pick a negative number which does not collide with the possible return values of fsockopen().
Comment #6
jbrauer CreditAttribution: jbrauer commentedThis makes sense... Re-roll with the third instance caught... Still looking into the possible return values for fsockopen() if there are suggestions as to what we should actually use for the error code.
Comment #7
jbrauer CreditAttribution: jbrauer commentedNow with compatible error numbers and distinct error numbers for each. Thanks @gabor for the guidance.
Comment #8
Gábor Hojtsy[Edit: fsockopen() uses the connect() system call, which uses constants as return values, see http://www.opengroup.org/onlinepubs/009695399/functions/connect.html ]
I've found freepascal mappings of the constants at http://www.freepascal.org/docs-html/packages/tcl80/index-2.html or an SCO doc with a comparative chart: http://docsrv.sco.com/SDK_porting/kernel_compat_errnos.html Alternatively you can also look at the kernel source code. Anyway, seems like taking numbers from -1000 downwards seems safe, eg. -1001, -1002 and -1003 for the three cases.
Comment #9
Gábor HojtsyNote: patch came before my note because we were chatting about it in the background. Moving status back.
Comment #10
Dries CreditAttribution: Dries commentedAs described in the issue description, this bug was discovered in the Mollom module. In #574940: Loss of communication - Visit admin/settings/mollom to reestablish communications , I wrote tests that reproduce this error as part of the Mollom module, and Mollom's ability to recover from a bad server list to be specific. Those tests are for the Drupal 6 module of Mollom.
I backported the patch in this issue (#7) to Drupal 6 core, and ran the Mollom tests. I confirm that on Drupal 6, the backported patch solves the bugs isolated in the Mollom tests. I attached the backported patch.
Comment #11
Dries CreditAttribution: Dries commentedIf we want to write tests for Drupal 7 core, there are a number of
drupal_http_request()tests in./modules/simpletest/tests/common.testthat should be really easy to extend. See theDrupalHTTPRequestTestCase. At first glance, it looks like we have tests for this, but that they are only checking for$missing_scheme->error(the error message) and not for$missing_scheme->code(the error code). Given that the XML-RPC library relies on the error code, and not on the error message, this looks like an oversight. Should be easy to fix.Comment #13
Dries CreditAttribution: Dries commentedHere is a Drupal 7 patch with tests. I think this is RTBC now. :-)
Comment #15
Dries CreditAttribution: Dries commentedAnd here is a patch for Drupal 5.
Comment #17
Gábor HojtsyI think the D7 patch (in #13) should be ready to go. Not sure that we'd be better off by defining constants for these values, so that can be done later there as well.
Comment #18
webchickCool, thanks for the thorough work investigating "safe" return values to use. A define for this seems overkill since it's only ever used in one function.
Committed to HEAD. Marking down to 6.x for #10.
Comment #19
Dries CreditAttribution: Dries commentedCommitted to DRUPAL-6 as well. Moving to Drupal 5.
Comment #20
drummCommitting to 5.x.
Comment #21
chx CreditAttribution: chx commentedFirst, the patch was wrong as it was rolled without -p. Two, it needs doxygen, how we are supposed to know what -1001 means?
Comment #22
sunAs much as I *love* to see a patch from a core maintainer (since years?), I have to agree with chx. :)
Comment #23
Dave ReidHow is rolling a patch without -p translate to 'needs work' after it's been committed? Or are you just trying to nitpick? Should we document every single possible return value? What kind of documentation are you proposing? Are you going to propose we do the same with drupal_http_request(), because we don't currently have any kind of error return value documentation there as well.
Comment #24
Dave ReidMoving to new 'xml-rpc system' component.
Comment #25
sunToo late.