WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 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 6 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 6 years ago.
A much better more generic solution to support all sorts of methods.

Download all attachments as: .zip

Change History (20)

@jbrinley
6 years ago

Patch to add DELETE, OPTIONS, and TRACE to WP_Http_Curl

#1 @jbrinley
6 years ago

  • Keywords has-patch added

#2 @dd32
6 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
6 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
6 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
6 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
6 years ago

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

#6 @zx2c4
6 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
6 years ago

I tested this and it works perfectly.

#8 @carlospaulino
6 years ago

Can this be included on the next release ?

#9 @sorich87
6 years ago

  • Type changed from defect (bug) to enhancement

#10 @zx2c4
6 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
6 years ago

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

#12 follow-up: @zx2c4
6 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
6 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
6 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
6 years ago

  • Milestone changed from Future Release to 3.4

#16 @dd32
6 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
6 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
6 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.