WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 7 years ago

#4779 closed enhancement (fixed)

Proposal for HTTP POST and REQUEST API

Reported by: darkdragon Owned by: jacobsantos
Milestone: 2.7 Priority: normal
Severity: normal Version:
Component: Optimization Keywords:
Focuses: Cc:

Description

Given that some hosts are paranoid, it would be a good idea to have a failover for fsockopen and/or fopen.

Given some of my tests and passed experience there are three ways to attempt a HTTP call that works on both PHP 4.3+ and PHP 5 and a few more ways added to just PHP 5.

  • Curl (if extension loaded)
  • Fopen (if ini_settings allows for this. however, PHP 4 and PHP 5 has an extra ini setting for allowing fopen and closing include(_once)/require(_once)
  • Sockets (fsockopen) (second prefer for PHP 4)
  • Streams (prefer for PHP 5 and PHP 4)
  • HTTP (PHP 5.2) (if exists, and if anyone knows how to use it, then use this for PHP 5.2)

If none of these work, then just spit out some error saying the hosts sucks or something.

What about Snoopy?

Snoopy supports two of the five, so either expand upon Snoopy, do proper testing and use it instead or just basically hack a few working functions up.

I would choose to hack a few functions up, given that it would be probably easier to add the remaining three to Snoopy. There should still be a function or two for which can be refactored later to use Snoopy.

Something about Tuesday

Matt said something about Tuesday, but I'm unsure how quickly I can get this API out by myself... Actually, I kid, should only take about 3 hours, tops. Except for HTTP as I have no idea how to work for it.

Attachments (35)

http.php (15.7 KB) - added by darkdragon 8 years ago.
Prototype of HTTP Request API
http.r6350.patch (17.9 KB) - added by darkdragon 7 years ago.
Changes WordPress core functions and Admin over to new HTTP API, based off of r6350
4779.remote_http.diff (3.5 KB) - added by DD32 7 years ago.
4779.remote_http.2.diff (5.9 KB) - added by DD32 7 years ago.
test_http_api.5.2.php (12.8 KB) - added by jacobsantos 7 years ago.
Update for recent changes to the API
http.10.3.php (25.2 KB) - added by jacobsantos 7 years ago.
Fix ExtHTTP bug
4779.core.r8512.diff (4.8 KB) - added by jacobsantos 7 years ago.
Converts spawn_cron() and wp_version_check() based off of r8512
http.10.4.php (26.2 KB) - added by jacobsantos 7 years ago.
Updated HTTP API with parameter switching for options before headers and body.
test_http_api.5.3.php (12.8 KB) - added by jacobsantos 7 years ago.
Test cases for HTTP API
4779.formatting+phpdoc.diff (26.3 KB) - added by DD32 7 years ago.
4779.formatting+phpdoc.2.diff (26.6 KB) - added by DD32 7 years ago.
4779.formatting2+non-blocking.diff (4.9 KB) - added by DD32 7 years ago.
4779.r8519.diff (21.5 KB) - added by santosj 7 years ago.
Adds cURL transport, but does not add it to the list, because it needs to be tested first. Fixes documentation and non-blocking mode.
4779.r8521.diff (2.2 KB) - added by santosj 7 years ago.
Various fixes.
4779.r8522.diff (2.4 KB) - added by santosj 7 years ago.
Fixes process headers for fopen header return, fixes HTTP extension response array.
4779.r8522.2.diff (3.9 KB) - added by santosj 7 years ago.
Fixes process headers for fopen header return, fixes HTTP extension response array. Also adds Curl implementation and allows for it to be used when using headers.
4779.r8523.diff (3.9 KB) - added by santosj 7 years ago.
Last set of fixes that I can find and from testing. Need more live testing and test cases. Based off of r8523
4779.r8524.diff (453 bytes) - added by jacobsantos 7 years ago.
CRON uses wp_guess_url() to hopefully fix cron problem.
4779.encode.post.data.diff (1.2 KB) - added by DD32 7 years ago.
4779.encode.post.data.2.diff (1.4 KB) - added by DD32 7 years ago.
4779.r8529.diff (6.1 KB) - added by jacobsantos 7 years ago.
Cycle through available transports trying each one. Based off of r8529
4779.r8534.diff (463 bytes) - added by santosj 7 years ago.
Return response in foreach loop and remove debugging line. Based off of r8534
4779.fix.reversed.encode.diff (1.1 KB) - added by DD32 7 years ago.
4779.r8547.diff (1.1 KB) - added by santosj 7 years ago.
Combines fix for #7456 and fixes HTTP extension chunk format. Converts chunk to unchunked body.
4779.r8548.diff (650 bytes) - added by santosj 7 years ago.
Fixes chunk format based off of r8548
4779.r8557.diff (571 bytes) - added by santosj 7 years ago.
Fix notice about body not having chunk formatting.
4779.r8572.diff (542 bytes) - added by santosj 7 years ago.
Checks the headers for the transfer-encoding for chunked and then runs the function. based off of r8572
4779.r8582.diff (433 bytes) - added by santosj 7 years ago.
Fix Copy and Paste bug in cURL transport based off of r8582
4779.r8585.diff (1.8 KB) - added by santosj 7 years ago.
Fixes major issues with fsockopen transport, based off r8585
4779.fsockopen.post.content-headers.diff (693 bytes) - added by DD32 7 years ago.
Fixes POST requests to add the Content-length and Content-type headers with FSockOpen
4779.r8620.diff (22.0 KB) - added by santosj 7 years ago.
Patch merges headers and body into $args, also adds support for chunked transfer-encoding decoding, based off of r8620
4779.r8624.diff (28.6 KB) - added by santosj 7 years ago.
More fixes, implements chunked transfer decoding, other enhancements, based off of r8624
4779.r8629.diff (28.9 KB) - added by santosj 7 years ago.
More fixes, implements chunked transfer decoding, other enhancements. Add more sanity checking. Based off of r8629
4779.r8630.diff (3.5 KB) - added by santosj 7 years ago.
Add @since to chunked decoding, return actual body when chunked. Based off of r8630
http-tests.zip (17.3 KB) - added by santosj 7 years ago.
Complete HTTP API test suite

Download all attachments as: .zip

Change History (174)

comment:1 follow-up: @santosj8 years ago

This should not have 2.4 milestone.

comment:2 in reply to: ↑ 1 @Nazgul8 years ago

Replying to santosj:

This should not have 2.4 milestone.

Usually we fix/change things in trunk first (which corresponds with the current 2.4 milestone) and then backport it to previous releases if that's deemed necessary.

So this issue may end up fixed in a maintenance release for 2.3.x, but for now it's first slated for 2.4.

comment:3 @santosj8 years ago

I was thinking more that it doesn't have a patch and is non-trival.

comment:4 @santosj8 years ago

I'll probably bring it up on WP-Hackers and write up some code this weekend. I've been thinking about this ticket and the best way to go about it. I don't think it should be all in one function, but using classes might slow down something that should be made as quick as possible.

comment:5 @lloydbudd8 years ago

  • Milestone changed from 2.4 to 2.5

@darkdragon8 years ago

Prototype of HTTP Request API

comment:6 @darkdragon8 years ago

Changes:

  • Created wp_remote_register_transport() and wp_remote_unregister_transport() instead of using a custom Action hook. I might just wrap those two functions around the Action API, but needs references to object.
  • Finished four of the five http request methods (missing !cURL).
  • Added hooks for user agent changing, as well as setting the different time outs with filters.

Missing:

  • !cURL implementation (might be plugin material).
  • method to setup what each object can be used for, fopen for PHP 4 can only be used for reading and not writing. Might be useful to decide based on $type used.
  • Comprehensive Unit Tests (coming next).
  • Missing hooks for parsing headers and body after response is finished (figured some things are better left to the user of the functions, better to return raw, instead of mangling when I shouldn't be).

Found this after writing the first prototype. I like mine better, but okay. The guy has some good stuff and I ported some of it over.

Again, this is just a prototype.

comment:7 @darkdragon8 years ago

  • Keywords dev-feedback added

Totally sweet! It works! That just blows my mind.

Unit Test covers most of the functions, does not cover wp_remote_get_body() or wp_remote_get_headers(). The Unit Tests informally cover the WP_HTTP_Fsockopen class, the functions cover the test() and the wp_remote_get_object() covers the request() method. Which does prove that the class does work. However, to complete the unit tests, the WP_HTTP_Base class methods need to be covered, as well as the rest of the transport classes.

The next patch, if this is still considered okay, will include complete unit tests and replace Snoopy and transition areas that use fsockopen to use these functions. It will also include splitting the five classes down to one or two (cURL might be useful to be packaged also).

@darkdragon7 years ago

Changes WordPress core functions and Admin over to new HTTP API, based off of r6350

comment:8 @darkdragon7 years ago

  • Keywords has-patch needs-testing added; dev-feedback removed

Unit Tests cover 99.9% over the HTTP API, the API is finished and works. The patch that changes the core of WordPress needs testing, but I'm planning on writing Unit Tests for that. Also need to go over performance tuning, but I'm not sure how much that will do.

Parts that need consideration:

  • Using the Plugin API instead of custom array
  • Performance Fine-tuning

comment:9 @DD327 years ago

  • Keywords dev-feedback added

Is there any chance for 2.6 WordPress can standardise on external access?

Currently WordPress has:

  • Snoopy
  • fsockopen() in various function
  • Curl
  • fopen()
  • A Wrapper around Snoopy
  • A Wrapper around Curl && fopen() (I swear i saw that)

Every piece of code tends to use a different one, Not only does that mean that certain code may work while another doesnt, It also doesnt help with those who want to set a proxy server, If it was wrapped up into a single function/method, It would greatly help those users.

There was never much traction on this ticket from Devs, But it'd be good to get a final word as to if anyone is interested.

I'm not too sure if the patches on here are the best though, But that depends on the direction devs want to take it. I'd be just as happy if fsockopen() was bound together into a single function which everything used(I use this in a few plugins):

function remote_http($url, $method='GET', $data='', $headers = array()){
	//Note: This returns all headers in lowercase.
	global $wp_version;
	//Partly stolen from update checker code :)
	if( is_array($data) )
		$data = http_build_query($data);

	$allowed_methods = array('GET', 'POST', 'HEAD');
	if( ! in_array($method, $allowed_methods) ) $method = 'GET';

	$port = 80;
	$schema = 'http';
	$path = $host = $query = '';
	$parts = parse_url($url);
	extract($parts);

	$http_request  = "$method $path?$query HTTP/1.0\r\n";
	$http_request .= "Host: $host\r\n";
	$http_request .= 'Content-Type: application/x-www-form-urlencoded; charset=' . get_option('blog_charset') . "\r\n";
	if( $data )
		$http_request .= 'Content-Length: ' . strlen($request) . "\r\n";
	$http_request .= 'User-Agent: WordPress/' . $wp_version . '; ' . '; ' . get_bloginfo('url') . "\r\n";
	if( ! empty($headers) )
		foreach($headers as $header => $value)
			$http_request .= "$header: $value\r\n";
	$http_request .= "\r\n";
	if( $data )
		$http_request .= $request;

	$response = '';
	if( false == ( $fs = @fsockopen( $host, $port, $errno, $errstr, 3) ) || ! is_resource($fs) )
		return false;

	fwrite($fs, $http_request);

	while ( !feof($fs) )
		$response .= fgets($fs, 1160); // One TCP-IP packet
	fclose($fs);
	$response = explode("\r\n\r\n", $response, 2);
	
	$ret = (object)array('headers' => array(), 'content'=> $response[1]);

	foreach( explode("\n", $response[0]) as $rheader){
		list($header, $value) = explode(':', $rheader, 2);
		if( !empty($header) && ! empty($value) ) {
			$ret->headers[ strtolower(trim($header)) ] = trim($value);
		} else if( strpos($rheader, 'HTTP') > -1) {
			list(,$status, $status_name) = preg_split('|\s+|', $rheader);
			$ret->headers[ 'status' ] = $status;
			$ret->headers[ 'status-name' ] = $status_name;
		}
	}

	return $ret;
}

At least it could then be improved on in the future if need be, But why not do it right, and get it in there now?

comment:10 @jacobsantos7 years ago

Your code is beautiful DD32.

I've been looking to set the code to use a function instead, but couldn't drive myself to steep to that level. I didn't think it would combine the feature set, but alas, you were able to do it and quite well.

$http_request .= 'Content-Type: application/x-www-form-urlencoded; charset=' . get_option('blog_charset') . "\r\n";
  • Needs to be wrapped in a conditional.
  • The header support needs to also handle strings.

Other than that I think I can wrap the code in the patch into a pluggable function and create another patch.

I want to get this in and I would like to get rid of Snoopy.

comment:11 @DD327 years ago

Other than that I think I can wrap the code in the patch into a pluggable function and create another patch.

I might just make a few changes to the code as you sugest and submit as a patch instead? Unless you're allready half way there of course? :)

@DD327 years ago

comment:12 @DD327 years ago

attachment 4779.remote_http.diff added.

  • Includes PHPDoc
  • Fixed a number of warnings & bugs related to POST request
  • Status and Status Value are no longer returned in the headers list, but are properties themselves.

Example return value:

$a = remote_http('http://api.wordpress.org/core/version-check/1.1/');

var_dump($a);
object(stdClass)[70]
  public 'status' => string '200' (length=3)
  public 'statusvalue' => string 'OK' (length=2)
  public 'headers' => 
    array
      'x-powered-by' => string 'PHP/5.2.5' (length=9)
      'content-type' => string 'text/plain; charset=utf-8' (length=25)
      'content-length' => string '44' (length=2)
      'date' => string 'Sun, 25 May 2008 04:46:16 GMT' (length=29)
      'server' => string 'LiteSpeed' (length=9)
      'connection' => string 'close' (length=5)
  public 'content' => string 'upgrade
http://wordpress.org/download/&#' (length=44)

(note: content field is truncated via var_dump due to a bug in the XDebug extension in that example)

comment:13 @jacobsantos7 years ago

I have a few suggestions:

  1. Add another parameter which adds a few other options, one of which for what HTTP version to send, however I must admit version 1.0 is a lot easier to handle.
  2. If $headers is not an array, then it will be cast to an array. I would rather like this to be a string also. If that is the case, then other handling can be done. There is a function used in WordPress that will merge both strings or array into another array. Forget the name of it, but it is used throughout WordPress where string|array are used.
  3. The data will not always be URL encoded content, might be XML, so might need another function which handles data like that for the user to make it easier. However, I would leave that as an exercise for the developer. If I send XML, I don't want to have to hack around the current implementation.
  4. Your current fsockopen does not handle redirection correctly and leaves that as an exercise for the user. This either needs to be noted in the Documentation or handled within the function. The problem is that most other HTTP handlers handle this automatically, so it might be problematic to the user to have to handle it when it will be just for one HTTP handler. I would do it for the user. You can see my current code which handles it (kind of nasty code).
  5. There is a typo in the documentation

Other than that it seems to be to be pretty awesome. I would rather the dev team accept the object oriented approach, but I would rather see this ticket closed.

comment:14 follow-up: @DD327 years ago

Add another parameter which adds a few other options, one of which for what HTTP version to send, however I must admit version 1.0 is a lot easier to handle.

I'm not up to speed with the HTTP 1.1 spec, Content-Encoding: deflate/chunked/etc just confuse me from a client perspective. It'd be nice to support gzipped data if possible though i guess.

If $headers is not an array, then it will be cast to an array. I would rather like this to be a string also. If that is the case, then other handling can be done.

wp_parse_args() i believe? Yeah, That'd be a better option actually.

The data will not always be URL encoded content ..[snip].. If I send XML, I don't want to have to hack around the current implementation.

Thats handled allready, Pass a Array and its converted to a URL encoded query string. Pass a string and its assumed you know what you want to pass, so it'll post the exact Serialized data or XML. (Unless i've stupidly done something i've not noticed?)

Your current fsockopen does not handle redirection correctly and leaves that as an exercise for the user. ..[snip]

Point taken, It would be best to follow a certain ammount of redirects like some of the WP functions do.

There is a typo in the documentation

oops, thanks for noting that :)

I would rather the dev team accept the object oriented approach

I'd like to see something thats useable by all people accepted. I have a feeling one of the reasons the OO one hasnt been used so far, is that it *may* be seen to by some as being a more complicated option that what currently exists.

comment:15 in reply to: ↑ 14 @jacobsantos7 years ago

Replying to DD32:

I'm not up to speed with the HTTP 1.1 spec, Content-Encoding: deflate/chunked/etc just confuse me from a client perspective. It'd be nice to support gzipped data if possible though i guess.


I'm not saying that the function handle all of that, I'm saying that we allow that option for the developer instead of forcing 1.0. If the developer understands the spec, then they'll probably not want to recreate another function just to send data and parse it back. If the HTTP 1.1 spec requires that chunked data be handled within the function, then perhaps it is better to maintain the 1.0 version.

GZipped can still be handled by the developer. The goal is to just handle sending and receiving, the user has to determine what to do with the response after that. It would become a massive function or library if the function did everything for the developer.

The data will not always be URL encoded content ..[snip].. If I send XML, I don't want to have to hack around the current implementation.

Thats handled allready, Pass a Array and its converted to a URL encoded query string. Pass a string and its assumed you know what you want to pass, so it'll post the exact Serialized data or XML. (Unless i've stupidly done something i've not noticed?)


I rechecked and you are correct, it is completely possible. It should still be noted in the documentation that it is the case for future reference.


Your current fsockopen does not handle redirection correctly and leaves that as an exercise for the user. ..[snip]

Point taken, It would be best to follow a certain ammount of redirects like some of the WP functions do.


That could be part of the optional last option parameter with the default at three.

@DD327 years ago

comment:16 @DD327 years ago

attachment 4779.remote_http.2.diff added.

  1. Changed 2nd param from a simple Type selector to a generic options passer, most values can be changed via that.
  2. Added code to follow redirects.
  3. If a dev wants to, they can set any version of HTTP to use. They may also accept Gzip data/etc by setting the appropriate headers. (Via #1)
  4. Added Examples in the PHPDoc and corrected spelling mistakes
  5. Timeout for the connection can be specified (Via #1)

comment:17 @jacobsantos7 years ago

Pretty sweet.

One more thing, pluggable functions are wrapped using the colon...endif; blocks.

if( function_exists(...) :
// ...
endif;

and not with curly brackets.

comment:18 @DD327 years ago

One more thing, pluggable functions are wrapped using the colon...endif; blocks.

Ignoring the current pluggable.php format, Whats the stance on the use of endif; in WordPress?
The coding standards mearly state the preferential use of the Braces, the longhand format of if () : endif; is not mentioned, nor is it mentioned in the PEAR docs. It makes sense in some places in the code (Some of the template functions where a large ammount of code is not fully indented, or has many levels) But the longhand method is also used in some places where it's really doesnt make sense to use it.(No examples off hand)

Any core dev carification on that? if () { ... } for short blocks, if (): endif; for longer blocks?

I'm fine for whatever, But i wont bother updateing the patch unless a dev actually indicates that its likely to be commited(Or a bug/feature gets added to current patch :))

comment:19 @mdawaffe7 years ago

I think the WP standard is to always use braces except in the following two circumstances:

  1. When there is "raw" HTML within the block. That is, when there is closing and opening PHP tags ?>, <?php. This occurs in many templates and template functions.
  2. Wrapped around pluggable functions.

I'm sure that there are places where WP does not adhere to that standard.

As an aside, I think it's WP standard to use no braces at all for one line loops/conditionals:

if ( $foo )
  do_something();

foreach ( $bobs as $bob )
  bob_it( $bob );

That is probably less adhered to (and more controversial) than the braces/long format issue above.

comment:20 follow-up: @jacobsantos7 years ago

I mentioned it because it is the standard in the pluggables file. I don't think it matters whether the all encompassing standard is, but what the rest of file standard is and how it is done in that file.

I would rather not think that it should either one or the other. I believe the preference was chosen to not have to indent the functions in the if block.

comment:21 in reply to: ↑ 20 @jacobsantos7 years ago

Replying to jacobsantos:

I mentioned it because it is the standard in the pluggables file. I don't think it matters whether the all encompassing standard is, but what the rest of file standard is and how it is done in that file.

I would rather not think that it should either one or the other. I believe the preference was chosen to not have to indent the functions in the if block.

I totally need to get some sleep.

comment:22 @DD327 years ago

I totally need to get some sleep.

True :P But it makes sense.

I'm sure that there are places where WP does not adhere to that standard.

Yep, I was just wondering why that standard was different in the pluggables, and to a certain extend, the compat.php too(Which uses a combination).

The indentation changes are a good one to pointout actually jacob.

I've personally used this method previously (And is my prefered method, but doesnt fit in with WP):

if( ! function_exists() ) {
function ..() {
...
}} //Double curly close

I'll change the style over if theres any more grip from that patch.

comment:23 @jacobsantos7 years ago

Need to get on IRC.

comment:24 @ryan7 years ago

Falling back to other methods out-of-the-box, as per the original patches, would be nice. I shouldn't have to install a plugin because my fsockopen doesn't work.

comment:25 @DD327 years ago

See also:
#4011 (add global proxy support in options)

comment:26 @DD327 years ago

Looks like this isnt going to go in 2.6.. Can something be done about this early in 2.7? Something really needs to be standardised upon both for core, and for plugins to rely on.

comment:27 @jacobsantos7 years ago

I should point out that I would like to either see this ticket die or be implemented. I'm unsure what to make of this ticket. For the entire time I went with the object oriented approach I didn't receive a word of feedback. Now with the procedural implementation there is some feedback that basically says that the implementation should be changed, so that it looks like crap.

If the OO implementation was "The Suck," it isn't any worse or better than the current BackPress. I'm unsure whether or not a similar implementation is going to be written for BackPress and included in WordPress.

If I may be curious and submit a second proposal with the object oriented approach along with the BackPress style along with the procedural implementation which implements all four or five methods.

You would think a fully documented API with test cases would score at least some points!

comment:28 @DD327 years ago

I should point out that I would like to either see this ticket die or be implemented.

Same here, That was the only reason i put forward a simple function, If the OO method wasnt going to make it, I'd at least have liked to see a simplistic function work its way in instead.

For the entire time I went with the object oriented approach I didn't receive a word of feedback.

I noticed that at the time, I just hoped that it meant that no-one had anything against it, And that it was a nice track to follow.

Maybe a list needs to be made for what *would* constitute a good API, and take it from there:

  • Single function call to retrieve a simple document
  • Automatic fallback to other methods in order of preference
  • GET/HEAD/POST fully supported and useable under each fallback
  • Able to be used by all core files without issue

the scattered fsockopen's are working pretty well on the majority of hosts at present, However, it lacks Proxy support (an extra 2 lines in each really..), different sections of code may use a different method (ie. updater uses fsockopen, ping uses remote_url_open & fopen() AFAIK) which leads to one section working, and another not.

Maybe some feedback from actual devs with reasons for/against the provided method, and/or what would need to be present in any approaches put forward

comment:29 @ryan7 years ago

I'm fine with either approach as long as it does its best to work with whatever transports are available. I want this to work out of the box for as many people as possible. The OOP patches seem to do that so that's a point in their favor as far as I'm concerned.

If we can pick an approach, we might be able to put this in 2.6 and use it in one or two lower-impact spots in the core code to try it out. We can migrate everything over in 2.7.

comment:30 @jacobsantos7 years ago

  • Keywords needs-patch added; has-patch removed

Sounds reassuring.

I believe it will be about two hours work to extend the test cases to cover the changes to the API. Also, use some of the code from DD32's patch in with the old code and refresh the old code. The cURL implementation also needs to be completed and test cases written for it also.

comment:31 @ryan7 years ago

  • Milestone changed from 2.9 to 2.7

comment:32 @santosj7 years ago

  • Owner changed from anonymous to jacobsantos

comment:33 @santosj7 years ago

Taking ownership of this, will implement recommendations.

comment:34 @santosj7 years ago

Basically, I refactored a lot of the code. It still needs modifications to the test cases and needs to be tested against the test cases to make sure there aren't any bugs with the code. I forgot where the file is, so I couldn't upload, but I think I'll do the extra steps, so that it can get in faster.

I'm also going to add cURL implementation to complete the library.

comment:35 @santosj7 years ago

  • Keywords has-patch tested added; needs-patch needs-testing removed

comment:36 @santosj7 years ago

Okay, it is done.

comment:37 @ryan7 years ago

Pretty. Let's transition a couple of places in core to use it and see how it handles. Maybe start with spawn_cron() and wp_update_plugins().

comment:38 @jacobsantos7 years ago

Okay. Sounds great, DD32 pointed out that the streams transport doesn't work. Need to test that on more systems before continuing or figure a way to allow for disabling transports through a plugin.

comment:39 @DD327 years ago

DD32 pointed out that the streams transport doesn't work

I think i can put that down to OS weirdness, It appears to be working pretty well now..

comment:40 @DD327 years ago

Just noticed a bug in the ExtHTTP class:

		switch($r['method'])
		{
			case 'GET':
				$r['method'] = HTTP_METH_GET;
				break;
			case 'POST':
				$r['method'] = HTTP_METH_POST;
				break;
			case 'HEAD':
				$r['method'] = HTTP_METH_HEAD;
			default:
				$r['method'] = HTTP_METH_GET;
		}

Head will drop through and get changed to a Get.

The default branch does not need to be last as such, its just nicer to have it there.. So either add a break in for the head statement, or chante it to:

			default:
			case 'GET':

@jacobsantos7 years ago

Update for recent changes to the API

@jacobsantos7 years ago

Fix ExtHTTP bug

comment:41 @jacobsantos7 years ago

Is it possible to remove the following files:

  • http.2.php
  • http.3.php
  • http.4.php
  • test_http_api.php
  • test_http_api.2.php
  • dragonu.http.php
  • http.6.php
  • http.7.php
  • http.8.php
  • http.9.php
  • http.10.php
  • http.10.1.php
  • http.10.2.php
  • test_http_api.4.php
  • test_http_api.5.php

Also, latest patch appears to be in working order. Will create patch to switch spawn_cron() and wp_update_plugins() over.

@jacobsantos7 years ago

Converts spawn_cron() and wp_version_check() based off of r8512

@jacobsantos7 years ago

Updated HTTP API with parameter switching for options before headers and body.

@jacobsantos7 years ago

Test cases for HTTP API

comment:42 @jacobsantos7 years ago

Okay, there are some flaws with the design, so the test cases will have to be extended to cover those flaws and hopefully correct them before it becomes a problem. Should be ready to go for live testing.

comment:43 @ryan7 years ago

(In [8516]) HTTP POST and REQUEST API from jacobsantos. see #4779

comment:44 @ryan7 years ago

We need a way to avoid doing a read for the cron request. We don't want to wait for a response and don't care what it has to say.

Some things need to use WP styling. Use a space after "if" and cuddle brackets.

comment:45 @ryan7 years ago

(In [8518]) Formatting and phpdoc cleanups from DD32. see #4779

comment:46 @DD327 years ago

4779.formatting2+non-blocking.diff

The changes in .2 after [8518]

Also includes some non-blocking codeblocks, which are untested as of right now.

From what i can tell, It appears the HTTP Extension doesnt have a documented non-blocking mode.

The PHPDoc on the classes mentioning Preferences also needs to be updated (Its in the wrong order)

comment:47 follow-up: @Viper007Bond7 years ago

I can currently not access my local development blog.

Warning: fopen(http://localhost/plugindev/wp-cron.php?check=26b6c8607f5d23860aeb874058881fae) [function.fopen]: failed to open stream: A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond. in D:\Webserver\htdocs\plugindev\wp-includes\http.php on line 535

Fatal error: Maximum execution time of 60 seconds exceeded in D:\Webserver\htdocs\plugindev\wp-includes\http.php on line 535

comment:48 in reply to: ↑ 47 @santosj7 years ago

Replying to Viper007Bond:

I can currently not access my local development blog.


That is the same error that DD32 came up with. It might be that a cURL needs to be added and used first. I'm unsure what the issue is with Streams transport that would keep causing that error with users. It would totally suck to keep having that error popup.

comment:49 @santosj7 years ago

Um, DD32 did mention that it might have something to do with the use of "localhost" and having issue to resolve the DNS for localhost. Hmm, it might be possible to check for whether localhost is being used as the host and not use Streams in that instance.

comment:50 @santosj7 years ago

Will work some more on this later today.

comment:51 @ryan7 years ago

One thing we do on wordpress.com is use SERVER_ADDR when doing the request in spawn_cron(). Might be worth a try here.

comment:52 @ryan7 years ago

(In [8519]) Formatting changes and non-blocking mode. Props DD32. see #4779

comment:53 @santosj7 years ago

I'm already adding non-blocking implementation. As well as other parts.

comment:54 @santosj7 years ago

Non-blocking mode just means that fread will immediately read the buffer instead of waiting for content in blocking mode.

comment:55 @ryan7 years ago

I think we need to distinguish between doing a non-blocking read and not reading at all. spawn_cron shouldn't even try to read.

comment:56 @santosj7 years ago

Yes, I understand that now. I'm adding cURL support in my next patch.

@santosj7 years ago

Adds cURL transport, but does not add it to the list, because it needs to be tested first. Fixes documentation and non-blocking mode.

comment:57 @ryan7 years ago

(In [8520]) Initial cURL support. Doc and non-blocking mode fixes. Props santosj. see #4779

comment:58 @santosj7 years ago

Another patch is coming.

comment:59 @santosj7 years ago

There are several bugs within HTTP extension transport and Curl that need to be fixed. I'm also testing cURL so that it can be enabled. It won't be used for body, so okay, that makes two that can't be used to send content, but it can send headers.

@santosj7 years ago

Various fixes.

comment:60 @santosj7 years ago

Still need to drill down where conflicts are.

comment:61 @ryan7 years ago

(In [8522]) Various http fixes from santosj. see #4779

@santosj7 years ago

Fixes process headers for fopen header return, fixes HTTP extension response array.

comment:62 follow-up: @santosj7 years ago

Yeah, the return headers and response won't work until the new patch is applied.

comment:63 in reply to: ↑ 62 @santosj7 years ago

Replying to santosj:

Yeah, the return headers and response won't work until the new patch is applied.

For Fopen and Streams transports

comment:64 @ryan7 years ago

(In [8523]) Fixes process headers for fopen header return, fixes HTTP extension response array. Props santosj. see #4779

@santosj7 years ago

Fixes process headers for fopen header return, fixes HTTP extension response array. Also adds Curl implementation and allows for it to be used when using headers.

@santosj7 years ago

Last set of fixes that I can find and from testing. Need more live testing and test cases. Based off of r8523

comment:65 @ryan7 years ago

(In [8524]) Fixes process headers for fopen header return, fixes HTTP extension response array. Also adds Curl implementation and allows for it to be used when using headers. Props santosj. see #4779

comment:66 @jacobsantos7 years ago

Hmm, need to write more test cases.

comment:67 @jacobsantos7 years ago

Real good thing it is only used in two places.

comment:68 @Viper007Bond7 years ago

How do I go about telling Apache that localhost == 127.0.0.1 ?

comment:69 @DD327 years ago

How do I go about telling Apache that localhost == 127.0.0.1 ?

I'm not sure, I think it may be a PHP thing, Specifically, I think when it see's localhost its not doing HTTP... or something weird.

In the request classes it might be worth either:

  • If hostname == localhost, Connect to 127.0.0.1, send Host: header as localhost
  • Do an IP lookup, eg, request is for google.com - it then looks up the IP manually(using the PHP function) and connects to that ip, sending the host header as expected.

number 1 is basically just number 2, but only applied to localhost..

comment:70 @ryan7 years ago

Have you tried $_SERVERSERVER_ADDR? yet? In spawn_cron(), use it instead of siteurl to create the cron_url. I know that's helped some people with cron issues.

comment:71 @jacobsantos7 years ago

I have, fixes the can't connect error.

comment:72 @jacobsantos7 years ago

I actually used wp_guess_url()

@jacobsantos7 years ago

CRON uses wp_guess_url() to hopefully fix cron problem.

comment:73 follow-up: @ryan7 years ago

That won't help if WP_SITEURL is defined. How about directly using HTTP_HOST? We might want to fall back to SERVER_ADDR if that is not defined.

comment:74 follow-up: @ryan7 years ago

See #7456. How's that patch look?

comment:75 in reply to: ↑ 74 @jacobsantos7 years ago

Replying to ryan:

See #7456. How's that patch look?


Looks good. I've been meaning to add better error handling to the other classes. I'm not sure, but I'm thinking the stored static should store the classes that are tested to work and then loop through attempting to connect. Breaking the loop when an array is passed.

It would be slower to a degree, depending on if the first class succeeds. It might also cause a sudden bug where the cron is run at most three times. It should always work, so I'm thinking it would be a good idea to ensure that the connection is always established and the caller gets the available information.

There will be overhead doing it that way.

comment:76 @DD327 years ago

It would be nice if i could pass a array as the body and have the base request class automatically URL encode and all that.

see patch.

@DD327 years ago

comment:77 in reply to: ↑ 73 @jacobsantos7 years ago

Replying to ryan:

That won't help if WP_SITEURL is defined. How about directly using HTTP_HOST? We might want to fall back to SERVER_ADDR if that is not defined.

I actually like that it works that way. If I'm testing WordPress on my local, then I want to set the constant so that I don't have to manually change the value in the database.

Actually, it probably would be less overhead if you duplicate the code in the function. However if there is an exploit or bug in the function, then one or the other is going to be fixed. Or that is the fear, I might be wrong.

comment:78 @DD327 years ago

attachment 4779.encode.post.data.2.diff added.

In adition to the previous change in that, Passing an array of optional headers wasnt working (Passing a string of headers was fine), Due to the closeness of the changes in the file, i've added it in the same patch.

comment:79 @DD327 years ago

Have you tried $_SERVERSERVER_ADDR? yet? In spawn_cron(), use it instead of siteurl to create the cron_url. I know that's helped some people with cron issues.

I doubt that will help with the localhost issue, SERVER_ADDR is still going to contain localhost, It'll still fail connecting in any case.

comment:80 @ryan7 years ago

(In [8528]) Timeout in WP_Http_Streams::request causes Fatal error abort. Props wet. fixes #7465 see #4779

comment:81 @ryan7 years ago

(In [8529]) Use wp_guess_url() for determining the cron URL. Props jacobsantos. see #4779

comment:82 follow-up: @jacobsantos7 years ago

DD32: Would a simple str_replace('localhost', '127.0.0.1', $host); work?

@jacobsantos7 years ago

Cycle through available transports trying each one. Based off of r8529

comment:83 in reply to: ↑ 82 @Viper007Bond7 years ago

Replying to jacobsantos:

DD32: Would a simple str_replace('localhost', '127.0.0.1', $host); work?

Are there any credentials to worry about? Or does the file just need to be run?

If it just needs to be run, I would think that'd work just fine, although it'd be worth running that through a filter I'd imagine.

comment:84 @DD327 years ago

DD32: Would a simple str_replace('localhost', '127.0.0.1', $host); work?

Maybe str_replace('http://localhost/', 'http://127.0.0.1/', $url); would work, But that has the side effect that the HOST header will probably be set wrong..

With FSockOpen its a simple fix, Just replace the hostname to connect to with localhost, But with FOpen/ExtHTTP/Streams i dont think you can set the hostname and the IP to connect to seperately. I have a feeling just substituting 127.0.0.1 for localhost would probably work in 95% of cases, the other 5% there probably isnt a particuallly clean method.

Any comments on: attachment 4779.encode.post.data.2.diff added. ? The header thing is probably not the best on second thoughts, maybe something like the following, but the Encoding of the data should be fine (Which is exactly whats in attachment 4779.encode.post.data.diff added.}

if ( is_null($headers) )
$headers = array();

if ( ! is_array($headers) ) {
$headers = <process headers>();
}

comment:85 @DD327 years ago

See also: Ticket #7458 (curl_setopt() throws errors in safe mode)

Also:
The CURL non-blocking code doesnt look like it'll work, AFAIK, the request is only made when curl_exec() is called?

comment:86 @tellyworth7 years ago

Quick observation: discover_pingback_server_uri() does a HTTP GET request that fetches only the first 2048 bytes of content. http.php appears (at a quick glance) not to provide a way to do that.

discover_pingback_server_uri() is buggy and ugly and it'd be nice to replace it. Perhaps a new 'max-bytes' arg for WP_Http::request()?

comment:87 follow-up: @DD327 years ago

discover_pingback_server_uri()

that really is a ugly function..

a HEAD request can be used to look for X-Pingback headers & workout the content-type, however, you're right, theres no way to mearly return the first x bytes of the document.. And on top of that, most of the HTTP methods are designed to fetch the entire file (Streams, Http Extension, curl)

comment:88 @DD327 years ago

(In [8529]) Use wp_guess_url() for determining the cron URL. Props jacobsantos. see #4779

wp_guess_url() doesnt work for installs where WordPress is in a subdir, yet WP is srved from the base... err.. ie. http://dd32.id.au/ is my front page, WP is installed in http://dd32.id.au/wordpress/

Eitherway, get_option('site_url') is the smarter choice, as the URL to the install is known, Except in cases where WP is installed behing a Proxy server.. hmm

comment:89 in reply to: ↑ 87 @jacobsantos7 years ago

Replying to DD32:

discover_pingback_server_uri()

that really is a ugly function..

a HEAD request can be used to look for X-Pingback headers & workout the content-type, however, you're right, theres no way to mearly return the first x bytes of the document.. And on top of that, most of the HTTP methods are designed to fetch the entire file (Streams, Http Extension, curl)


Well, I wanted all of the classes to work to the same for consistency. It is impossible to know which class you are going to be using. It is still possible to support something like this by cutting the string to that size, but it is probably better to look at what the function is trying to do and correct it.

comment:90 @DD327 years ago

Well, I wanted all of the classes to work to the same for consistency

Yeah, "And on top of that, most of the HTTP methods are designed to fetch the entire file" wasnt exactly directed at you, Rather at the person who wrote the file url wrapper & http extension, AFAIK, they're designed to return the full data stream rather than just a portion of it.

Allthough, That could be worked around with a fread() & family instead of stream_get_whatever()?

comment:91 @jacobsantos7 years ago

Okay, well. The HTTP extension does use streams in some areas, but I think it is for deflate and compressing the data when it goes over. This is pretty sweet for adding 1.1 support, eventually.

comment:92 @ryan7 years ago

(In [8533]) Cycle through available transports trying each one. Props jacobsantos. see #4779

comment:93 follow-ups: @ryan7 years ago

4779.encode.post.data.2.diff is good to go?

comment:94 in reply to: ↑ 93 @santosj7 years ago

Replying to ryan:

4779.encode.post.data.2.diff is good to go?

Yes.

I still need to test my patch to make sure that the fallback cycle breaks when the response is returned.

comment:95 @ryan7 years ago

(In [8534]) Don't call curl_setopt() if safe_mode is enabled. Props ionfish and Po0ky. fixes #7458 see #4779

comment:96 in reply to: ↑ 93 @santosj7 years ago

Replying to ryan:

4779.encode.post.data.2.diff is good to go?

Actually, sorry I didn't see that you already committed my patch. I combined DD32 code with my patch.

comment:97 @santosj7 years ago

I think that if the response is not an error you can just return it within the foreach. This will also return the WP_Error, if complete failure.

@santosj7 years ago

Return response in foreach loop and remove debugging line. Based off of r8534

comment:98 @santosj7 years ago

4779.r8534.diff

Just returns the response if it isn't an error should make sure that the fall back isn't iterating through all of the available classes. It should also return the wp_error class on complete failure.

I forgot about the debug line I put in. The patch removes it.

comment:99 @santosj7 years ago

The API implements suggest made in #5065.

comment:100 @ryan7 years ago

(In [8535]) Return response in foreach loop and remove debugging line. Props santosj. see #4779

comment:101 @ryan7 years ago

(In [8536]) Tests for whether is an array or an object before running http_build_query() on . Props santosj. fixes #7460 see #4779

comment:102 @ryan7 years ago

(In [8537]) Don't use wp_guess_url(). Caused big problems. see #4779

comment:103 @ryan7 years ago

wp_guess_url() had big problems on subdir systems. Resulted in requests like this:

wp-cron.php?check=04758d1395fa563dede4bb7d9a47dd6ewp-cron.php?check=04758d1395fa563dede4bb7d9a47dd6ewp-cron.php?check=04758d1395fa563dede4

That just grew and grew.

comment:104 @DD327 years ago

The logic is reversed for GET/POST $body encoding.

$body is used for POST requests.

Patch also includes correction for custom headers being passed in as an array ( Currently: Pass array of headers in, it ignores them, pass a string of headers in, it accepts them).

comment:105 @jacobsantos7 years ago

Well, yeah that patch does make a lot of sense. Thanks man!

comment:106 @ryan7 years ago

(In [8544]) Fix reversed encoding. Props DD32. see #4779

comment:107 @ryan7 years ago

(In [8545]) Suppress fopen errors if WP_DEBUG is not enabled. Props jacobsantos. fixes #7456 see #4779

@santosj7 years ago

Combines fix for #7456 and fixes HTTP extension chunk format. Converts chunk to unchunked body.

comment:108 @ryan7 years ago

(In [8548]) Streams transport fixes from jacobsantos. fixes #7456 see #4779

@santosj7 years ago

Fixes chunk format based off of r8548

comment:109 @ryan7 years ago

(In [8549]) Fix chunked decoding. Props santosj. see #4779

@santosj7 years ago

Fix notice about body not having chunk formatting.

comment:110 @santosj7 years ago

I suspect there is still some more testing to do, but latest patch prevents decoding the body if there is no body to decode. Might need to suppress error, because I can't find anything anywhere that allows for testing whether or not the body is chunk-encoded and the regex for that would be pretty difficult without studying the RFC for many hours.

comment:111 @ryan7 years ago

(In [8560]) Fix notice about body not having chunk formatting. Props santosj. see #4779

@santosj7 years ago

Checks the headers for the transfer-encoding for chunked and then runs the function. based off of r8572

comment:112 @santosj7 years ago

Okay, I tested the new patch and it corrects the issue where it was throwing notices that it was trying to decode the chunks when no chunks were found. I just remembered that the return headers actually tell you when it has encoded something as chunked, which explains why there is no function to tell you when the body is chunk encoded.

Go figure. I should read the RFC again.

comment:113 @ryan7 years ago

(In [8578]) Check the headers for the transfer-encoding for chunked. Props santosj. see #4779

@santosj7 years ago

Fix Copy and Paste bug in cURL transport based off of r8582

comment:114 @santosj7 years ago

Latest patch needs to be committed for response to work correctly for cURL transport.

comment:115 @ryan7 years ago

(In [8583]) Fix Copy and Paste bug in cURL transport based. Props santosj. see #4779

comment:116 @ryan7 years ago

(In [8584]) Fixes cURL for Cron in non-blocking mode. Props santosj. fixes #7481 see #4779

comment:117 @santosj7 years ago

I'm going to write test cases which send requests against the WordPress version checker, the plugin version checker, and one that uses query string. I just found a lot of errors in the Fsockopen transport.

@santosj7 years ago

Fixes major issues with fsockopen transport, based off r8585

comment:118 @santosj7 years ago

  • Keywords needs-patch needs-unit-tests added; has-patch tested dev-feedback removed

I suppose the issue now is the level of stability:

  1. HTTP
  2. cURL
  3. Streams
  4. Fopen
  5. Fsockopen

The last three needs to be the primary focus and even more so for the fsockopen, since I believe that is the one that will be most used out of all of them. The fallback also has to be looked at. I found that it isn't cycling through enough, it stopped at fopen when it should have went down and tried fsockopen.

comment:119 @ryan7 years ago

(In [8586]) Fix issues with fsockopen transport. Props santosj. see #4779

comment:120 @santosj7 years ago

Unit tests also have to drill down the many, many code branches to ensure edge cases. Other improvements include standardizing the user-agent. Some work has already gone into that, but its not complete. I suppose test coverage will need close to 60 to 100 tests to provide full coverage.

Well, another 15 to 25 should cover most of the trouble spots. I'll work on these first.

@DD327 years ago

Fixes POST requests to add the Content-length and Content-type headers with FSockOpen

comment:121 follow-up: @DD327 years ago

attachment 4779.fsockopen.post.content-headers.diff added.
Fixes POST requests to add the Content-length and Content-type headers with FSockOpen

As the comment suggests, The FSockOpen class currently fails to POST data due to the content-type and content-length headers not being set.

comment:122 in reply to: ↑ 121 @jacobsantos7 years ago

Replying to DD32:

attachment 4779.fsockopen.post.content-headers.diff added.
Fixes POST requests to add the Content-length and Content-type headers with FSockOpen

As the comment suggests, The FSockOpen class currently fails to POST data due to the content-type and content-length headers not being set.

what if you're not sending POST data?

comment:123 @DD327 years ago

what if you're not sending POST data?

Then those headers do not need to be set, According to the HTTP 1.0 RFC they only need to be set if some actual post data is being sent, 7.2.1/7.2.2 of http://www.w3.org/Protocols/rfc1945/rfc1945

comment:124 @westi7 years ago

(In [8588]) Ensure fsockopen HTTP requests have Content-length and Content-type headers. See #4779 props DD32.

comment:125 @santosj7 years ago

I think it is possible to support chunk transfer-encoding in the other transports. It seems some servers are sending HTTP/1.1 responses, when the requests were sent using HTTP/1.0. It means that when the response comes back it is most often chunk transfer-encoded, which screws up major portions of the site.

I'm going to build a custom test suite for testing the HTTP API, because using the official Automattic WordPress Tests isn't exactly working for me. I'm going to extend the test suite to cover the chunk decoding (the algorithm doesn't seem difficult). It should also allow for users to test the transports on their system (much like the plugin upgrader debug plugin). Doing it this way should mean that it won't concern the developer with having to test the response for chunk encoding.

The ExtHTTP transport supports it, therefore all of the others need to support it also.

comment:126 @jacobsantos7 years ago

See #2875, problem in wp_remote_head() function that should be fixed using the HTTP API. It doesn't use the HTTP API yet, but eventually.

comment:127 @santosj7 years ago

Needs to add error checking to the update and cron, and plugin installer, to detect offline. Well, not offline per say, but just checking for the return WP_Error.

Need to do more research on timeouts to ensure people aren't experiencing PHP time limit timeouts.

comment:128 @santosj7 years ago

Patch, I'm working on will add support for chunked transfer-encoding, so 1.1 should be supported within WordPress, instead of just 1.0. Some servers I've seen want to send 1.1 headers back when 1.0 headers are sent.

Patch, will also move $body and $headers parameters into the $args parameter and will update the various areas where the HTTP API is used currently.

Patch will also fix WP_Error people are having with not being connected to the Internet.

I'm also building a HTTP API test suite that will hopefully find and fix a lot of the issues.

The Patch will be pretty massive, as it is already. I'll upload the HTTP API test suite when I'm done.

@santosj7 years ago

Patch merges headers and body into $args, also adds support for chunked transfer-encoding decoding, based off of r8620

comment:129 @santosj7 years ago

Patch is still a work in progress. Any ideas, suggestions, tips?

comment:130 @santosj7 years ago

HTTP API test suite goes in the base WordPress directory to work.

comment:131 @ryan7 years ago

Looks good to me. Want to switch to the new calling style and unleash?

comment:132 @santosj7 years ago

I'm about to upload a new patch that fixes a great deal more problems. I do like the new calling style, I shouldn't doubt DD32. He has consistency been right more often than he is wrong, if he has been wrong.

comment:133 @santosj7 years ago

HTTP tests suite 8/12/08 completes most of the tests, just need to add tests for the Plugin Installer and that shouldn't take much longer.

Going to upload new patch, since I'm happy that it fixes majority of known issues.

@santosj7 years ago

More fixes, implements chunked transfer decoding, other enhancements, based off of r8624

comment:134 @santosj7 years ago

Patch includes many fixes from tests and from another ticket. It converts the plugin version checker over to use wp_remote_request() also. There aren't any tests for the plugin installer requests, but you can test it by going to the pages and seeing if the plugins are shown.

comment:135 @santosj7 years ago

Of course, there will be more bugs, but I think the new bugs should be made in new tickets.

@santosj7 years ago

More fixes, implements chunked transfer decoding, other enhancements. Add more sanity checking. Based off of r8629

comment:136 @westi7 years ago

(In [8630]) HTTP API improvements. Implements chunked transfer decoding. Moves plugin update checker over to api. see #4779 props santosj.

comment:137 @santosj7 years ago

Patch fixes chunked bugs. Adds documentation.

@santosj7 years ago

Add @since to chunked decoding, return actual body when chunked. Based off of r8630

@santosj7 years ago

Complete HTTP API test suite

comment:138 @westi7 years ago

(In [8634]) Fix bugs in chunked encoding handling. See #4779 props santosj.

comment:139 @santosj7 years ago

  • Keywords needs-patch needs-unit-tests removed
  • Resolution set to fixed
  • Status changed from new to closed

I'm going to give it another look over to make sure everything is working okay, but it appears to be working to me. Any other issues can be made in new tickets.

Thanks for committing this!

Note: See TracTickets for help on using tickets.