WordPress.org

Make WordPress Core

Opened 10 years ago

Closed 10 years ago

#18589 closed defect (bug) (fixed)

class WP_Http_Curl missing support for DELETE method

Reported by: jbrinley Owned by: dd32
Milestone: 3.4 Priority: normal
Severity: normal Version:
Component: HTTP API Keywords: has-patch needs-testing 3.4-early
Focuses: Cc:

Description

WP_Http_Curl ignores any request methods other than HEAD, POST, and PUT, treating anything else as GET.

I'm particularly interested in the DELETE method, although a reasonable case could be made for also handling OPTIONS and TRACE.

The switch statement in WP_Http_Curl::request() just needs a new case for each:

case 'DELETE':
	curl_setopt( $handle, CURLOPT_CUSTOMREQUEST, 'DELETE' );
	break;
case 'OPTIONS':
	curl_setopt( $handle, CURLOPT_CUSTOMREQUEST, 'OPTIONS' );
	break;
case 'TRACE':
	curl_setopt( $handle, CURLOPT_CUSTOMREQUEST, 'TRACE' );
	break;

Attachments (2)

18589.diff (668 bytes) - added by jbrinley 10 years ago.
Patch to add DELETE, OPTIONS, and TRACE to WP_Http_Curl
wordpress-curl-http-custom-method-fix.patch (522 bytes) - added by zx2c4 10 years ago.
A much better more generic solution to support all sorts of methods.

Download all attachments as: .zip

Change History (20)

@jbrinley
10 years ago

Patch to add DELETE, OPTIONS, and TRACE to WP_Http_Curl

#1 @jbrinley
10 years ago

  • Keywords has-patch added

#2 @dd32
10 years ago

  • Keywords needs-testing added

We'll need to ensure that the way those requests are handled by Curl, is the same as the way it's handled by the other transports.

#3 follow-up: @dd32
10 years ago

Closed duplicate: #18727 - wp_remote_request with HTTP Curl Class Does Not Support Custom HTTP Methods

The patch noted there of simply setting default: curl_setopt( $handle, CURLOPT_CUSTOMREQUEST, $r['method'] ); seems like a reasonable option at first glance.

#4 in reply to: ↑ 3 @jbrinley
10 years ago

Replying to dd32:

The patch noted there of simply setting default: curl_setopt( $handle, CURLOPT_CUSTOMREQUEST, $r['method'] ); seems like a reasonable option at first glance.

Agreed. zx2c4's patch looks like a better, more flexible option, and it's more in line with how the other transports handle arbitrary methods.

#5 @dd32
10 years ago

  • Owner set to dd32
  • Status changed from new to accepted
  • Type changed from enhancement to defect (bug)

Marking as a defect as the curl class handles them differently than the others.

It seems no validation occurs on the other classes at all, with Curl limiting it to GET/POST/HEAD/PUT.

@zx2c4
10 years ago

A much better more generic solution to support all sorts of methods.

#6 @zx2c4
10 years ago

I'm satisfied now; it's been sufficiently tested for my standards:

My patch has been running on a site that receives ~300k hits a week for the last couple days with zero regressions.

#7 @carlospaulino
10 years ago

I tested this and it works perfectly.

#8 @carlospaulino
10 years ago

Can this be included on the next release ?

#9 @sorich87
10 years ago

  • Type changed from defect (bug) to enhancement

#10 @zx2c4
10 years ago

  • Type changed from enhancement to defect (bug)

This is most certainly not an enhancement, but rather a bug, as it introduces a significant regression, since prior versions of wordpress contained the correct functionality, and so 3rd party code that depended on that functionality is now broken. This is a bug, a regression. This is not a mere enhancement request.

#11 @nacin
10 years ago

I agree this is a bug, but where is the evidence that it is a regression?

#12 follow-up: @zx2c4
10 years ago

It worked with the previous version of wordpress. This is widely accepted. Popular plugins such as w3 cache rely on it (for the Varnish support).

You acknowledged that it is a bug now.

worked on previous version + is a bug now = what we call a regression

#13 in reply to: ↑ 12 @dd32
10 years ago

  • Keywords 3.4-early added
  • Milestone changed from Awaiting Review to Future Release

Replying to zx2c4:

It worked with the previous version of wordpress. This is widely accepted. Popular plugins such as w3 cache rely on it (for the Varnish support).

You acknowledged that it is a bug now.

worked on previous version + is a bug now = what we call a regression

It has never worked in previous versions reliably, WP_HTTP has 3 different transports which might be used depending on server configuration, The Curl handler has never handled non-GET/HEAD/POST requests. The other transports have supported it, but simply by coincidence. The now-removed HTTP Extension transport didn't support it either.

This is a defect, however, not a regression.

#14 @zx2c4
10 years ago

The default chosen handler supported it. Now the default chosen one doesn't. But sure. Maybe it's been partially broken (unimplemented) all along, and it just never surfaced until now.

#15 @dd32
10 years ago

  • Milestone changed from Future Release to 3.4

#16 @dd32
10 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In [19696]:

Support custom HTTP Methods in WP_Http_Curl. Brings it in line with the other HTTP transports of respecting the requested method. Props zx2c4. Fixes #18589

#17 @dd32
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

One critical part was left out from this change, GET variables are processed as expected, however the body being sent (that we usually refer to as POST data) isn't being sent.

As a result, using methods such as PATCH which expect a body, causes the remote server to wait for the data.. which isn't going to be sent. causing a timeout. Ignoring the timeout issue, custom requests are useless without adding the body to the request.

#18 @dd32
10 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In [20183]:

WP_HTTP: Send the body with custom method requests when using cURL. Fixes #18589

Note: See TracTickets for help on using tickets.