Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Contextually check for a valid transport #101

Merged
merged 6 commits into from
Feb 11, 2014
Merged

Contextually check for a valid transport #101

merged 6 commits into from
Feb 11, 2014

Conversation

ozh
Copy link
Collaborator

@ozh ozh commented Feb 5, 2014

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 have get_transport(array $capabilities). Currently only ssl is 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 (true or false depending on whether $url starts with https or 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

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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this serialize to the same thing regardless of array order? Haven't tested, but I suspect no, so might need a sort.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely. Nice catch.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.14%) when pulling 2695b1a on ozh:transport_with_context into 5052e11 on rmccue:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.14%) when pulling 2695b1a on ozh:transport_with_context into 5052e11 on rmccue:master.

@ozh
Copy link
Collaborator Author

ozh commented Feb 7, 2014

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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to kill the else here, as the if block throws anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you misunderstood the test: if no preg_match, throw, else: populate options['need_ssl'] as needed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that. I thought it would make it clearer where that $matches comes from. Removing it :)

@rmccue
Copy link
Collaborator

rmccue commented Feb 7, 2014

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?

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.)

@rmccue
Copy link
Collaborator

rmccue commented Feb 7, 2014

(Looks like the failing tests here are due to timeouts, not a bug. Looks like we might need the local httpbin mirror. :( )

@rmccue
Copy link
Collaborator

rmccue commented Feb 10, 2014

Alright, I'm happy to merge this, but let's kill the needs_ssl option. Thanks again for the patch! 🍍

@rmccue rmccue added this to the 1.7 milestone Feb 10, 2014
@ozh
Copy link
Collaborator Author

ozh commented Feb 10, 2014

Sorry, fixed.

[A stupid message was here about intercepting exceptions to avoid timeouts on httpbin.org. Redacted.]

Also, please, simply move the 'hhvm' bit of the .travis.yml into its own branch :) (that will start the PR for #93)

rmccue added a commit that referenced this pull request Feb 11, 2014
Contextually check for a valid transport
@rmccue rmccue merged commit a689479 into WordPress:master Feb 11, 2014
rmccue added a commit that referenced this pull request Mar 23, 2014
See #101. Turns out fsockopen was overriding this, but we have contextual
checking support now, so let's use that.
rmccue added a commit that referenced this pull request May 18, 2014
See #101. Turns out fsockopen was overriding this, but we have contextual
checking support now, so let's use that.
@ozh ozh deleted the transport_with_context branch April 17, 2015 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants