WordPress.org

Make WordPress Core

#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)

37843.patch (1.3 KB) - added by ocean90 13 months ago.
37843.diff (3.3 KB) - added by rmccue 12 months ago.
Attach Requests hooks to native WP hooks
37843.2.diff (3.3 KB) - added by rmccue 12 months ago.
Updated instantiation with params

Download all attachments as: .zip

Change History (28)

#1 @swissspidy
13 months ago

  • Keywords dev-feedback added

#2 @ptasker
13 months ago

Voting for this one as well. Would be good to be able to filter the curl_setopt() flags again.

#3 @JRGould
13 months 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 @rekmla
13 months 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 @smerriman
13 months 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.

Last edited 13 months ago by smerriman (previous) (diff)

@ocean90
13 months ago

#6 @ocean90
13 months 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:

rmccue:

[…] 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 @dd32
13 months 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 @jaredh123
12 months 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 @SteveNetRivet
12 months 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 @lalunecreative
12 months 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 @rmccue
12 months ago

  • Keywords needs-patch added; dev-feedback removed
  • Owner set to rmccue
  • Status changed from new to assigned

@rmccue
12 months ago

Attach Requests hooks to native WP hooks

#12 @rmccue
12 months 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 @jaredh123
12 months 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.

@rmccue
12 months ago

Updated instantiation with params

#14 follow-up: @rmccue
12 months ago

@jaredh123 Whoops, forgot to include that change in my patch. Updated in 37843.2.diff.

#15 in reply to: ↑ 14 @jaredh123
12 months 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!

#16 @ocean90
12 months ago

#38205 was marked as a duplicate.

#17 follow-up: @reidbusi
12 months 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.

Last edited 12 months ago by reidbusi (previous) (diff)

#18 @reidbusi
12 months 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 @rmccue
12 months 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 @reidbusi
12 months 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.

Last edited 12 months ago by reidbusi (previous) (diff)

#21 @reidbusi
12 months 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 @dd32
12 months 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.


11 months ago

#24 @dd32
11 months 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.

#25 @dd32
10 months ago

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

In 39212:

WP_HTTP: Map internal Requests hooks to WordPress actions.
This change introduces a requests-{$hook} action which can be used to hook into Requests hooks, and restores the http_api_curl action.

Props rmccue.
Fixes #37843.

Note: See TracTickets for help on using tickets.