#37843 closed defect (bug) (fixed)
`http_api_curl` hook no longer available for adding custom cURL options
Reported by: | ChaseWiseman | Owned by: | rmccue |
---|---|---|---|
Milestone: | 4.7 | Priority: | normal |
Severity: | major | Version: | 4.6 |
Component: | HTTP API | Keywords: | has-patch |
Focuses: | Cc: |
Description
Now that 4.6 has implemented the new Requests library, the http_api_curl
hook is no longer used when building cURL requests, and I see that its removal was mentioned and expected in #33055. That said, as far as I can tell there is no longer an easy way to intercept before curl_exec
is run. There are a lot of posts and projects that suggest this old hook for setting options like CURLOPT_SSLVERSION
and the like, so I think it'd be beneficial to maintain this ability.
The only way around it I've found is to skip wp_remote_request()
and roll your own Requests::request()
call and pass it the $options['hooks']
after registering your own callback for the curl.before_send
hook.
Would it make sense to add a filter to $options
before calling Requests::request()
so folks can register their own callbacks, similar to this https://core.trac.wordpress.org/browser/trunk/src/wp-includes/class-http.php#L311? Or perhaps there is another way of going about it that I'm missing, and I'm happy to work up a patch if someone more familiar with the library can suggest a good place for this.
Thanks!
Attachments (3)
Change History (29)
#3
@
8 years ago
The HTTP API currently feels pretty crippled without the ability to set curl options or utilize Requests' hooks in general. Adding a filter on $options
with an additional $url
arg just before calling Requests::request()
would be incredibly helpful.
#4
@
8 years ago
We're also at a loss as to how we'll access curl_setopt
without giving up on using wp_remote_request()
in certain situations.
#5
@
8 years ago
I'm in the same boat here - our server is unable to correctly route IPv6 addresses. This happens regularly, for example, with Facebook graph requests.
We avoid this by using http_api_curl to set the CURL_IPRESOLVE option to CURL_IPRESOLVE_V4. WP4.6 has broken this.
#6
@
8 years ago
- Milestone changed from Awaiting Review to 4.7
Another report: https://github.com/rmccue/Requests/issues/233.
#33055 had a hook adapter WP_Requests_Hooks
but it was removed before the final commit:
[…] 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.
See also [37429].
37843.patch allows to define your own hooks for custom requests. But this still doesn't allow to customize all requests per default. A filter like @JRGould has proposed makes probably more sense here.
@dd32, @rmccue: What are your thoughts on this?
#7
@
8 years ago
What are your thoughts on this?
It was intentional to drop support for that hook in my mind.
We can restore the action easily enough by passing a custom hook to Requests and then firing our action(s) for BC.
#8
@
8 years ago
This is affecting us pretty badly too. We have a percentage of our users unable to get updates from our servers now that we cannot intercept the curl request and customize it.
I would love to see this make it into 4.6.x instead of 4.7, because this feels like a pretty major bug/regression, not a feature. We'll be hurting for a while if this isn't possible until 4.7.
#9
@
8 years ago
+1
Our users rely on auto-update of our theme software for bug fixes and feature releases.
Until this is resolved - hopefully in the near future, during 4.6 and not waiting for 4.7 - a large group of our users won't be able to receive our patches.
#10
@
8 years ago
- Severity changed from normal to major
+1 I'm also adding to this one, our site uses auto updates for bug fixes and features for our custom theme which is currently in a broken state until this gets resolved.
#11
@
8 years ago
- Keywords needs-patch added; dev-feedback removed
- Owner set to rmccue
- Status changed from new to assigned
#12
@
8 years ago
- Keywords has-patch added; needs-patch removed
Added a patch that adds a requests-{$hook}
dynamic hook for WordPress usage of Requests internal hooks. Anything in Requests will be mapped to this automatically (e.g. curl.before_send
will be available as requests-curl.before_send
).
Additionally, it also maps the curl.before_send
hook to the old http_api_curl
hook.
Please note: if you're hooking into this, please consider filtering something else, as your code won't work if the user doesn't have cURL installed. The point of WP_Http is to abstract the need for cURL away, so using this hook is a bad idea.
#13
@
8 years ago
@rmccue I tried to test your patch, but got errors. Looks like your WP_Requests_Hooks
class is supposed to be constructed with some parameters, but your code doesn't pass any in.
#14
follow-up:
↓ 15
@
8 years ago
@jaredh123 Whoops, forgot to include that change in my patch. Updated in 37843.2.diff.
#15
in reply to:
↑ 14
@
8 years ago
Replying to rmccue:
@jaredh123 Whoops, forgot to include that change in my patch. Updated in 37843.2.diff.
@rmccue tested again with updated diff on one of our users sites experiencing problems, and it worked well. Thanks for your time and attention to this!
#17
follow-up:
↓ 19
@
8 years ago
Wouldn't this be a lot simpler?:
WP 4.6.1 wp-includes\class-http.php lines 299-314:
// Setup arguments
$headers = $r['headers'];
$data = $r['body'];
$type = $r['method'];
$options = array(
'timeout' => $r['timeout'],
'useragent' => $r['user-agent'],
'blocking' => $r['blocking'],
'hooks' => new Requests_Hooks(),
);
// Ensure redirects follow browser behaviour.
$options['hooks']->register( 'requests.before_redirect', array( get_class(), 'browser_redirect_compatibility' ) );
// Allow registration of additional Requests hooks
apply_filters( 'http_api_requests_lib_hooks', $options['hooks'] );
Simply added one comment and a one line apply_filters call. I just tested this approach and it works fine for me. I don't really see the need for anything more complicated unless I am missing something?
Thus in my plugin I can just do this:
if ( version_compare( $wp_version, "4.6", ">=" ) ) {
if ( class_exists( 'Requests' ) ) {
add_filter( 'http_api_requests_lib_hooks', 'rpco_http_api_requests_lib_hooks', PHP_INT_MAX, 1 );
}
} else {
add_action( 'http_api_curl', 'rpco_http_api_curl', PHP_INT_MAX, 1 );
}
// ...
function rpco_http_api_requests_lib_hooks( $hooks ) {
$hooks->register( 'curl.before_send', 'rpco_http_api_curl', PHP_INT_MAX );
}
Also...:
Please note: if you're hooking into this, please consider filtering something else, as your code won't work if the user doesn't have cURL installed. The point of WP_Http is to abstract the need for cURL away, so using this hook is a bad idea.
Thanks for the advice, but I'd prefer if you leave that to us:
https://wordpress.org/plugins/reid-plugins-curl-options/ (the whole point of the plugin is to be able to control PHP cURL)
e.g.:
if ( function_exists( 'curl_version' ) ) {
// do cURLy stuff
} else {
echo '<p>PHP cURL is not installed or enabled on your server, this plugin will not do anything.</p>';
}
I suppose I can test this patch, but I'm not convinced so complicated/convoluted a solution is required.
Also, like others, I would like to see this fixed before WP 4.7. In my opinion, once the best solution is settled, it is worth an immediate minor version release.
#18
@
8 years ago
It occurs to me that my one-liner filter suggested above only addresses the http(s) protocol case. I will proceed to test rmmcue's patch, which in theory should handle other protocols as well. However, I am concerned by the special case applied to curl.before_send, the most important one (especially to me and my clients).
#19
in reply to:
↑ 17
@
8 years ago
Replying to reidbusi:
Wouldn't this be a lot simpler?
This enshrines the hooks object into the WordPress API, and would make it harder if we wanted to migrate away from it later (or if Requests changes the format).
Thanks for the advice, but I'd prefer if you leave that to us:
That's why the action still exists. In 99% of cases, you're best off using the higher level abstractions for compatibility.
#20
@
8 years ago
Thanks for the explanation! Makes sense. So now I can just add a regular WP action inside a WP version conditional like so:
if ( version_compare( $wp_version, "4.6", ">=" ) ) {
if ( class_exists( 'Requests' ) ) {
add_action( 'requests-curl.before_send', 'rpco_http_api_curl', PHP_INT_MAX, 1 );
}
} else {
add_action( 'http_api_curl', 'rpco_http_api_curl', PHP_INT_MAX, 1 );
}
That makes it pretty simple and straightforward. I have tested your posted patch (on CentOS 6.8) and it is behaving as expected and desired.
Current PHP version: 7.0.9 cURL version: 7.19.7 Host: x86_64-redhat-linux-gnu IPv6 support: yes Kerberos V4 support: no SSL support: yes SSL version: NSS/3.21 Basic ECC libz HTTP deflate support: yes libz version: 1.2.3 Protocols: tftp, ftp, telnet, dict, ldap, ldaps, http, file, https, ftps, scp, sftp
That's why the action still exists. In 99% of cases, you're best off using the higher level abstractions for compatibility.
Thanks for keeping it, because CentOS is pretty popular with hosts and as of CentOS 7, cURL still uses NSS instead of OpenSSL, and depending on the build environment cURL may or may not be able to auto-negotiate SSL versions and cipher suites, which are pretty critical to eCommerce websites.
Edit: so now with a little more testing, we just need to get this pushed out as soon as possible, like others above, I feel that waiting for WP v4.7 might be too long to let it go like it is.
#21
@
8 years ago
I would like to point out, I believe that some developers have already abandoned the WP HTTP API because of this issue.
2016.08.30 - version 1.0.7
Tweak - Replace all wp_remote_requests with raw cURL.
https://woocommerce.com/products/square/
Note the date. Business must go on, regardless of what happens to the platform it is running on.
I admit, this is speculation on my part since I do not have access to the development repository, but the timing of such a change sure looks like this is the reason.
Actually, looking back through the versions I have saved, I see a comment from 1.0.6:
// if posting image we need to use regular curl because
// for some unknown reason wp_remote_request does not work.
// @TODO: Need to look deeper to see why wp_remote_request cannot do this.
So I would suspect that given this issue, and the change to the WP core HTTP API, they just went with straight PHP cURL for greater control and reliability regardless of what happens to the WP core functionality.
Note the author of WooCommerce Square:
Author: Automattic
I don't want to seem ungrateful for anyone's contributions to WP, they are all appreciated, but I feel it is important to point these kinds of things out. Things need to be more carefully thought through and more thoroughly tested in my opinion.
#22
@
8 years ago
- Milestone changed from 4.7 to 4.6.2
@rmccue Patch approach seems good to me, is there anything else you'd like to change here?
For the 4.6 backport (which is needed, at least for the curl.before_send
=> http_api_curl
handling) the class should be suffixed to an existing file to abide by our no-new-files-in-minor-releases policy for autoupdates.
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
8 years ago
#24
@
8 years ago
- Milestone changed from 4.6.2 to 4.7
pushing back into the 4.7 milestone as at this point it doesn't look like a 4.6.2 will be released before 4.7, and we can review at the point of commit.
@rmccue I'll ping you directly to chat about this and work out what we're doing.
#26
@
7 years ago
r39212 was never backported to 4.6? Why not, because the change looks miniscule enough? Just had a nasty surprise for a client where core version upgrade strategy is a bit on the slow side.
EDIT I manually backported this on 4.6.10 right now. Everything works and I'm not seeing any bad side effects.
Voting for this one as well. Would be good to be able to filter the curl_setopt() flags again.