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
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 26 additions & 12 deletions library/Requests.php
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,9 @@ class Requests {
*
* Use {@see get_transport()} instead
*
* @var string|null
* @var array
*/
public static $transport = null;
public static $transport = array();

/**
* This is a static class, do not instantiate it
Expand Down Expand Up @@ -147,11 +147,16 @@ public static function add_transport($transport) {
* @throws Requests_Exception If no valid transport is found (`notransport`)
* @return Requests_Transport
*/
protected static function get_transport() {
protected static function get_transport($capabilities = array()) {
// Caching code, don't bother testing coverage
// @codeCoverageIgnoreStart
if (self::$transport !== null) {
return new self::$transport();
// array of capabilities as a string to be used as an array key
ksort($capabilities);
$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.


// Don't search for a transport if it's already been done for these $capabilities
if (isset(self::$transport[$cap_string]) && self::$transport[$cap_string] !== null) {
return new self::$transport[$cap_string]();
}
// @codeCoverageIgnoreEnd

Expand All @@ -167,17 +172,17 @@ protected static function get_transport() {
if (!class_exists($class))
continue;

$result = call_user_func(array($class, 'test'));
$result = call_user_func(array($class, 'test'), $capabilities);
if ($result) {
self::$transport = $class;
self::$transport[$cap_string] = $class;
break;
}
}
if (self::$transport === null) {
if (self::$transport[$cap_string] === null) {
throw new Requests_Exception('No working transports found', 'notransport', self::$transports);
}

return new self::$transport();
return new self::$transport[$cap_string]();
}

/**#@+
Expand Down Expand Up @@ -277,6 +282,10 @@ public static function patch($url, $headers, $data = array(), $options = array()
* transport object. Defaults to the first working transport from
* {@see getTransport()}
* (string|Requests_Transport, default: {@see getTransport()})
* - `needs_ssl`: whether the chosen transport will need to be able to perform HTTPS
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need to be an option, or can we just autodetect?

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 wasn't sure there would not be a case scenario where someone would want to force that... Couldn't think of one myself, but I'm just the plain regular HTTP query guy, so I made that an option. What do you think? Just autodetect?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think? Just autodetect?

Let's go with just autodetection for now; if someone needs access to this, they're probably doing it wrong. Worst case scenario, we can always add APIs, but we can't remove them as easily. :)

* requests. If unset, this option automatically sets to True when the requested
* URL starts with 'https://'
* (boolean, default: false if $url is HTTP, true if $url is HTTPS)
* - `hooks`: Hooks handler.
* (Requests_Hooker, default: new Requests_Hooks())
* - `verify`: Should we verify SSL certificates? Allows passing in a custom
Expand Down Expand Up @@ -314,7 +323,8 @@ public static function request($url, $headers = array(), $data = array(), $type
}
}
else {
$transport = self::get_transport();
$capabilities = array('ssl' => $options['needs_ssl']);
$transport = self::get_transport($capabilities);
}
$response = $transport->request($url, $headers, $data, $options);

Expand Down Expand Up @@ -479,8 +489,12 @@ protected static function get_default_options($multirequest = false) {
* @return array $options
*/
protected static function set_defaults(&$url, &$headers, &$data, &$type, &$options) {
if (!preg_match('/^http(s)?:\/\//i', $url)) {
if (!preg_match('/^http(s)?:\/\//i', $url, $matches)) {
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 :)

if (empty($options['needs_ssl'])) {
$options['needs_ssl'] = ($matches[0] == 'https://');
}
}

if (empty($options['hooks'])) {
Expand Down
14 changes: 12 additions & 2 deletions library/Requests/Transport/cURL.php
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,17 @@ protected static function format_get($url, $data) {
* @codeCoverageIgnore
* @return boolean True if the transport is valid, false otherwise.
*/
public static function test() {
return (function_exists('curl_init') && function_exists('curl_exec'));
public static function test($capabilities = array()) {
if (!function_exists('curl_init') && !function_exists('curl_exec'))
return false;

// If needed, check that our installed curl version supports SSL
if (isset( $capabilities['ssl'] ) && $capabilities['ssl']) {
$curl_version = curl_version();
if (!(CURL_VERSION_SSL & $curl_version['features']))
return false;
}

return true;
}
}
13 changes: 11 additions & 2 deletions library/Requests/Transport/fsockopen.php
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,16 @@ public function verify_certificate_from_context($host, $context) {
* @codeCoverageIgnore
* @return boolean True if the transport is valid, false otherwise.
*/
public static function test() {
return function_exists('fsockopen');
public static function test($capabilities = array()) {
if (!function_exists('fsockopen'))
return false;

// If needed, check that streams support SSL
if (isset( $capabilities['ssl'] ) && $capabilities['ssl']) {
if (!extension_loaded('openssl') || !function_exists('openssl_x509_parse'))
return false;
}

return true;
}
}