Make WordPress Core

Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#33055 closed task (blessed) (fixed)

Support Parallel HTTP Requests in WP_Http, et al

Reported by: wonderboymusic's profile wonderboymusic Owned by: rmccue's profile rmccue
Milestone: 4.6 Priority: high
Severity: normal Version:
Component: HTTP API Keywords: needs-patch
Focuses: performance Cc:

Description

HTTP-driven applications need to be performant. Being able to make multiple HTTP requests asynchronously is vital to making this a reality. Currently, WordPress only allows us to make one request at a time, serially:

$req1 = wp_remote_get( 'http://failwhale.com/posts', [ 'timeout' => 2 ] );
$req2 = wp_remote_get( 'http://failwhale.com/burritos', [ 'timeout' => 2 ] );

Wouldn't it be cool if we could do:

$responses = wp_remote_get( [
    [ 'http://api.bandsintown.com/artists/Ed%20Sheeran.json?app_id=woo', [ 'timeout' => 2 ] ],
    [ 'http://api.bandsintown.com/artists/Taylor%20Swift.json?app_id=woo', [ 'timeout' => 2 ] ]
] );

My patch does this by doing the following:

  • Introduces WP_Http_Request
  • If an array is passed to WP_Http::request(), automatically calls WP_Http::request_multi()
  • When multiple requests are made, hashes the request to the maintain a reference generated by WP_Http::hash_request()
  • Adds WP_Http::_get_first_available_multi_transport() to initially support cURL only, we could also add support for PECL HTTP and HttpRequestPool
  • If an array of data is passed to WP_Http::_dispatch_request(), calls ->request_multi() instead of ->request() on the approved multi transport
  • In WP_Http_Curl, does kung fu and jiu jitsu at the same time to dynamically support streaming of multiple requests simultaneously via ->__call()
  • Implements cURL Multi

Attachments (5)

33055.diff (36.1 KB) - added by wonderboymusic 9 years ago.
33055.2.diff (428.3 KB) - added by rmccue 9 years ago.
Replace WP_Http internals with Requests
33055.3.diff (428.0 KB) - added by rmccue 9 years ago.
Updated patch
33055.4.diff (213.4 KB) - added by rmccue 9 years ago.
Updated Requests, deduplicated certificates
33055.5.diff (211.4 KB) - added by rmccue 9 years ago.
Remove hook adapter

Download all attachments as: .zip

Change History (80)

@wonderboymusic
9 years ago

#1 @wonderboymusic
9 years ago

  • Keywords needs-patch added; has-patch needs-refresh removed
  • Milestone changed from Future Release to 4.4

My patch rips into the existing classes, but it's just a POC ....

I have been chatting with @rmccue and @dd32, we should look into the viability of things like:
https://github.com/rmccue/Requests-WPHTTP

We need a solution for parallel HTTP if we want to encourage people to use WP for a web-service driven website, example: a site powered by a REST API backend

#2 @rmccue
9 years ago

I am fully on board with replacing WP_Http with Requests. It's totally possible with a small compatibility layer (as you can see in the link), and we'd gain a bunch of stuff: high test coverage, more hooks, less dependency on WP core (and hence, more reusable outside of WP; e.g. wp-cli uses Requests to download WP), and PSR-7 compatibility (in progress at the moment).

