WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#16748 closed defect (bug) (fixed)

wp_die() should return proper errors when called from xmlrpc

Reported by: koke Owned by: westi
Milestone: 3.2 Priority: normal
Severity: normal Version: 3.1
Component: XML-RPC Keywords: has-patch mobile needs-testing
Focuses: Cc:

Description

Right now, when something goes wrong you get a html page with the error instead of a proper xml response.

Example: try to post the same comment twice

Attachments (2)

16748.diff (892 bytes) - added by koke 7 years ago.
16748-2.diff (776 bytes) - added by ericmann 7 years ago.

Download all attachments as: .zip

Change History (18)

@koke
7 years ago

#1 @koke
7 years ago

  • Keywords has-patch added

Added patch that fixes the issue.

I'm not really sure if the best way to proceed is adding the function in xmlrpc.php, or checking for the XMLRPC_REQUEST constant in wp-include/functions.php

#2 @nacin
7 years ago

Big plus one on your proposed solution.

#3 @westi
7 years ago

  • Component changed from General to XML-RPC
  • Keywords 3.2-early added
  • Milestone changed from Awaiting Review to Future Release
  • Owner set to westi
  • Status changed from new to accepted

#4 @josephscott
7 years ago

  • Cc josephscott added

+1 I like it

#5 @koke
7 years ago

  • Cc jbernal@… added

#6 @westi
7 years ago

  • Keywords 3.2-early removed
  • Milestone changed from Future Release to 3.2
  • Version set to 3.1

#7 @koke
7 years ago

  • Keywords mobile added

#8 @hakre
7 years ago

Related: #9093

#9 follow-up: @ericmann
7 years ago

I like your solution, and I think the location is accurate since we're hooking in to the wp_die_handler filter. The only thing I would recommend is changing the method name to xmlrpc_die_handler() for consistency (since the default is _default_wp_die_handler). I'd hate for some unsuspecting developer to call xmlrpc_wp_die() directly rather than wp_die().

Attaching a slightly modified patch that changes the method name and uses a simpler filter return.

@ericmann
7 years ago

#10 @jane
7 years ago

  • Keywords needs-testing added

#11 in reply to: ↑ 9 ; follow-up: @westi
7 years ago

Replying to ericmann:

I like your solution, and I think the location is accurate since we're hooking in to the wp_die_handler filter. The only thing I would recommend is changing the method name to xmlrpc_die_handler() for consistency (since the default is _default_wp_die_handler). I'd hate for some unsuspecting developer to call xmlrpc_wp_die() directly rather than wp_die().

Attaching a slightly modified patch that changes the method name and uses a simpler filter return.

A better name for the function is a good idea.

Using a closure instead of a function to hook this in isn't as it makes it really hard to remove_filter the function.

Also I think we shouldn't define the function in xmlrpc.php - someone might want to re-use it when serving xmlrpc requests on a different endpoint.

#12 @westi
7 years ago

[17643]

Introduce a special wp_die handler for XMLRPC requests to ensure we send an XML response. Props koke for the original patch. See #16748.

#13 @westi
7 years ago

@koke: Please could run some testing of this with the clients and know failure cases and then feedback if all is ok.

#14 in reply to: ↑ 11 @ericmann
7 years ago

Replying to westi:

Using a closure instead of a function to hook this in isn't as it makes it really hard to remove_filter the function.

I can accept that. And I like the actual commit patch even better.

Also I think we shouldn't define the function in xmlrpc.php - someone might want to re-use it when serving xmlrpc requests on a different endpoint.

I think defining a different XMLRPC endpoint would be an extreme edge case, and since the custom function was specific to our XMLRPC handler I figured that would be the best place to put it. But thinking forward to the possibility of other custom die() handlers (I've seen a couple of custom REST systems in development) I think dropping it in functions is a better solution.

#15 @koke
7 years ago

Works for me (tested in trunk)

#16 @westi
7 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed
Note: See TracTickets for help on using tickets.