Duplicate check for comments: Inappropriate errorhandling for xmlrpc

[mrutz]

The duplicate check for comments just dies in case a dulicate is detected(see wp-includes/comment.php around line 550). This ends up with a html page delivered in any cases. When accessing through xmlrpc this leads to the problem that this html page is send back to the xmlrpc client instead of a proper xmlrpc formated message.

[nacin]

We make a DOING_AJAX check there. I would want to add in an XMLRPC check, but that seems lame. Maybe we can drop-in an action there, that XMLRPC can then intercept?

[mrutz]

Looking at the code I see that there is already the action 'comment_duplicate_trigger'. I don't know exactly, but I would assume that the XMLRPC code should attach to this and handle the duplicate appropriate. Or am I missing something?

If I'm completely wrong, I still would opt for the action as it would be prepared for future upcoming protocols too.

[nacin]

Indeed, exactly. We'd want to do this for APP requests too.

Of note, there's also comment_flood_trigger that has the same wp_die().

This isn't a regression, so I'm moving this to future release.

[solarissmoke]

Looking at the code I see that there is already the action 'comment_duplicate_trigger'. I don't know exactly, but I would assume that the XMLRPC code should attach to this and handle the duplicate appropriate.

I might be missing something obvious, but I don't think this is possible. IXR_Server calls the wp_newComment() function when that method is invoked. Now, the only way to get a proper error message sent back to the client is to have wp_newComment return an IXR_Error object. Hooking into comment_duplicate_trigger doesn't help because it's an action and not a filter. So you can't use it to tell wp_newComment() what to return.

It seems to me that we need something like DOING_AJAX to get this to work. Or just replicate the check separately in the XML-RPC class (and APP, etc).

[dd32]

Hopefully implementing some of #16979 will allow the duplicate comment check to be handled properly for xml-rpc

[markoheijnen]

Shouldn't we change the function wp_allow_comment() to not use the function die()/wp_die(). Makes more sense that the function what calles wp_allow_comment() to do that.

[mdgl]

Patch uploaded to #18630 that follows markoheijnen's suggestion and would allow APIs such as XML-RPC, JSON and others to add comments safely and with reasonable error handling.

[mdgl]

Still needs patch to XML-RPC handler.

[websupporter]

With https://core.trac.wordpress.org/ticket/36901 in core, wp_new_comment() returns the comment ID, false or a WP_Error. <strike>14452.diff takes this into account.</strike>

Ah sorry, my patch works on wp_trackback.phpnot the general XMLRPC. Sorry! But wp_new_comment() has changed. It should be easily changeable now :)

[websupporter]

But this time it should be ok :)

[markoheijnen]

Uploaded a new patch with one unit tests testing for duplicated comment. That is the only error we can get as others aren't been checked for as wp_handle_comment_submission() is not being used.

Maybe we do need to check in the XML-RPC for an empty comment?

[johnbillion]

In 39045:

XML-RPC: Correctly handle empty and duplicate comments.

This prevents wp_die() being sent in response to an XML-RPC call that attempts to submit a duplicate comment, and correctly returns an error in response to an attempt to submit an empty comment.

Props markoheijnen, websupporter.
Fixes #14452, #38466.
See #36901