#33055 closed task (blessed) (fixed)
Support Parallel HTTP Requests in WP_Http, et al
Reported by: | wonderboymusic | Owned by: | 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 callsWP_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 andHttpRequestPool
- 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)
Change History (80)
#1
@
9 years ago
- Keywords needs-patch added; has-patch needs-refresh removed
- Milestone changed from Future Release to 4.4
#2
@
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:
↓ 4
@
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
@
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
@
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
@
9 years ago
- Type changed from enhancement to task (blessed)
Had a lovely chat with @rmccue about this yesterday
#9
@
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.
#11
@
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
@
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
@
9 years ago
- Keywords 4.5-early added
- Milestone changed from 4.4 to Future Release
as per @dd32
#14
@
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
#18
@
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:
↓ 20
@
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
@
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
?
- What's the status of https://github.com/rmccue/Requests-WPHTTP/issues/1?
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.
- 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)
Yes. At a glance, the following should be fixed:
- #13841 and #32932 - Requests checks specifically for timeouts
- #17010 - Requests hides the 100 Continue from the end-user, as it's for internal handling only.
- #33271 - Requests uses
wb
mode rather thanw+
- #34053 - Pretty sure Requests isn't susceptible to this.
- #34538 - Requests uses ip2long instead of a regex.
- #36253 - Requests handles this differently
- 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.
#21
follow-up:
↓ 42
@
9 years ago
Updated patch:
- Introduces
WP_HTTP_Requests_Response
, which acts as a backwards-compatible return value withArrayAccess
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 havewp-includes/class-requests.php
andwp-includes/Requests/
#22
@
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.
#24
follow-up:
↓ 25
@
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:
↓ 26
@
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
@
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
@
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
#29
@
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.
#33
follow-ups:
↓ 39
↓ 70
@
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
@
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
@
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
@
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.
#39
in reply to:
↑ 33
@
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:
↓ 41
↓ 43
↓ 45
@
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.
#42
in reply to:
↑ 21
@
9 years ago
Replying to rmccue:
Updated patch:
- Introduces
WP_HTTP_Requests_Response
, which acts as a backwards-compatible return value withArrayAccess
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:
↓ 46
@
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.
#46
in reply to:
↑ 43
;
follow-up:
↓ 47
@
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:
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
@
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:
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
@
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.
#50
@
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:
↓ 52
@
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.
+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 array_key_exists( 'body', $response)
check (which just needs to be changed to isset( $response[ 'body' ] )
). 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.
#52
in reply to:
↑ 51
;
follow-up:
↓ 53
@
9 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 toisset( $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 }
#54
follow-up:
↓ 55
@
9 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.
#55
in reply to:
↑ 54
@
9 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
@
9 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
@
9 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
@
9 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
@
9 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
@
9 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.
This ticket was mentioned in Slack in #core by ocean90. View the logs.
8 years ago
#64
@
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
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
#73
follow-up:
↓ 74
@
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
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