[wp-trac] [WordPress Trac] #14786: WP_Http Transport test order refactoring

[WordPress Trac]

#14786: WP_Http Transport test order refactoring
--------------------------+-------------------------------------------------
 Reporter:  hakre         |       Owner:                        
     Type:  defect (bug)  |      Status:  new                   
 Priority:  normal        |   Milestone:  Awaiting Review       
Component:  HTTP          |     Version:  3.0.1                 
 Severity:  normal        |    Keywords:  has-patch dev-feedback
--------------------------+-------------------------------------------------

Comment(by jacobsantos):

 Replying to [comment:18 hakre]:
 > Replying to [comment:16 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.''
 ([http://php.net/manual/en/language.oop5.static.php 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 [comment:17 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.

-- 
Ticket URL: <http://core.trac.wordpress.org/ticket/14786#comment:21>
WordPress Trac <http://core.trac.wordpress.org/>
WordPress blogging software