WP_Http Transport test order refactoring

[hakre]

Just a typo of the class name.

[hakre]

Additionally the function can benefit by a little refactoring of the code. I'll add a second patch.

[hakre]

This would need the same for postTransport, that is basically running the same code (?!). Let me know if that duplicate code can be safely replaced with single, original code.

[hakre]

Reducing duplicate code, optionally iterate over contexts.

[hakre]

Small correction, isset checks for null already.

[hakre]

Correction, Operator Precedence related.

[hakre]

fopen not supported in post (sending a body with the request) context

[hakre]

Whitespaces

[hakre]

fopen WP_Http might be considered obsolete since PHP 4.3 requirements. Should be the same as streams WP_Http.

[hakre]

Allow Traversable of many kind, next to array

[jacobsantos]

I like this patch better than my own. There is a related ticket I'll post when I find it.

[jacobsantos]

Related: #8622

[jacobsantos]

Replying to hakre:

fopen WP_Http might be considered obsolete since PHP 4.3 requirements. Should be the same as streams WP_Http.

$context was added in PHP5, which streams relies on. While streaming was added in PHP4.3, it didn't support HTTP that is available in PHP5.

[dd32]

is_scalar( $named ) && ( $named = array( (string) $named ) ); 
foreach ( $named as $context ) { 

Can be simplified to:

foreach ( (array)$named as $context ) { 

However, $context is undefined outside of the loop in the return statements of the code.

I think originally, The priority orders for Get/Post were different, which is why there are 2 methods?

I'd rather see WP_HTTP_Fopen::test() return false for POST requests to limit the locations which detects which transport is available

Related Tickets: #8622 #11613

[dd32]

Actually, What's the thought behind $named/$context being an array in the first place?

[dd32]

However, $context is undefined outside of the loop in the return statements of the code.

Seems i was mistaken, I could've sworn the garbage collector cleans that up, It might just be certain versions of PHP which do it.. or maybe i'm completely off :)

[hakre]

Replying to jacobsantos:

$context was added in PHP5, which streams relies on.

ACK, my previous comment regarding fopen is wrong then, please ignore it. The WP_Http-stream class needs context, it's actually checked inside of it.

---

Replying to dd32:

...

is_scalar( $named ) && ( $named = array( (string) $named ) ); 
foreach ( $named as $context ) { 

Can be simplified to:

foreach ( (array)$named as $context ) { 

The casting to array will prevent future use of other iterators than array. That's why I didn't suggested that one. It might not look that important in this case, but generally this casting to array in foreach should be considered bad practice because it removes flexibilty from froeach. There are some articels which explain that with greater detail.

---

Replying to dd32:

I think originally, The priority orders for Get/Post were different, which is why there are 2 methods?

From what I read (but please double check) both have the same order. Anyway, as this function already uses an array for the priority order, it should be easy to extend it to something configure-able.

I'd rather see WP_HTTP_Fopen::test() return false for POST requests to limit the locations which detects which transport is available

I'll add an additional patch that is reflecting that.

[hakre]

Fopen supports get only. Test reflects that now.

[dd32]

From what I read (but please double check) both have the same order.

It was meant as a background as to why there were 2 functions, The ordering has been played with over the lifetime of WP_HTTP as bugs poped up here and there.

[hakre]

Replying to dd32:

Actually, What's the thought behind $named/$context being an array in the first place?

It's actually in the second place, it was the $context string in the first place. The array is optional to allow passing more than one context at once, as this is a usage case in the constructor.

[hakre]

Replying to dd32:

From what I read (but please double check) both have the same order.

It was meant as a background as to why there were 2 functions, The ordering has been played with over the lifetime of WP_HTTP as bugs poped up here and there.

So what do you would say? Is it a need to have multiple transport orders per context for the future? If so, we can add a filter onto the array probably and/or make it a parameter.

Adding a filter would help to easily let plugins control the available transports and even add own ones. Hey I like that idea. Currently it's not possible and control is a bit akward (​forcing curl plugin example).

[hakre]

Strictly there is no need of an iterator inside the function.

[hakre]

Transports defined in WP_Http::$transports; Filterable per context / args; WP_Http_ExtHTTP class name case changed to make data more simple (same casing for all); Changed case of other WP_HtTp* class names as well.

[hakre]

I now incorporated the feedback partially.

• The iterator over multiple contexts inside the function as commented on by dd32 is strictly taken not necessary. I've removed it to make the function more clear.
• WP_Http_Fopen::test() now returns false on any request methods other than the default 'GET' one. This is based on dd32's feedback as well.
• The test order is now defined in the WP_Http class and can be filtered per each context.
• If multiple transports should be allowed/returned as suggested in #8622 patch ​8622.3.diff, then the break; needs to be removed.
• The design limitations as discussed in #11613 are not dealt with so far. But The changes so far should make it more easily to add such a functionality.

[jacobsantos]

if ( !call_user_func( array( $class, 'test' ), $args ) ) 
 	 continue;

Should be:

if( ! $class::test($args) )
	continue;

for performance. The syntax should work fine, but I haven't tested it.

Also, the Method check in Fopen is wrong. The problem is not the method but sending with a body as part of the message. It is possible to send a body with GET requests.

[jacobsantos]

Replying to hakre:

• The design limitations as discussed in #11613 are not dealt with so far. But The changes so far should make it more easily to add such a functionality.

I believe the solution in that ticket, should fix the problems. I think too much is being relied on with the test() method, which should be revisited to not be as used as much as it is now. The test() method was only ever supposed to be used as to whether the transport was supported by the PHP version and whether the extension was installed.

Using it as a means to whether the transport supports SSL is wrong. They should all support SSL provided openSSL extension is installed and barring any PHP specific bugs. Also, the fear with some of the checks, for example nonblocking, is that so few of the transport supports nonblocking that you limit which transport will be used.

Also, given that also some transports like cURL aren't written to support nonblocking correctly it adds additional complexity to adding it.

[hakre]

Replying to jacobsantos:

if ( !call_user_func( array( $class, 'test' ), $args ) ) 
 	 continue;

Should be:

if( ! $class::test($args) )
	continue;

for performance. The syntax should work fine, but I haven't tested it.

AFAIK the later is PHP 5.3. So for static calls on a variable class name I choosed the first method: As of PHP 5.3.0, it's possible to reference the class using a variable. (​From the PHP Manual)

Also, the Method check in Fopen is wrong. The problem is not the method but sending with a body as part of the message. It is possible to send a body with GET requests.

Okay, that needs to be added in ..._fopen::test() then. I'm not that firm with the body concept. Anyway, on instatiating WP_Http, args will ever be an empty array. So this might be just the design flaw?

---

Replying to jacobsantos:

Replying to hakre:

• The design limitations as discussed in #11613 are not dealt with so far. But The changes so far should make it more easily to add such a functionality.

I believe the solution in that ticket, should fix the problems. I think too much is being relied on with the test() method, which should be revisited to not be as used as much as it is now. The test() method was only ever supposed to be used as to whether the transport was supported by the PHP version and whether the extension was installed.

So this should only be necessary to run once per request regardless of which args etc. . That would explain why for the &_post... function there was a different test order then in the &_get... variant. Just hardcoding.

I then suggest to only test() on a first initialisation done once per request (not per class instantiation), to leave WP_Http with an array of available transports that can be used later on.

Using it as a means to whether the transport supports SSL is wrong. They should all support SSL provided openSSL extension is installed and barring any PHP specific bugs.

I'm not that firm with SSL but as far as my PHP experience goes on hosts, it's like you describe it. It's there or it isn't, but the type of transport/streamwrapper does not make a difference then.

Also, the fear with some of the checks, for example nonblocking, is that so few of the transport supports nonblocking that you limit which transport will be used.

Are there even non-blocking usage examples in Wordpress?

Also, given that also some transports like cURL aren't written to support nonblocking correctly it adds additional complexity to adding it.

Again, where are these non-blocking requests used in a shared-nothing, single request application architecture? Is there some fetching multiple RSS feeds at once or so?

[hakre]

Replying to jacobsantos:

Also, the Method check in Fopen is wrong. The problem is not the method but sending with a body as part of the message. It is possible to send a body with GET requests.

Your statement made me curious and I re-read the HTTP 1.1 RFC according to that. I'm referring to this document (collection):

A message-body MUST NOT be included in a request if the specification of the request method (section 5.1.1) does not allow sending an entity-body in requests.

​rfc2616 section 4.3

And in section 5.1.1:

"GET" ; Section 9.3

​rfc2616 section 5.1.1

And in section 9.3:

The GET method means retrieve whatever information (in the form of an entity) is identified by the Request-URI ...

​rfc2616 section 9.3

I can not find any hint in 9.3 as the specification of the GET method that allows to send a request body. Following the description in section 4.3, this means that a message body MUST NOT be included in the GET request.

From what I can see here is that to include a request body in a get request is just violating the HTTP standards. Can you please check for yourself again? I might have misread something or you have a source you can point to which shows the opposite.

[jacobsantos]

Replying to hakre:

Replying to jacobsantos:

Also, the Method check in Fopen is wrong. The problem is not the method but sending with a body as part of the message. It is possible to send a body with GET requests.

Your statement made me curious and I re-read the HTTP 1.1 RFC according to that. I'm referring to this document (collection):
I can not find any hint in 9.3 as the specification of the GET method that allows to send a request body. Following the description in section 4.3, this means that a message body MUST NOT be included in the GET request.

From what I can see here is that to include a request body in a get request is just violating the HTTP standards. Can you please check for yourself again? I might have misread something or you have a source you can point to which shows the opposite.

Nothing from what I can read says that it is not allowed and using this: "A server SHOULD read and forward a message-body on any request; if the request method does not include defined semantics for an entity-body, then the message-body SHOULD be ignored when handling the request." from the same ​rfc2616 section 4.3 states that the server should accept it any way, not that it has to do anything with the message-body. The fact that it is possible and not disallowed, means that it can be done. I agree that it should not be done, but it is something I have done in the past with results. Given the liberal nature of the specification, it is allowed, because it might break the web otherwise.

The HEAD method is the only one that I can see that specifically states that the message-body must not exist. However, it is true that just because you CAN send a message-body with a GET request, doesn't mean the server has to actually process it with the request. In this, if you want to be sure that the server will handle the message-body, then it has to be either PUT or POST or another that specifically states a request for a message body.

It is semantics, the point is that since it is possible and the server will still accept it, not allowing it based on method seems too restrictive. The point is to be a bit more liberal as long as it doesn't break the requirements of the spec which specifically state that something must not do or must do something.

[jacobsantos]

Replying to hakre:

Replying to jacobsantos:

  if( ! $class::test($args) )
  	continue;

AFAIK the later is PHP 5.3. So for static calls on a variable class name I choosed the first method: As of PHP 5.3.0, it's possible to reference the class using a variable. (​From the PHP Manual)

This is unfortunate, but okay. I don't suppose this is going to change for the next few years, but I do wish it was possible to do it the alternative way.

Also, the Method check in Fopen is wrong. The problem is not the method but sending with a body as part of the message. It is possible to send a body with GET requests.

Okay, that needs to be added in ..._fopen::test() then. I'm not that firm with the body concept. Anyway, on instatiating WP_Http, args will ever be an empty array. So this might be just the design flaw?

I think that is the point, it shouldn't be part of the test() method. The design flaw is that the name of the method was poorly chosen. It was never meant to be a catch all for whether the method supports a given request.

The issue is that it is used for two purposes with only the first one really supported.

There should be two methods. One for the initial, "Is this transport supported on PHP?" and a second for, "Does this transport support this request." Right now, the same method is only supported for the first question and the first request. Every new request won't be tested.

For example, if the first request is for SSL, then every other request call will use transports for SSL whether or not the next request requires it. Therefore, there might be requests that would otherwise succeed, not requiring SSL, that would fail because the first request required it and was cached.

The same is true for the counter. If the first request is for one that does not require SSL and a new one does require it, then it might use a transport that does not support it.

The solution, from my perspective is to split the call in to the cached and the not cached. Checking for whether a transport supports any feature is a simple relative call and might only require a single transport list array.

---

Replying to jacobsantos:
So this should only be necessary to run once per request regardless of which args etc. . That would explain why for the &_post... function there was a different test order then in the &_get... variant. Just hardcoding.

Well, initially, the cURL transport did not support body requests, so the _post method didn't include it. After it was added, the list became more like the _get. However, backward compatibility prevent really a solution, plus, during the initial tweaking phase, it seems better to leave it as it is (if it isn't broke, don't fix it), but I think it is a lot better to change it now since the design flaw is causing a bit of headaches for moving forward.

I then suggest to only test() on a first initialisation done once per request (not per class instantiation), to leave WP_Http with an array of available transports that can be used later on.

This might be useful since really, there are only a few features, currently, that are tested and supported in the library. The issue is that as the HTTP library adds features that aren't supported well by other transports, then the lists might get a bit large.

Using it as a means to whether the transport supports SSL is wrong. They should all support SSL provided openSSL extension is installed and barring any PHP specific bugs.

I'm not that firm with SSL but as far as my PHP experience goes on hosts, it's like you describe it. It's there or it isn't, but the type of transport/streamwrapper does not make a difference then.

It was one example, nonblocking is another and the message-body is the final.

Also, the fear with some of the checks, for example nonblocking, is that so few of the transport supports nonblocking that you limit which transport will be used.

Are there even non-blocking usage examples in Wordpress?

The HTTP cronjob is the only known example in WordPress. Whether or not other people use it, meh, it is debatable. It is only useful for POST where the response isn't important.

Also, given that also some transports like cURL aren't written to support nonblocking correctly it adds additional complexity to adding it.

Again, where are these non-blocking requests used in a shared-nothing, single request application architecture? Is there some fetching multiple RSS feeds at once or so?

Well, I was incorrect. cURL doesn't support nonblocking, but it does support multiple HTTP requests occurring at the same time, while still blocking. And no, it isn't currently used, partly because it isn't implemented. The Socket Transport is really the only one that fully supports nonblocking and even that doesn't fully implement the multiple HTTP requests at one time.

This is really something that needs to be its own transport and while it might give a tiny bit of performance, I don't think enough hosts, or environments really need it as a feature. It is something that after your patch is implemented, might be implemented as a transport that a plugin author can add later.

[hakre]

Replying to jacobsantos:

Nothing from what I can read says that it is not allowed ...

This discussion is out of the scope of this ticket, but the docs clearly state that the request-body must be allowed (which implies that "not disallowed" is not enough), otherwise MUST NOT be used in requests. The get method specification does not give allowance nor does it provide any semantics about the entity-body. Compare to the post method specification for example, there you have full semantics.

But because of your comment I now better understand the issue. If some users ask for that bad practice (to say at least, I would say it's violating standards and to be considered harmful), then I can understand that they might want to be able to send such requests. As this is technically not a problem, why to create constraints where it can be kept flexible.

---

Replying to jacobsantos:

...

Thanks for the summary, i now much better see the whole picture. I suggest to commit the first changes here as they don't break anything of the API and reduce complexity of the code. And then to continue on the design issues. I like the idea that transports can signal availability on an installment so that the list of transports can be set up on first use (lazy loading). Additionally the transports should provide a method that can be used to obtain information about the features that a concrete transport supports:

• SSL
• Non-Blocking
• GET with Request-Body
• Headers
• Parallel Requests (?)
• Proxy (?)

The other ticket(s) already have some useful information, so it could be finished there then. One Idea I had is a base transport class that provides the needed functionality so that an extending concrete transport only need to add some data for this functionality. But that's in the scope of #11613 I assume.

If we can write down a specification of $args values to trigger which features, it should be even possible to backwards compatible still provide &_get.. / &_post... functions that successfully deliver working transports transparently. That would mean, that for most part of the application code, no changes would be necessary. We just would replace the current, failty caching mechanism with a better strategy based on $args and the (then technically correct) list of available transports.

[hakre]

Related: #11831

[nacin]

(In [17282]) Just a typo of the class name. fixes #14786.