When calling xmlrpc_server_call() successfully and the associated handler returns a value which is not stdClass, e.g. a string value, then line 78 in xmlrpcs.inc produces a watchdog entry when doing if ($result->is_error)

The attached patch is resolving this.

CommentFileSizeAuthor
#14 677750_xmlrpcs_d6_1.patch492 bytesjurgenhaas
#10 xmprpcs.5.patch818 bytesjurgenhaas
#7 xmlrpcs.4.patch503 bytesjurgenhaas
#4 xmlrpcs.3.patch494 bytesjurgenhaas
#2 xmlrpcs.2.patch513 bytesjurgenhaas
xmlrpcs.1.patch491 bytesjurgenhaas

Comments

Status: Needs review » Needs work

The last submitted patch, xmlrpcs.1.patch, failed testing.

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented
Status: Needs work » Needs review
StatusFileSize
new xmlrpcs.2.patch513 bytes

Converted line endings and included filepaths

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
Status: Needs review » Needs work

Good catch. I agree that the extra check is necessary here, as XMLRPC handlers can return any type of data.

+++ includes/xmlrpcs.inc	(working copy)
@@ -75,7 +75,7 @@
+  if (isset($result) && is_object($result) && $result->is_error) {

isset() is not required here: $result will exist no matter what.

Powered by Dreditor.

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented
Status: Needs work » Needs review
StatusFileSize
new xmlrpcs.3.patch494 bytes

Good point. Rerolled the patch and removed isset().

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Quick fix

Has the testbot's approval: http://qa.drupal.org/pifr/test/38758 (and mine too)

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
Status: Reviewed & tested by the community » Needs work

While trying to figure out whether or not this needed backporting to D6, it seems it was fixed in D6. The if-statement also looks slightly different in that fix, so setting this back to CNW.

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented
Status: Needs work » Needs review
StatusFileSize
new xmlrpcs.4.patch503 bytes

Right, here is the next version.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
Status: Needs review » Reviewed & tested by the community

Thanks. Looks good and follows the same approach as the D6 fix, so RTBC.

dries’s picture

dries commented

There are a number of if ($result->is_error) { in the code and this patch only addresses one. I wonder if they all need to be addressed.

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new xmprpcs.5.patch818 bytes

I found one more.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
Status: Needs review » Reviewed & tested by the community

Double-checked the patch in #10 now fixes all instances in core where $result->is_error is used without first ensuring that it's an object. The second hunk will need to be backported to D6 since this was missed in #208195: remove xmlrpcs.inc notice.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
Version: 7.x-dev » 6.x-dev
Status: Fixed » Patch (to be ported)

Thanks!

@jurgenhaas: interested in providing a D6 patch too?

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented
StatusFileSize
new 677750_xmlrpcs_d6_1.patch492 bytes
jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented
Status: Patch (to be ported) » Needs review

Sorry, submitted without a comment:

Here is the patch for D6. One of the occurances of $result->is_error was covered already, the second one is handled by the attched patch.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
Status: Needs review » Reviewed & tested by the community

Thanks, applied and reviewed, RTBC.

fgm’s picture

fgm
Primary language French
Location Paris, France
commented

I wonder: in this case, is if (is_object($result) && !empty($result->is_error))

any better than just: if (is_object($result) && $result->is_error)

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented

@fgm: see #208195: remove xmlrpcs.inc notice for some info on why !empty($result->is_error) is used instead if $result->is_error. My understanding is that the first option prevents notices in the unlikely event that $result is an object but does not have an is_error property.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Fixed

Thanks, committed.

Status: Fixed » Closed (fixed)
Issue tags: -Quick fix

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

Issue tags: +Quick fix

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.