WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 6 years ago

#8727 closed enhancement (fixed)

WP_Http_Curl doesn't handle cookies

Reported by: Denis-de-Bernardy Owned by:
Milestone: 2.8 Priority: normal
Severity: normal Version: 2.7
Component: HTTP API Keywords: has-patch dev-feedback
Focuses: Cc:

Description

WP_Http_Curl doesn't handle cookies properly, and I see no means to "fix things up" using a plugin hook.

for information, note that some hosts fail when using curl_setop() for CURLOPT_COOKIEJAR and CURLOPT_COOKIEFILE.

I worked around this in one of my own classes at one point. The code I used is the following:

curl_setopt($ch, CURLOPT_HEADERFUNCTION, array('sem_http', 'curl_cookies'));

with:

	#
	# curl_cookies()
	#
	# via http://www.php.net/manual/en/function.curl-setopt.php#70043
	#

	function curl_cookies($ch, $string)
	{
		global $sem_curl_cookies;

		# ^overrides the function param $ch
		# this is okay because we need to
		# update the global $ch with
		# new cookies

		$length = strlen($string);

		if ( !strncmp($string, "Set-Cookie:", 11) )
		{
			# get the cookie
			$cookiestr = trim(substr($string, 11, -1));
			$cookie = explode(';', $cookiestr);
			$cookie = explode('=', $cookie[0]);
			$cookiename = trim(array_shift($cookie));
			$sem_curl_cookies[$cookiename] = trim(implode('=', $cookie));
		}

		if ( trim($string) == "" )
		{
			$cookie = "";

			foreach ( (array) $sem_curl_cookies as $key => $value )
			{
				$cookie .= "$key=$value; ";
			}

			$sem_curl_cookies = array();

			#dump(curl_getinfo($ch, CURLINFO_EFFECTIVE_URL), $cookie);

			curl_setopt($ch, CURLOPT_COOKIE, $cookie);
		}

		return $length;
	} # curl_cookies()

a few remarks on the above, since WP has a broader user base than the handful of customers I distributed the above to:

  • you'll want to use a class variable instead of a global variable
  • strlen isn't multi-byte safe, so there might need some tweaking here
  • $cookie .= "$key=$value; "; may need some attention. I'm uncertain how well curl behaves when $key or $value contain strings that might need to be urlencoded.

else it works fine...

Attachments (1)

8727.diff (2.6 KB) - added by jacobsantos 7 years ago.
Debugging actions and action for curl object.

Download all attachments as: .zip

Change History (17)

@jacobsantos7 years ago

Debugging actions and action for curl object.

comment:1 @jacobsantos7 years ago

See if line #1023 helps you out?

comment:2 @jacobsantos7 years ago

  • Keywords has-patch dev-feedback reporter-feedback added
  • Type changed from defect (bug) to enhancement

Does this work. I included some debugging. I was going to include some other filters, but I've seen they were already added.

comment:3 @jacobsantos7 years ago

  • Component changed from Administration to Plugins
  • Owner anonymous deleted

comment:4 @jacobsantos7 years ago

What the patch does is send a reference to the variable through the do_action_by_ref(), which allows for further modification from a plugin. There are also other methods for adding cookie details in WP_Http::request() method.

comment:5 follow-up: @Denis-de-Bernardy7 years ago

k, so if I get this right, I can catch the curl handle using this:

add_action('http_api_curl', 'foo');

and:

function foo(&$bar)
{
  // do stuff on $bar
}

correct?

if so, that works, I suppose, but I still think cookies should be handled out of the box for consistency. snoopy handled them quite well if I remember correctly.

comment:6 @jacobsantos7 years ago

  • Component changed from Plugins to HTTP

comment:7 @jacobsantos7 years ago

Denis-de-Bernardy:

I think it might be acceptable, since Gzip support will be handled out of the box also. The fundamental design of the HTTP API, was to support the basics of what 95% of what people will use. Any new feature has to be tested and supported on all of the five transports, therefore any feature will require additional development time and testing.

Given that it is HTTP, the testing environment is quite painful and slow. Authentication should also be handled out of the box, but that will most likely come in 2.9.

comment:8 in reply to: ↑ 5 @jacobsantos7 years ago

Replying to Denis-de-Bernardy:

k, so if I get this right, I can catch the curl handle using this:

add_action('http_api_curl', 'foo');

and:

function foo(&$bar)
{
  // do stuff on $bar
}

correct?

Yes.

comment:9 @jacobsantos7 years ago

Can this be committed to 2.8 first?

comment:10 @ryan7 years ago

(In [10281]) HTTP API debug and action for curl object. Props jacobsantos. see #8727

comment:11 follow-up: @sarekofvulcan7 years ago

When I updated to the nightly today, all of the blog pages I checked showed the following message at the top:

Warning: Call-time pass-by-reference has been deprecated; If you would like to pass it by reference, modify the declaration of do_action_ref_array(). If you would like to enable call-time pass-by-reference, you can set allow_call_time_pass_reference to true in your INI file in /home/.vuitton/sarekofvulcan/donnael.com/blog/wp-includes/http.php on line 1038

I've put a block comment around the do_action_ref_array() call for now, but I'd like to know if I should change my settings somewhere to make this work properly, or if it's a bug in the function call. Thanks.

comment:12 in reply to: ↑ 11 @jacobsantos7 years ago

Replying to sarekofvulcan:

When I updated to the nightly today, all of the blog pages I checked showed the following message at the top:

Warning: Call-time pass-by-reference has been deprecated; If you would like to pass it by reference, modify the declaration of do_action_ref_array(). If you would like to enable call-time pass-by-reference, you can set allow_call_time_pass_reference to true in your INI file in /home/.vuitton/sarekofvulcan/donnael.com/blog/wp-includes/http.php on line 1038

I've put a block comment around the do_action_ref_array() call for now, but I'd like to know if I should change my settings somewhere to make this work properly, or if it's a bug in the function call. Thanks.

See #8701.

comment:13 @Denis-de-Bernardy6 years ago

  • Keywords reporter-feedback removed

I'm still itching to suggest that the cookies should be managed out of the box, so as to have a consistent behavior throughout the http transports.

comment:14 @jacobsantos6 years ago

No, I agree, but there doesn't seem to be an overwhelming support for enhancements in 2.8 for HTTP API (enhancements usually cause bugs, which need to be fixed). 2.9 will most likely see it.

comment:15 @ryan6 years ago

  • Milestone changed from 2.7.1 to 2.8

comment:16 @ryan6 years ago

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

(In [10512]) Cookie support for HTTP API. Props beaulebens. fixes #9049 #9037 #8727

Note: See TracTickets for help on using tickets.