From 9fbaa532e6e8e68eaf4252d9c00089420c9a7d02 Mon Sep 17 00:00:00 2001 From: Michael Dowling Date: Sun, 21 Sep 2014 12:03:37 -0700 Subject: [PATCH] Cleaning up ring responses, adding guard to ensure toString does not throw --- src/Client.php | 71 +++++++++++++++------------- src/Message/FutureResponse.php | 7 ++- src/RingBridge.php | 19 ++++---- tests/ClientTest.php | 4 +- tests/Message/FutureResponseTest.php | 12 +++++ 5 files changed, 67 insertions(+), 46 deletions(-) diff --git a/src/Client.php b/src/Client.php index 366a018e..1f97b87a 100644 --- a/src/Client.php +++ b/src/Client.php @@ -249,8 +249,7 @@ class Client implements ClientInterface // Future responses do not need to process right away. if ($response instanceof RingFutureInterface) { - $trans->response = $this->createFutureResponse($trans, $response); - return $trans->response; + return $this->createFutureResponse($trans, $response); } // Throw a wrapped exception if the transactions has an error. @@ -266,33 +265,6 @@ class Client implements ClientInterface throw RingBridge::getNoRingResponseException($trans->request); } - private function createFutureResponse( - Transaction $trans, - RingFutureInterface $response - ) { - // Create a future response that's hooked up to the ring future. - return new FutureResponse( - // Dereference function - function () use ($response, $trans) { - // Dereference the underlying future and block until complete. - $response->deref(); - // Throw an exception if present. Remove them to prevent this. - if ($trans->exception) { - throw RequestEvents::wrapException($trans->request, $trans->exception); - } - // No exception, so the transaction should have a response. - if ($trans->response) { - return $trans->response; - } - throw RingBridge::getNoRingResponseException($trans->request); - }, - // Cancel function. Just proxy to the underlying future. - function () use ($response) { - return $response->cancel(); - } - ); - } - /** * Get an array of default options to apply to the client * @@ -319,6 +291,33 @@ class Client implements ClientInterface return $settings; } + private function createFutureResponse( + Transaction $trans, + RingFutureInterface $response + ) { + // Create a future response that's hooked up to the ring future. + return new FutureResponse( + // Dereference function + function () use ($response, $trans) { + $response->deref(); + // Throw an exception if present. You need to remove transaction + // exceptions to prevent them from being thrown. + if ($trans->exception) { + throw RequestEvents::wrapException($trans->request, $trans->exception); + } elseif ($trans->response) { + // Return the response if one was set on the transaction. + return $trans->response; + } + // If we know that the events were fired, then there's a problem. + throw RingBridge::getNoRingResponseException($trans->request); + }, + // Cancel function. Just proxy to the underlying future. + function () use ($response) { + return $response->cancel(); + } + ); + } + /** * Expand a URI template and inherit from the base URL if it's relative * @@ -328,15 +327,19 @@ class Client implements ClientInterface */ private function buildUrl($url) { + // URI template (absolute or relative) if (!is_array($url)) { - if (strpos($url, '://')) { - return (string) $url; - } - return (string) $this->baseUrl->combine($url); - } elseif (strpos($url[0], '://')) { + return strpos($url, '://') + ? (string) $url + : (string) $this->baseUrl->combine($url); + } + + // Absolute URL + if (strpos($url[0], '://')) { return Utils::uriTemplate($url[0], $url[1]); } + // Combine the relative URL with the base URL return (string) $this->baseUrl->combine( Utils::uriTemplate($url[0], $url[1]) ); diff --git a/src/Message/FutureResponse.php b/src/Message/FutureResponse.php index ac709420..f9fee2a6 100644 --- a/src/Message/FutureResponse.php +++ b/src/Message/FutureResponse.php @@ -56,7 +56,12 @@ class FutureResponse implements ResponseInterface, FutureInterface public function __toString() { - return $this->result->__toString(); + try { + return $this->result->__toString(); + } catch (\Exception $e) { + trigger_error($e->getMessage(), E_USER_WARNING); + return ''; + } } public function getProtocolVersion() diff --git a/src/RingBridge.php b/src/RingBridge.php index bc191781..f210358b 100644 --- a/src/RingBridge.php +++ b/src/RingBridge.php @@ -131,8 +131,6 @@ class RingBridge ? $response['transfer_info'] : []; if (!empty($response['status'])) { - // Transition to "error" if an error is present. Otherwise, complete. - $trans->state = isset($response['error']) ? 'error' : 'complete'; $options = []; if (isset($response['version'])) { $options['protocol_version'] = $response['version']; @@ -142,7 +140,7 @@ class RingBridge } $trans->response = $messageFactory->createResponse( $response['status'], - $response['headers'], + isset($response['headers']) ? $response['headers'] : [], isset($response['body']) ? $response['body'] : null, $options ); @@ -201,11 +199,14 @@ class RingBridge */ public static function getNoRingResponseException(RequestInterface $request) { - return new RequestException( - 'Sending the request did not return a response, exception, or ' - . 'populate the transaction with a response. This is most likely ' - . 'due to an incorrectly implemented Guzzle Ring adapter.', - $request - ); + $message = <<assertFalse($future->cancel()); $future->getStatusCode(); } + + public function testExceptionInToStringTriggersError() + { + $future = new FutureResponse(function () {}); + $err = ''; + set_error_handler(function () use (&$err) { + $err = func_get_args()[1]; + }); + echo $future; + restore_error_handler(); + $this->assertContains('Future did not return a valid response', $err); + } }