Requests needs one or two slight changes, but already matches the vast majority of the features in WP_Http. (This makes sense, seeing as I've worked on both. ;) )

If we want to make this happen, I'm in. This needs buy-in from at least @dd32 on the core side though.

#3 follow-up: @dd32
9 years ago

I am 100% for replacing our WP_HTTP library with another, be it Requests or another well-maintained library.

My only requirements are:

  • 100% API compatibility for existing features
  • Properly maintained and used by others
  • Great test coverage

All should be easy for any contender to hit..

Some of the existing hooks can go away in the process IMHO if they don't make sense - http_api_curl for example, and http_api_debug to a lesser degree.

I expect there could be compatibility issues with some edge-case things we currently support - URL Blocking / Cookie handling for example, but if we have to include some minimal handling of those within a compat wrapper.. I'm good with that if it reduces our overall maintenance burden.

A note on https://github.com/rmccue/Requests-WPHTTP - That's written to add Requests as a transport provider to our existing WP_HTTP classes, if we were to adopt it, we'd instead rip out our existing stuff, and add a compat wrapper in it's place instead.

#4 in reply to: ↑ 3 @rmccue
9 years ago

  • Owner set to rmccue
  • Status changed from new to assigned

Replying to dd32:

I expect there could be compatibility issues with some edge-case things we currently support - URL Blocking / Cookie handling for example, but if we have to include some minimal handling of those within a compat wrapper.. I'm good with that if it reduces our overall maintenance burden.

I don't think these ones should be too hard, since they're mostly at a higher level than the actual HTTP requests.

A note on https://github.com/rmccue/Requests-WPHTTP - That's written to add Requests as a transport provider to our existing WP_HTTP classes, if we were to adopt it, we'd instead rip out our existing stuff, and add a compat wrapper in it's place instead.

Indeed; everything after the pre_http_request can be replaced with what I have in Requests-WPHTTP (keeping the pre filter, of course).

#5 @wonderboymusic
9 years ago

Anecdote: I spent a couple of hours trying to backport Guzzle to PHP 5.2, just for laughs. It is/was/will be next-to-impossible because of the Promises/A+ implementation and rampant use of static closures and parent scope. I even invented a proxy object that would mimic the callbacks, eventually hit a wall. Turns out Requests might be our best bet here.

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


9 years ago

This ticket was mentioned in Slack in #core-restapi by rachelbaker. View the logs.


9 years ago

#8 @wonderboymusic
9 years ago

  • Type changed from enhancement to task (blessed)

Had a lovely chat with @rmccue about this yesterday

#9 @rmccue
9 years ago

I've been working on making this happen. Requests has now been updated to add the few things it was missing that WP_Http has (byte limiting being the main one), and I've fixed a few other bugs.

Running Requests-WPHTTP now only causes one test failure, which is due to 302 redirects: WP_Http follows browser behaviour, in which a 302 redirect changes POST to GET; Requests follows the spec behaviour, which is to keep the same method unless otherwise stated. (The spec states that 302 handling MAY change the method, but only for browser compat.)

I talked to @dd32 about this, and I think we're both happy with leaving it and changing the WP test instead; WP_Http isn't a browser, so it doesn't need to emulate one.

Code coverage is at 87%; I'm planning to get this up a bit further, but it beats WP_Http hands down in that regard.

If we're happy, I'll whip up a patch and we can keep this moving.

#10 @wonderboymusic
9 years ago

WHIP UP A PATCH. I'll let this ride for a few weeks anyways.

@rmccue
9 years ago

Replace WP_Http internals with Requests

#11 @rmccue
9 years ago

Initial patch for Requests added, Mr @wonderboymusic.

In this first patch, I've done essentially no changes that remove existing code. This means there's now a bunch of WP_Http that isn't relevant or used, but I'm erring heavily on the side of caution here. If you'd prefer, I can gut these methods. (WP_Http::make_absolute_url can also be replaced with Requests_IRI::absolutize as well.)

We can also remove the HTTP tests from the core tests if you'd like, since they're all covered by Requests' tests already. This will fix the occasional failures we get in the tests as well, which is a bonus.

This needs sign-off from @dd32.

#12 @rmccue
9 years ago

  • Keywords has-patch added; needs-patch removed

Oh, I forgot to address the primary point of this ticket too: you can now also do parallel requests by using Requests directly:

// Setup what we want to request
$requests = array(
	array(
		'url' => 'http://httpbin.org/get',
		'headers' => array('Accept' => 'application/javascript'),
	),
	'post' => array(
		'url' => 'http://httpbin.org/post',
		'data' => array('mydata' => 'something'),
	),
	'delayed' => array(
		'url' => 'http://httpbin.org/delay/10',
		'options' => array(
			'timeout' => 20,
		),
	),
);

$options = array(
	'complete' => function my_callback(&$request, $id) {
		var_dump($id, $request);
	},
);

$responses = Requests::request_multiple($requests, $options);

#13 @wonderboymusic
9 years ago

  • Keywords 4.5-early added
  • Milestone changed from 4.4 to Future Release

as per @dd32

#14 @dd32
9 years ago

For reference, I'd prefer this goes in at the start of a dev cycle.
However, my suggestion is to commit it now during beta and remove it before release (Still haven't had a chance to review the patch) to see if it breaks anything.

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


9 years ago

#16 @rmccue
9 years ago

  • Milestone changed from Future Release to 4.5

#17 @kirasong
9 years ago

What's the status on this?

#18 @kirasong
9 years ago

  • Focuses performance added
  • Keywords early needs-refresh added; has-patch 4.5-early removed
  • Milestone changed from 4.5 to Future Release

I'd love to see this land, but it's too late in the cycle.

Punting towards the future.

#19 follow-up: @ocean90
9 years ago

Some random thoughts about 33055.2.diff:

  • Requests uses spl_autoload_register() so we have to provide a fallback in case SPL is disabled.
  • WP_Requests_Hooks needs some whitespace love. Is it part of Request or WP? Asking because of the @package tag and because it's in the root of /wp-includes/.
  • /wp-includes/requests/Requests/ looks weird. Can we move it up one dir?
  • What's the status of https://github.com/rmccue/Requests-WPHTTP/issues/1?
  • Are they any other tickets which could be marked as fixed as soon as Requests lands? (https://core.trac.wordpress.org/query?status=!closed&component=HTTP+API)
  • Can we get a refresh of the patch so it could potentially land on day one of 4.6?
  • "We can also remove the HTTP tests from the core tests if you'd like, since they're all covered by Requests' tests already. " I wouldn't remove them directly. Let's make sure that our tests are passing too and remove them only when the implementation is finished.

#20 in reply to: ↑ 19 @rmccue
9 years ago

Replying to ocean90:

Some random thoughts about 33055.2.diff:

  • Requests uses spl_autoload_register() so we have to provide a fallback in case SPL is disabled.

See #36335; I want to add a shim as part of that ticket, which would handle it nicely. If that doesn't ship, should be easy enough to do.

  • WP_Requests_Hooks needs some whitespace love. Is it part of Request or WP? Asking because of the @package tag and because it's in the root of /wp-includes/.

WP; I'll correct the whitespacing.

  • /wp-includes/requests/Requests/ looks weird. Can we move it up one dir?

We can, but then Requests.php needs to live somewhere, and may be a bit harder to update from upstream. I guess if we have to, a class-requests.php?

In-progress; Requests issue 178 handles the 302, although 9 discusses why we might not want to fix that. I'll round these up this week.

Yes. At a glance, the following should be fixed:

  • Can we get a refresh of the patch so it could potentially land on day one of 4.6?

Will-do.

  • "We can also remove the HTTP tests from the core tests if you'd like, since they're all covered by Requests' tests already. " I wouldn't remove them directly. Let's make sure that our tests are passing too and remove them only when the implementation is finished.

Makes sense.

@rmccue
9 years ago

Updated patch

#21 follow-up: @rmccue
9 years ago

Updated patch:

  • Introduces WP_HTTP_Requests_Response, which acts as a backwards-compatible return value with ArrayAccess while allowing access to the underlying object.
  • Uses a new requests.before_redirect hook to change POST to GET on a 302 redirect.
  • Updates the tests to be less strict about type checking, as we're using ArrayAccess objects rather than actual arrays
  • Fixes the 304 test - 304 is not a redirect, it's a Not Modified response. (It essentially means "redirect to internal cached")
  • Moves wp-includes/requests/* up, so we now have wp-includes/class-requests.php and wp-includes/Requests/

#22 @rmccue
9 years ago

  • Keywords dev-feedback added; early needs-refresh removed
  • Milestone changed from Future Release to 4.6

Waiting for @dd32's review. I'll also update Requests to the latest before we commit.

#23 @ocean90
9 years ago

  • Keywords has-patch added
  • Priority changed from normal to high

#24 follow-up: @dd32
9 years ago

After reading through 33055.3.diff the only real concern I have so far is the duplication of the SSL Certificates file.
Talking with @rmccue reveals several options, removing it from Requests as-is may break code relying upon Requests, removing it from WordPress may break WordPress BC, so we're looking for something in the middle.

I'm sure there's edge cases and bugs, but we can iterate on the approach as it's still early.

#25 in reply to: ↑ 24 ; follow-up: @ocean90
9 years ago

  • Keywords early added

Replying to dd32:

After reading through 33055.3.diff the only real concern I have so far is the duplication of the SSL Certificates file.

Isn't the "in the middle" part that we pass WP's certificates to Requests per default if one of the wp_remote_* functions is used?

#26 in reply to: ↑ 25 @dd32
9 years ago

Replying to ocean90:

Replying to dd32:

After reading through 33055.3.diff the only real concern I have so far is the duplication of the SSL Certificates file.

Isn't the "in the middle" part that we pass WP's certificates to Requests per default if one of the wp_remote_* functions is used?

If a plugin, or an external package used within a plugin, uses Requests:: directly then unless we ship the ca-bundle with Requests too then it'll be unable to SSL.

So we could rely upon Requests bundled SSL certs instead (and update it as need be), but if we remove our copy of it and a plugin is referencing it directly, things could break.

There's two possible options in my mind:
a) in wp_remote_* detect if wp-includes/certificate/* was passed in, and if so, ignore the parameter
b) in Requests allow some way of defining the path to the SSL certs (@rmccue's idea there) - either through one of the ini settings PHP has in modern versions, through a constant, or a static property on Requests etc.
I'm leaving that part up to @rmccue's best judgement

#27 @rmccue
9 years ago

I'm going to patch Requests to allow a global path to the CA certs, since hosts might also want to set that themselves, so it's pretty generally useful anyway.

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


9 years ago

@rmccue
9 years ago

Updated Requests, deduplicated certificates

@rmccue
9 years ago

Remove hook adapter

#29 @rmccue
9 years ago

Updated patch; this includes the changes to Requests, and removes the hook adapter. Hooks in Requests are scoped to a single request/session, and the internal plugins (Basic auth, proxying, etc) rely on this fact. Attaching the hooks to WP actions/filters breaks this.

I'll follow this up in a new ticket in the future.

#30 @rmccue
9 years ago

In 37428:

HTTP API: Replace internals with Requests library.

Requests is a library very similar to WP_HTTP, with a high level of unit test coverage, and has a common lineage and development team. It also supports parallel requests.

See #33055.

#31 @rmccue
9 years ago

In 37429:

HTTP API: Add browser compatibility hook for 3xx redirects.

WordPress erroneously follows browser-style behaviour with 3xx redirects, where a POST to 302 becomes a GET. Requests instead follows the specification and keeps the same method. Requests also exposes a hook to allow changing the behaviour.

[37428] used the wrong method of adding this hook, now corrected.

See #33055.

#32 @rmccue
9 years ago

In 37430:

HTTP API: Fix compatibility with cURL <7.22

Ensure connections are closed after usage to fix an issue with WP.com URLs used in the tests.

Resynched from GitHub at 95518ce.

See #33055.

#33 follow-ups: @rmccue
9 years ago

We now have Requests in core, but we still need the additional layer in WP_HTTP to support parallel requests without requiring use of Requests directly.

#34 @kraftbj
9 years ago

Fatal error: Cannot use object of type Requests_Response as array in /home/kraftbj/public_html/wp-includes/http.php on line 255

That line in http.php: if ( is_wp_error($response) || ! isset($response['response']) || ! is_array($response['response'])) of wp_remote_retrieve_response_code.

Making Requests_Response implements ArrayAccess and adding the needed functions seem to resolve it for me, but unfamiliar with the internals of Requests to know if that's a feasible solution or just duct tape.

#35 @rmccue
9 years ago

@kraftbj The offsetGet callback should be handling this already. I can dig into this, but unsure why that wouldn't be getting called, unless something is casting the response to an array earlier?

#36 @rmccue
9 years ago

Aha, tracked down the issue: http_api_debug is called with the raw Requests_Response object instead of the array-like object.

#37 @rmccue
9 years ago

In 37436:

HTTP API: Pass array-like object to http_api_debug.

This was mistakingly passing the Requests_Response object, which caused fatal errors with debugging tools.

See #33055.

#38 @dd32
9 years ago

#36847 was marked as a duplicate.

#39 in reply to: ↑ 33 @ocean90
9 years ago

  • Keywords needs-patch added; dev-feedback has-patch early removed

Replying to rmccue:

We now have Requests in core, but we still need the additional layer in WP_HTTP to support parallel requests without requiring use of Requests directly.

#40 follow-ups: @kovshenin
9 years ago

Unfortunately this broke cron spawning across wordpress.org because of the 10 milliseconds timeout, which used to be rounded up to 1 second prior to CURLOPT_TIMEOUT_MS. Ten milliseconds is often not enough to perform a DNS request, or a TLS handshake. If we still want to support milliseconds, we should increase the timeout when spawning the WordPress cron.

#41 in reply to: ↑ 40 @ocean90
9 years ago

Replying to kovshenin:

Ten milliseconds is often not enough to perform a DNS request, or a TLS handshake. If we still want to support milliseconds, we should increase the timeout when spawning the WordPress cron.

Related: #8923, #18738

#42 in reply to: ↑ 21 @aaroncampbell
9 years ago

Replying to rmccue:

Updated patch:

  • Introduces WP_HTTP_Requests_Response, which acts as a backwards-compatible return value with ArrayAccess while allowing access to the underlying object.

Actually, it doesn't seem that this is completely back-compat. I ran into this because we had code like the following:

$response = wp_remote_get( $url );

if ( is_wp_error( $response ) ) {
        return $response;
}

if ( ! is_array( $response ) || ! isset( $response['body'] ) ) {
        return new WP_Error( 'custom_error' );
}

The issue is that PHP's is_array() returns false for objects that implement ArrayAccess. Obviously the old return was a real array, so it worked. Changing to check instanceof ArrayAccess as well as is_array() seems to work with old and new:

if ( ! ( $response instanceof ArrayAccess || is_array( $response ) ) || ! isset( $response['body'] ) ) {
        return new WP_Error( 'custom_error' );
}

We might want to try to make devs aware as soon as possible, and maybe even check the plugin dir.

#43 in reply to: ↑ 40 ; follow-up: @rmccue
9 years ago

Replying to kovshenin:

Unfortunately this broke cron spawning across wordpress.org because of the 10 milliseconds timeout, which used to be rounded up to 1 second prior to CURLOPT_TIMEOUT_MS. Ten milliseconds is often not enough to perform a DNS request, or a TLS handshake. If we still want to support milliseconds, we should increase the timeout when spawning the WordPress cron.

This definitely sounds like a misuse of the HTTP API; if you're specifying a 10ms timeout, then you should get that. Expecting it to round up to a second is using undefined behaviour. Let's fix it in the other tickets though. :)

Replying to aaroncampbell:

The issue is that PHP's is_array() returns false for objects that implement ArrayAccess. Obviously the old return was a real array, so it worked.

Technically speaking, yeah, it's a BC break there. Annoyingly, is_array() doesn't check ArrayAccess :'(

However, the HTTP API only ever returned either a WP_Error or an array (now an error or an array-like object), so there's no real need to check the type at all. I get the idea of defensively programming here, but it has the result of making the code brittle to changes.

#44 @ocean90
9 years ago

Related: #36969, a possible regression caused by [37428].

#45 in reply to: ↑ 40 @ocean90
9 years ago

Replying to kovshenin:

Unfortunately this broke cron spawning across wordpress.org because of the 10 milliseconds timeout, which used to be rounded up to 1 second prior to CURLOPT_TIMEOUT_MS.

Previously: #11505, [12472]

#46 in reply to: ↑ 43 ; follow-up: @Otto42
9 years ago

Replying to rmccue:

This definitely sounds like a misuse of the HTTP API; if you're specifying a 10ms timeout, then you should get that. Expecting it to round up to a second is using undefined behaviour. Let's fix it in the other tickets though. :)

I agree, however if you're specifying anything less than half a second, this code would make your timeout get set to zero:

https://core.trac.wordpress.org/browser/trunk/src/wp-includes/Requests/Transport/cURL.php?rev=37428#L351

curl_setopt($this->handle, CURLOPT_TIMEOUT_MS, round($options['timeout'] * 1000));

Same for line 358.

Having a case where somebody specifies a low timeout and then having it interpret that in a way which causes the entire library to do nothing but twiddle its thumbs seems like a bad idea.

Also, the curl documentation says that the minimum timeout allowed is 1 second.

https://curl.haxx.se/libcurl/c/CURLOPT_TIMEOUT_MS.html

If libcurl is built to use the standard system name resolver, that portion of the transfer will still use full-second resolution for timeouts with a minimum timeout allowed of one second.

#47 in reply to: ↑ 46 @rmccue
9 years ago

Replying to Otto42:

I agree, however if you're specifying anything less than half a second, this code would make your timeout get set to zero:

https://core.trac.wordpress.org/browser/trunk/src/wp-includes/Requests/Transport/cURL.php?rev=37428#L351

curl_setopt($this->handle, CURLOPT_TIMEOUT_MS, round($options['timeout'] * 1000));

timeout is a float value for the number of seconds; this code multiplies it up to milliseconds, then rounds to the nearest millisecond.

e.g. if I specify 'timeout' => 0.0121, this code multiplies that up to 12.1, then rounds to an integer millisecond value of 12

Also, the curl documentation says that the minimum timeout allowed is 1 second.

https://curl.haxx.se/libcurl/c/CURLOPT_TIMEOUT_MS.html

If libcurl is built to use the standard system name resolver, that portion of the transfer will still use full-second resolution for timeouts with a minimum timeout allowed of one second.

cURL internally rounds this up if necessary, and that only affects the name resolution (i.e. DNS), not the actual transfer time. That note is just a warning that specifying 1ms doesn't guarantee it'll only take 1ms :)

#48 @Otto42
9 years ago

cURL internally rounds this up if necessary

In the past (6-7 years ago), we found that not to always be the case. While I have not done any tests recently or tested all known versions of libcurl, it has been shown that giving too low of a timeout to a call to curl will result in curl simply not making the resulting web request at all.

Basically, if you send at least some versions of curl a timeout value lower than 1 second, then the HTTP GET request never actually happens. It doesn't "timeout", because it never actually times in to start with. It simply returns, having done nothing whatsoever.

#49 @ocean90
9 years ago

  • Keywords needs-dev-note added

#50 @jtsternberg
9 years ago

Regarding this introduction, it would probably be a good idea to inform developers that they should no longer be explicitly checking for an array from the wp_remote_* functions. Casting the result of the wp_remote_* calls will also fail.

#51 follow-up: @mnelson4
8 years ago

Regarding this introduction, it would probably be a good idea to inform developers that they should no longer be explicitly checking for an array from the wp_remote_* functions. Casting the result of the wp_remote_* calls will also fail.

+1. This change isn't exactly 100% backwards compatible so it might be good to bring this more to the community's attention.

This just broke our plugin's paypal integration because we had an is_array($response) check (which just needs to be changed to is_array($response) || $response instanceof ArrayAccess). Glad we caught this before 4.6 got released, hopefully we can push a fix to our plugins and have all our users update before they update WP core to 4.6 because then it will break for them on live too.

Version 0, edited 8 years ago by mnelson4 (next)

#52 in reply to: ↑ 51 ; follow-up: @dd32
8 years ago

Replying to mnelson4:

This just broke our plugin's paypal integration because we had an array_key_exists( 'body', $response) check (which just needs to be changed to isset( $response[ 'body' ] )).

For anyone else who runs into this, please don't continue with an is_array/ArrayAccess check, which is the wrong thing to do.
Instead, do something like the following:

$request = wp_remote_get(..);
if ( ! is_wp_error( $request ) ) {
   ... Request succeeded, perform operations
}
OR:
if ( $headers = wp_remote_retrieve_headers() ) {
  $body = wp_remote_retrieve_body()
  ... act upon $headers/$body
}

#53 in reply to: ↑ 52 @mnelson4
8 years ago

thanks for the clarification @dd32

#54 follow-up: @giuseppe.mazzapica
8 years ago

HTTP API is a public API, and since the API exists all public API methods are documented to return an array, so I think is not a good idea just switch the return type.

If I am not wrong, WordPress core itself used to check is_array for HTTP API response. It's not that hard to imaginate a lot of code out there is doing the same.

Moreover, changing return type also means that any code using type-hint is going to break.

For example:

function do_something_with_response(array $response) {
  /* ... */
}


$response = wp_remote_get( /* ... */ );

if ( is_wp_error( $response ) ) {
  // do something with error
} else {
  do_something_with_response( $response );
}

The code above uses is_error what seems to be the correct way to check for proper response, but still fails, because the changed return type.

Even if there are functions like wp_remote_retrieve_body() or wp_remote_retrieve_headers() and similar, they are considered just "helpers" in official documentation (see Codex: https://codex.wordpress.org/HTTP_API#Helper_Functions).

English is not my native tongue, but I think that "helpers" != "mandatory" and more so "helpers" != "recommended", or am I wrong?

Developers, because reasons, expect WordPress to be backward compatible even in its internal API, surely they expect to be strongly backward compatible in public API.

At least, I do. And I found myself pretty "safe" using a return type of a public API method for my type-hint.

Changing return type of an entire API methods is a huge backward compatibility break.

I, for myself, am pretty sure 90% of the my code that uses HTTP API is going to break because of this change.

My point is: why don't convert to array the object returned by Requests and so be 100% backward compatible?

In that case would also be possible to add an argument, e.g. something like response_type and defaults to array and let users decide if they want an object.

In the other cases, convert Request_Response to array is very easy, something like:

$response = Requests::request( /* ... */ );

if ( isset( $args['response_type'] ) && OBJECT == $args['response_type'] ) ) {
  return $response;
}

$response_array = get_object_vars( $response );

// headers is an instance of Requests_Response_Headers
$response_array['headers'] = iterator_to_array( $response_array['headers']->getIterator() );

// cookies is an instance of Requests_Cookie_Jar
$response_array['cookies'] = iterator_to_array( $response_array['cookies']->getIterator() )

return $response_array;

WordPress did much worse things for backward compatibility.

Last edited 8 years ago by giuseppe.mazzapica (previous) (diff)

#55 in reply to: ↑ 54 @dnaber-de
8 years ago

I couldn't agree more with giuseppe.mazzapica here. Basically, backward incompatible changes are no big deal when they are transparent. As WordPress doesn't follow the rules of semantic versioning, those changes wouldn't be very transparent. Further, WordPress is known for a strict focus on backward compatibility in the past.

#56 @Otto42
8 years ago

There seems to be a rather large number of issues with this change, overall. Backwards compat breaks, cron going haywire, curl incompatibilities.

If this were any other change, then it would not seem to be ready for inclusion in a major release, is what I'm getting at. I get that it is a better library, but leaving this in core when it's causing so many problems seems something like favoritism here.

These issues need to be addressed. Backwards compatibility, especially on publicly documented functions and not internal-only calls, needs to be maintained. Behaviors like curl timeouts need to be maintained.

Making a library to replace the old HTTP calls, but which also straight up breaks nearly every plugin that actually uses those calls, doesn't seem like a good candidate for core inclusion.

Why is this still in trunk and not reverted already? It has already broken major parts of WordPress.org.

#57 @toscho
8 years ago

The class Requests_Auth_Basic says it implements Requests_Auth, but it has two unnecessary public members (making the object mutable) and three public methods which are not specified in that interface. These additional methods either need a separate interface or set to be private. Otherwise using them must be seen as deprecated by default. This is not a good design. :)

#58 @DrewAPicture
8 years ago

I have to agree with feedback about the back-compat break in terms of type checking. We can't just change it in the name of "this is a better way", at least not without a broader roll-out and time to educate developers about "best practice".

Up until [37428], even core was type checking for an array. So to imagine in some way that core isn't used as the reference point for best-practice, is to ignore a basic tenet of the core/extension model.

We can certainly change how core consumes its internal APIs internally, but in the case of internal APIs exposed publicly in any way, it has to be slower, and it has to be more deliberate: wp_remote_* can't be allowed to break, and I'm not sure why we'd allow that to happen here when we actively promote backward compatibility in every other established API.

#59 @mordauk
8 years ago

I'm also going to echo the sentiments of concern regarding the breaking changes.

Since I work primarily in eCommerce, I'll use that as my example. Easy Digital Downloads, and numerous other eCommerce plugins, use the WP HTTP API for handling remote requests to merchant processors. I know that my team has always expected responses to be arrays since day one. That's what WordPress gave us so that's what we used.

I went through all of our plugins today and updated them to be fully compatible with both the current and the newly proposed versions.

I am not the typical plugin developer. I pay attention to trac and the make blogs so that I know these kind of changes are happening, but most plugin developers will never have known this change is coming and that could result in really bad and unforseen consequences.

While using ImplementArrayAccess does take care of a lot of cases, there are always going to times when request responses were strictly checked for is_array. When that happens, the request process will fail.

In the realm of eCommerce, that could potentially means ridiculously large numbers of failed or missed transactions, and that is a risk I simply do not believe is justified.

I love what the new version provides and I really want to see it move forward, but I feel it needs to happen in multiple steps, not one giant and abrupt change.

#60 @rmccue
8 years ago

One of the reasons we wanted to roll this in early is to find the breakages, and it seems like we're finding a bit. :)

Let's change the return type back to the array and expose the Response object another way.

There are still other smaller issues to be fixed as well; the cURL timeout is an issue I've been looking into, and think I may have a solution for. I'm not sure of any other breakage apart from these two issues though.

#61 @rmccue
8 years ago

Split the return compatibility issue off to #37097 to switch back to an array.

#62 @rmccue
8 years ago

In 37694:

HTTP API: Update Requests.

This introduces a minimum value of 1 second for timeouts passed to cURL.

Internally, cURL uses alarm() for interrupts, which accepts a second-resolution timeout. Any values lower than 1 second are instantly failed rather than being rounded upwards. While this makes the experience worse for those using asynchronous DNS lookups, there's no way to detect which DNS resolver is being used from PHP.

See #33055, #8923.

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


8 years ago

#64 @dsquared
8 years ago

I'm thrilled that this is finally occurring....

I'm curious, could something like this be done for css and/or javascript in 4.7?

Or plugin conflicts??? Like sliders.......

I made a post with this question recently, I provide the link below:
https://wordpress.org/support/topic/eliminate-render-blocking-javascript-and-css-in-above-the-fold-content-34?replies=1

Please advise. Thank you for your thoughts?

I look forward to 4.6

#65 @rmccue
8 years ago

In 37989:

HTTP API: Switch back to returning an array.

The array-compatibility object we started returning in r37428 unfortunately isn't enough like an array. In particular, is_array() checks fail, despite the object implementing ArrayAccess. Mea culpa.

This moves the WP_HTTP_Response object to a new http_response key in the array, and changes the value back to an actual array.

Fixes #37097.
See #33055.

#66 @ocean90
8 years ago

In 38053:

HTTP API: Update Requests.

Fixes an issue where you couldn't set a Requests_Proxy_HTTP object as a proxy setting.

See https://github.com/rmccue/Requests/pull/223.
See #37107, #33055.

#67 @ocean90
8 years ago

In 38054:

HTTP API: Pass proxy settings to Requests.

WP_HTTP_Proxy() is used directly in WP_Http_Curl() and WP_Http_Streams(). Since WP_Http::request() doesn't use them anymore we have to move the proxy handling into WP_Http::request() so the proxy data can be passed to Requests::request().

Props rmccue.
See #33055.
Fixes #37107.

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


8 years ago

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


8 years ago

#70 in reply to: ↑ 33 @ocean90
8 years ago

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

Replying to rmccue:

We now have Requests in core, but we still need the additional layer in WP_HTTP to support parallel requests without requiring use of Requests directly.

Moved into #37459.

Closing this ticket as fixed for 4.6. Please create new tickets for new issues.

#71 @ocean90
8 years ago

In 38163:

HTTP API: Bump version of Requests to 1.7.

See #33055.

#72 @ocean90
8 years ago

  • Keywords needs-dev-note removed

#73 follow-up: @Hrohh
8 years ago

Hi, please, now I have WP4.6 and my filter "http_api_curl" dont work at all. How can I use this action? Thx

#74 in reply to: ↑ 73 @rekmla
8 years ago

Replying to Hrohh:

Hi, please, now I have WP4.6 and my filter "http_api_curl" dont work at all. How can I use this action? Thx

See this ticket.https://core.trac.wordpress.org/ticket/37843

#75 @dd32
8 years ago

#22952 was marked as a duplicate.

Note: See TracTickets for help on using tickets.