HTTP API may make a request on an unsupporting transport

[dd32]

At present, When the HTTP API is called, It iterates through the transports testing each one to see if it can support the current request params (eg. SSL). This result is then stored for future requests.

The issue i've run into, Is that i've added a case to a transport, that it should not be used if its a HEAD request (As it simply doesnt support it on that transport), The problem is, if a request is made before me, for say, a GET request, that transport gets cached for it, even if it doesnt support it..

The attached patch adds a $key to the cache, which is a md5 of the serialized args.

In most cases, this doesnt cause any issues, but if a host is using a lesser transport which cannot do everything (Such as FOpen) then errors can be hit that the transport doesnt support it.

[dd32]

logic typo fix in posttransports empty check

[Denis-de-Bernardy]

won't the serialized args based key result in each transport being tested on every call?

[dd32]

won't the serialized args based key result in each transport being tested on every call?

Just the calls which have varying args. The only non-arg in that list is the post body, which can probably be nulled out, that way multiple post requests will be able to use the cache again. It doesnt contain the URL, so not a problem there.. But does differentiate between GET/POST/etc.

It is unavoidable that they get tested multiple times, It'd be much simpler if the fallback to the next transport should the first fail worked properly, #8622 that way, the arg could run ::test() itself when being called, and if it wasnt up for the job, simply pass it onto the next best transport.

Most pages will only have a small number of requests made, while it may add another couple of ms to the function, in my opinion, the benifit of the request actually suceeding and returning the correct data is worth more.

[dd32]

Latest patch generates keys as such string(8) "GET,1,,1" much simpler, and only contains information which the ::test() args need.

[Denis-de-Bernardy]

I've uploaded 11613.5.diff, which is based on the .4.diff. The difference is it checks for isset() rather than empty(), since checking for empty when there are no valid transports could potentially lead the tests to be redone each time.

Re tests done, I merely stuck to verifying that RSS widgets and update checking still work.

[dd32]

The difference is it checks for isset() rather than empty(), since checking for empty when there are no valid transports could potentially lead the tests to be redone each time.

Thats a fair enough change.

[hakre]

Is it possible to deactivate caching for a certain transport operation so an author an actively prevent such race-conditions in the cache?

[dd32]

Is it possible to deactivate caching for a certain transport operation so an author an actively prevent such race-conditions in the cache?

No. And probably shouldnt be supported.

However, With this patch, It'd be possible to break the cache if the code is calling with different args which the current transport doesnt understand.

Developers can always call a specific transport directly if they require, but its highly unsupported.

[jacobsantos]

If this is the case, then why won't you just do the check in the transport and then change the HTTPS scheme to a HTTP one? The transports should only be checking whether it supports SSL, supports sending a body and supported by PHP. Everything else should just be done in the transport, unless it is something that needs to be supported by every transport then needs to be a method or a class.

[dd32]

then why won't you just do the check in the transport and then change the HTTPS scheme to a HTTP one?

Changing from HTTPS to HTTP is a no-no, If someone asks for a HTTPS connection, You give them a HTTPS connection.

The problem here is, that not all the transports can support a HTTPS connection, If you have multiple connections being made on a pageload, the first request determines which transport is used for that connection

So if the first request is a HTTP request, It may be made over Class_NoSSL, Then a HTTPS request comes along (of the same GET/POST type), it'll be passed to Class_NoSSL as ::test() passed last time.

This is due to #8622, Which should probably be fixed instead of this, or along side.

[jacobsantos]

I see, thanks for the clarification. To me the ticket seemed to be that servers did not support secure HEAD requests and should be non-SSL instead. Given that it is quite frankly improbable that a WordPress installation is going to change servers any time soon, it seems fair to say that a refactoring of the code is in order. Right now it seems quite the mess.

[dd32]

To me the ticket seemed to be that servers did not support secure HEAD requests and should be non-SSL instead.

That was just an example.

It can happen with any functionality which one transport supports that another doesnt.

HEAD requests, SSL, SSL options, Custom headers, etc. If the transport chosen for the first request doesnt support the functions the 2nd request requires, the 2nd request will fail.

[jacobsantos]

Non-tested prototype for better solution.

[hakre]

Related: #14786, #8622

[dd32]

With the removal of the fopen transport (Which only supported GET requests), there is no longer a requirement for a difference in the GET/POST transport orders.

This in addition to #8622 is preventing unit testing of the HTTP API as well.

The following commit replaces _getTransport() & _postTransport() and adds a _dispatch_request() private method, the simple purpose of _dispatch_request() is to take the $args and determine a transport which the request can be made on, and then dispatch the request to that transport worker. (In addition, it clarifys the PHPDoc on the return of WP_HTTP::request() that it can return a WP_Error instance upon failure)

[dd32]

(In [17550]) Fix WP_HTTP to only make a request upon a working transport, as well as to allow Unit Testing. Removes the getTransport() & postTransport() methods as they're no longer needed, replaces them with a single _dispatch_request() method. Fixes #11613

[hakre]

Follow-Up; Related: #16959

[hakre]

In [17550] I've spotted a bug in the new introduced function _dispatch_request().

Please see Related: #16978

[dd32]

(In [17566]) Use the correct variable name; Simpler static initialisation. Props hakre. See #11613 & #16978

[westi]

The changes in [17550] break any plugin which relies on http_transport_post_debug or http_transport_get_debug to work around bugs in the SSL detection code in the WP_HTTP class like Jetpack does.

We don't normally remove actions or filters like this.

[westi]

mdawaffe pointed out that the patches on ​http://core.trac.wordpress.org/ticket/16606 negate the need for these actions in Jetpack so we should get those in and then all will be ok :)