Contextually check for a valid transport #101
Conversation
| if (self::$transport !== null) { | ||
| return new self::$transport(); | ||
| // array of capabilities as a string to be used as an array key | ||
| $cap_string = serialize($capabilities); |
There was a problem hiding this comment.
Will this serialize to the same thing regardless of array order? Haven't tested, but I suspect no, so might need a sort.
There was a problem hiding this comment.
Absolutely. Nice catch.
|
More or less off topic: probably not a big deal here but I'm not sure what's that coverage and how it works. How can I make it not decrease but 0.14% on that PR? |
| throw new Requests_Exception('Only HTTP requests are handled.', 'nonhttp', $url); | ||
| } else { |
There was a problem hiding this comment.
I'd like to kill the else here, as the if block throws anyway.
There was a problem hiding this comment.
I think you misunderstood the test: if no preg_match, throw, else: populate options['need_ssl'] as needed.
There was a problem hiding this comment.
Right, but we don't need the else, since there's two options: preg_match doesn't match, so we throw (and exit from the function), or we continue the function. There's no need for the else, since the rest of the function is basically an else. :)
There was a problem hiding this comment.
Oh, that. I thought it would make it clearer where that $matches comes from. Removing it :)
The coverage is the percentage of lines covered by tests. Decreasing by 0.14% means that you've probably got a couple of lines that you added that aren't covered by tests. If you click on the coverage bubble, you can see the exact number of lines that changed. You can then click on the files themselves and see in green/red which lines are covered/not covered by tests (and on the right, exactly how many tests run that line of code). (Note that it's not always accurate, since Coveralls seems to have a few bugs. I tend to ignore anything under 1% usually, but I check the breakdown anyway to make sure.) |
|
(Looks like the failing tests here are due to timeouts, not a bug. Looks like we might need the local httpbin mirror. :( ) |
|
Alright, I'm happy to merge this, but let's kill the |
|
Sorry, fixed. [A stupid message was here about intercepting exceptions to avoid timeouts on httpbin.org. Redacted.] Also, please, simply move the |
Contextually check for a valid transport
See #101. Turns out fsockopen was overriding this, but we have contextual checking support now, so let's use that.
See #101. Turns out fsockopen was overriding this, but we have contextual checking support now, so let's use that.
Currently a valid transport is selected with simple tests (functions exist).
This can lead to error: using streams but without having openssl, or using curl with an old build, a transport that cannot perform HTTPS requests will still be selected.
My PR checks for a valid transport in a given context: do we need to make SSL requests or not?
The main change is that instead of
get_transport()we now haveget_transport(array $capabilities). Currently onlysslis checked but we can imagine this being expanded if needed in the future.A new option is introduced and documented,
need_ssl, but this should be mostly left untouched as its default value will be set to what is needed (trueorfalsedepending on whether$urlstarts withhttpsor not)Of course a check for a valid transport is performed only once for the same capability
I think everything is pretty much self explanatory, but feel free to ask for details if needed