Consolidate WP_Http argument processing into a single class

[jacobsantos]

Consolidates the argument processing for all of the transports and main WP_Http class.

Since the beginning the argument processing was handled in both WP_Http::request() and in the individual transports. As more arguments and features were added, it became that WP_Http::request() was getting all of the support and the rest of the transports were becoming stagnate with argument processing.

The individual transports appear to be used more individually, which would cause problems with argument processing or require duplication of WP_Http::request() code.

[jacobsantos]

To say this needs testing is to maybe suggest that there has been any testing. To clarify, there hasn't. I'm just putting this up here, so I can get feedback of if the current code is acceptable to continue for improvement or if it might end up in WordPress.

[jacobsantos]

Sorry, there are two more enhancements besides inline documentation improvements. Those can probably be separate tickets since they'd be more likely to get into core since they have minimal potential damage.

They include:

• Splitting Fsockopen response into a method for those who wish to change how the response is retrieved with the request.
• Change all headers to lowercase for ease of checking header keys.

[jacobsantos]

Initial request for comment on argument processing class.

[dd32]

I don't see any need for this.

The WP_HTTP::request() method takes care of the incoming request and can handle processing the args. The processing of the args in each transport is specific to it's needs, to abstract the minimal code duplication will introduce more code and increase the churn rate of the existing code.

[hakre]

Not to forget that unrolled code always has the better performance. ;)

[jacobsantos]

Replying to dd32:

I don't see any need for this.

The WP_HTTP::request() method takes care of the incoming request and can handle processing the args. The processing of the args in each transport is specific to it's needs, to abstract the minimal code duplication will introduce more code and increase the churn rate of the existing code.

Come again? Perhaps you should review the existing code and this patch again. If I was unclear in the summary, then please ask where you need further information.

Perhaps you should look at the transports again and see how they in fact do not duplicate the code in WP_HTTP::request() since it would lengthen the methods unnecessary. It has been a point of contention that you really can't call individual transports, because the majority of the processing in WP_HTTP::request() is not found in any of the other transports.

I mean, I'm not going to update the patch, so if you wish to close it for that reason, then go ahead. However, it is advised in case someone else wishes to support this to give proper explanation. Also performance will be negligible or at least normal overhead given using objects.

TL;DR Reopening ticket because reason for closing is not supported by fact.

[dd32]

Replying to jacobsantos:

Come again? Perhaps you should review the existing code and this patch again. If I was unclear in the summary, then please ask where you need further information.

Then perhaps you should point out where I'm wrong in my comment.

I've read the ticket, I've read the patch, and other than consolidating the Arguement processing int oa separate class, I cannot see any benefits.

After reading your comment, The only other option I could understand is that you're suggesting moving all the processing to a separate class so that you can call WP_HTTP directly OR call a subclass directly?

[jacobsantos]

Replying to dd32:

After reading your comment, The only other option I could understand is that you're suggesting moving all the processing to a separate class so that you can call WP_HTTP directly OR call a subclass directly?

Yes. Right now, if you want to use a Transport and want to send post data, something the WP_HTTP does automatically to set the headers and format the post data, if array; you'd have to duplicate that code before you use the Transport. Goes for other features of WP_HTTP::request() handling as well. Since the WP_HTTP::request() received / receives the majority of support and feature additions, it is better to consolidate all of the code to a single place (a later patch that I never uploaded used a function instead) and call it for each of the transports.

In this way, the WP_HTTP::request() only handles calling the transports and passing the processed information. It would shorten the WP_HTTP::request() to a single action (there is a term used for OOP that I can't remember at the moment, meaning limiting a method and, or class to a single task or service). Right now the WP_HTTP::request() is doing two things, processing the args and running through the working transports.

This ticket is to move the processing of the args out of WP_HTTP::request() to allow for the Transports to use the same code without duplication.

[dd32]

Then my opinion remains the same, The Core HTTP API should require users call the handler (WP_HTTP) rather than subclasses directly, they should all have the same support on every system and if they don't, then the WP_HTTP handler should not allocate a request to one.

I realise this may differ from your original intentions of the HTTP API, but my opinion is being based on what I feel is best for WordPress developers AND users as a whole; that is, that everything should "just work" and a plugin relying upon a certain transport does not fit that idea to me.

[nacin]

Replying to dd32:

Then my opinion remains the same, The Core HTTP API should require users call the handler (WP_HTTP) rather than subclasses directly, they should all have the same support on every system and if they don't, then the WP_HTTP handler should not allocate a request to one.

I realise this may differ from your original intentions of the HTTP API, but my opinion is being based on what I feel is best for WordPress developers AND users as a whole; that is, that everything should "just work" and a plugin relying upon a certain transport does not fit that idea to me.

Strongly agree with this. The current architecture makes sense in the context of what should be the direction and goals of the HTTP API.

[jacobsantos]

Replying to dd32:

Then my opinion remains the same, The Core HTTP API should require users call the handler (WP_HTTP) rather than subclasses directly, they should all have the same support on every system and if they don't, then the WP_HTTP handler should not allocate a request to one.

I realise this may differ from your original intentions of the HTTP API, but my opinion is being based on what I feel is best for WordPress developers AND users as a whole; that is, that everything should "just work" and a plugin relying upon a certain transport does not fit that idea to me.

Okay, I can accept this reason. Whether or not I agree, at least it explains the direction for someone wondering why it is this way.

Thanks for the explanation.

[hakre]

Example only

[hakre]

13777.2.patch​ is an example only that as well contains some code making test and request of concrete http transports protected as what was said intended above.