Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#37503 closed defect (bug) (fixed)

Requests doesn't pass through custom methods in cURL transport

Reported by: ipstenu's profile Ipstenu Owned by: rmccue's profile rmccue
Milestone: 4.6 Priority: normal
Severity: normal Version: 4.6
Component: HTTP API Keywords: upstream has-patch commit
Focuses: Cc:

Description

Passing custom methods through wp_remote_request is defaulting to GET instead of whatever the method actually is.

This code:

$response = wp_remote_request($purgeme, array('method' => 'PURGE', 'headers' => array( 'host' => $p['host'], 'X-Purge-Method' => $varnish_x_purgemethod ) ) );

Would output a GET instead of a PURGE for example. It's a method used by quite a lot of Varnish related plugins and would break caching for them. Or rather, it would make caching stop flushing. (There's a saving water in California joke in there somewhere.)

Thanks to @ocean90 for pointing out that this:

https://github.com/rmccue/Requests/blob/125746209802f03316a3ee2d872df19044735a13/library/Requests/Transport/cURL.php#L331-L350

was missing the defaults found here:

https://core.trac.wordpress.org/browser/trunk/src/wp-includes/class-wp-http-curl.php?rev=37492&marks=163-167#L158

Attached is a patch that adds a default back. If I use a method of PURGE, PURGE is sent. If I do not specify a method, GET is sent.

Looking at it, perhaps lines 336-342 could be removed, as that is the same as the default?

Attachments (3)

37503.patch (607 bytes) - added by Ipstenu 8 years ago.
Adding in a default to the CURL calls
37503.2.patch (1.1 KB) - added by ocean90 8 years ago.
37503.3.patch (1.3 KB) - added by ocean90 8 years ago.

Download all attachments as: .zip

Change History (15)

@Ipstenu
8 years ago

Adding in a default to the CURL calls

#1 @ocean90
8 years ago

  • Keywords upstream added
  • Milestone changed from Awaiting Review to 4.6
  • Owner set to rmccue
  • Status changed from new to reviewing

Created a pull request as well: https://github.com/rmccue/Requests/pull/227/

If this doesn't get accepted for some reasons we might have to look at the existing hooks, like curl.before_request. Sadly those don't provide any context.

#2 @ocean90
8 years ago

  • Summary changed from Request API doesn't pass through custom methods to Requests doesn't pass through custom methods in cURL transport

#3 @Ipstenu
8 years ago

  • Keywords has-patch added

@ocean90
8 years ago

#4 @ocean90
8 years ago

  • Owner changed from rmccue to dd32

37503.2.patch removes the case for PATCH, PUT, DELETE and OPTIONS since it's the same as the default case.

I'm unsure if we need a check for the $data value here. It was previously wrapped with a is_null() condition. From the API documentation:

If CURLOPT_POSTFIELDS is explicitly set to NULL then libcurl will get the POST data from the read callback. If you want to send a zero-byte POST set CURLOPT_POSTFIELDS to an empty string, or set CURLOPT_POST to 1 and CURLOPT_POSTFIELDSIZE to 0.
https://curl.haxx.se/libcurl/c/CURLOPT_POSTFIELDS.html

PHP source: https://github.com/php/php-src/blob/f7df40f45a17f23b982f01d4c07ae784d5171414/ext/curl/interface.c#L2769-L2787

@dd32 What do you think about this? Would like to have this for RC2.

#5 @dd32
8 years ago

I don't see anything wrong with the approach here.

#6 @ocean90
8 years ago

  • Owner changed from dd32 to rmccue

Thanks @dd32, moving owner back to @rmccue.

@ocean90
8 years ago

#8 @ocean90
8 years ago

  • Keywords commit added

This ticket was mentioned in Slack in #core by ocean90. View the logs.


8 years ago

#10 @dd32
8 years ago

Upstream has been merged, this is safe for 4.6 commit.

#11 @dd32
8 years ago

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

In 38191:

Requests: Merge handling for custom HTTP methods.

This merges the latest changes to Requests from upstream to add support for custom HTTP methods.
See https://github.com/rmccue/Requests/commit/1b5ffd8501503e0641b7fa0c555ccf99973135e2

Props Ipstenu, ocean90.
Fixes #37503 for trunk.

#12 @dd32
8 years ago

In 38192:

Requests: Merge handling for custom HTTP methods.

This merges the latest changes to Requests from upstream to add support for custom HTTP methods.
See https://github.com/rmccue/Requests/commit/1b5ffd8501503e0641b7fa0c555ccf99973135e2

Props Ipstenu, ocean90.
Merges [38191] to the 4.6 branch.
Fixes #37503 for 4.6.

Note: See TracTickets for help on using tickets.