Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#37843 closed defect (bug) (fixed)

`http_api_curl` hook no longer available for adding custom cURL options

Reported by: chasewiseman's profile ChaseWiseman Owned by: rmccue's profile 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 8 years ago.
37843.diff (3.3 KB) - added by rmccue 8 years ago.
Attach Requests hooks to native WP hooks
37843.2.diff (3.3 KB) - added by rmccue 8 years ago.
Updated instantiation with params

Download all attachments as: .zip

Change History (29)

#1 @swissspidy
8 years ago

  • Keywords dev-feedback added

#2 @ptasker
8 years ago

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

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

Last edited 8 years ago by smerriman (previous) (diff)

@ocean90
8 years ago

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

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
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 @jaredh123
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 @SteveNetRivet
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 @lalunecreative
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 @rmccue
8 years ago

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

@rmccue
8 years ago

Attach Requests hooks to native WP hooks

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

@rmccue
8 years ago

Updated instantiation with params

#14 follow-up: @rmccue
8 years ago

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

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

#16 @ocean90
8 years ago

#38205 was marked as a duplicate.

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

Last edited 8 years ago by reidbusi (previous) (diff)

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

Last edited 8 years ago by reidbusi (previous) (diff)

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

#25 @dd32
8 years 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.

#26 @lkraav
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.

Last edited 7 years ago by lkraav (previous) (diff)
Note: See TracTickets for help on using tickets.