Proposal for HTTP POST and REQUEST API

[darkdragon]

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.

[santosj]

This should not have 2.4 milestone.

[Nazgul]

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.

[santosj]

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

[santosj]

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.

[darkdragon]

Prototype of HTTP Request API

[darkdragon]

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.

[darkdragon]

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

[darkdragon]

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

[darkdragon]

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

[DD32]

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?

[jacobsantos]

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.

[DD32]

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? :)

[DD32]

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)

[jacobsantos]

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.

[DD32]

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.

[jacobsantos]

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.

[DD32]

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)

[jacobsantos]

Pretty sweet.

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

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

and not with curly brackets.

[DD32]

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

[mdawaffe]

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.

[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.

[jacobsantos]

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.

[DD32]

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.

[jacobsantos]

Need to get on IRC.

[ryan]

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.

[DD32]

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

[DD32]

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.

[jacobsantos]

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!

[DD32]

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

[ryan]

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.

[jacobsantos]

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.

[santosj]

Taking ownership of this, will implement recommendations.

[santosj]

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.

[santosj]

Okay, it is done.

[ryan]

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().

[jacobsantos]

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.

[DD32]

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..

[DD32]

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':

[jacobsantos]

Update for recent changes to the API

[jacobsantos]

Fix ExtHTTP bug

[jacobsantos]

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.

[jacobsantos]

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

[jacobsantos]

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

[jacobsantos]

Test cases for HTTP API

[jacobsantos]

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.

[ryan]

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

[ryan]

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.

[ryan]

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

[DD32]

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)

[Viper007Bond]

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

[santosj]

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.

[santosj]

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.

[santosj]

Will work some more on this later today.

[ryan]

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

[ryan]

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

[santosj]

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

[santosj]

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

[ryan]

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.

[santosj]

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

[santosj]

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

[ryan]

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

[santosj]

Another patch is coming.

[santosj]

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.

[santosj]

Various fixes.

[santosj]

Still need to drill down where conflicts are.

[ryan]

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

[santosj]

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

[santosj]

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

[santosj]

Replying to santosj:

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

For Fopen and Streams transports

[ryan]

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

[santosj]

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.

[santosj]

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

[ryan]

(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

[jacobsantos]

Hmm, need to write more test cases.

[jacobsantos]

Real good thing it is only used in two places.

[Viper007Bond]

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

[DD32]

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..

[ryan]

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.

[jacobsantos]

I have, fixes the can't connect error.

[jacobsantos]

I actually used wp_guess_url()

[jacobsantos]

CRON uses wp_guess_url() to hopefully fix cron problem.

[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.

[ryan]

See #7456. How's that patch look?

[jacobsantos]

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.

[DD32]

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.

[jacobsantos]

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.

[DD32]

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.

[DD32]

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.

[ryan]

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

[ryan]

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

[jacobsantos]

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

[jacobsantos]

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

[Viper007Bond]

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.

[DD32]

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>();
}

[DD32]

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?

[tellyworth]

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()?

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

[DD32]

(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

[jacobsantos]

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.

[DD32]

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()?

[jacobsantos]

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.

[ryan]

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

[ryan]

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

[santosj]

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.

[ryan]

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

[santosj]

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.

[santosj]

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.

[santosj]

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

[santosj]

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.

[santosj]

The API implements suggest made in #5065.

[ryan]

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

[ryan]

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

[ryan]

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

[ryan]

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.

[DD32]

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

[jacobsantos]

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

[ryan]

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

[ryan]

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

[santosj]

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

[ryan]

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

[santosj]

Fixes chunk format based off of r8548

[ryan]

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

[santosj]

Fix notice about body not having chunk formatting.

[santosj]

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.

[ryan]

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

[santosj]

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

[santosj]

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.

[ryan]

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

[santosj]

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

[santosj]

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

[ryan]

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

[ryan]

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

[santosj]

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.

[santosj]

Fixes major issues with fsockopen transport, based off r8585

[santosj]

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.

[ryan]

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

[santosj]

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.

[DD32]

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

[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.

[jacobsantos]

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?

[DD32]

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

[westi]

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

[santosj]

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.

[jacobsantos]

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.

[santosj]

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.

[santosj]

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.

[santosj]

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

[santosj]

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

[santosj]

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

[ryan]

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

[santosj]

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.

[santosj]

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.

[santosj]

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

[santosj]

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.

[santosj]

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

[santosj]

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

[westi]

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

[santosj]

Patch fixes chunked bugs. Adds documentation.

[santosj]

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

[santosj]

Complete HTTP API test suite

[westi]

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

[santosj]

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!