WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#13777 closed enhancement (wontfix)

Consolidate WP_Http argument processing into a single class

Reported by: jacobsantos Owned by: jacobsantos
Milestone: Priority: low
Severity: minor Version: 3.0
Component: HTTP API Keywords: has-patch 2nd-opinion close
Focuses: Cc:

Description

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.

Attachments (2)

13777.patch (23.5 KB) - added by jacobsantos 5 years ago.
Initial request for comment on argument processing class.
13777.2.patch (19.7 KB) - added by hakre 4 years ago.
Example only

Download all attachments as: .zip

Change History (15)

comment:1 @jacobsantos5 years ago

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.

comment:2 @jacobsantos5 years ago

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.

@jacobsantos5 years ago

Initial request for comment on argument processing class.

comment:3 @scribu5 years ago

  • Keywords dev-feedback added; needs-testing removed

comment:4 @jacobsantos5 years ago

  • Milestone changed from Unassigned to Future Release

comment:5 follow-up: @dd324 years ago

  • Keywords dev-feedback removed
  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from new to closed

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.

comment:6 @hakre4 years ago

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

comment:7 in reply to: ↑ 5 ; follow-up: @jacobsantos4 years ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

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.

comment:8 in reply to: ↑ 7 ; follow-up: @dd324 years ago

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?

comment:9 in reply to: ↑ 8 @jacobsantos4 years ago

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.

comment:10 follow-ups: @dd324 years ago

  • Keywords 2nd-opinion close added

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.

comment:11 in reply to: ↑ 10 @nacin4 years ago

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.

comment:12 in reply to: ↑ 10 @jacobsantos4 years ago

  • Resolution set to wontfix
  • Status changed from reopened to closed

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.

Version 0, edited 4 years ago by jacobsantos (next)

@hakre4 years ago

Example only

comment:13 @hakre4 years ago

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.

Note: See TracTickets for help on using tickets.