-
Notifications
You must be signed in to change notification settings - Fork 39
Refactor exceptions #43
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
Changes from 3 commits
81ea240
e341e16
6dc544b
916048d
c54bbf1
895872d
a943f2d
c630ab7
4174e1a
df3bdbb
58a3d52
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
<?php | ||
|
||
namespace spec\Http\Client\Exception; | ||
|
||
use Http\Client\Exception\TransferException; | ||
use Psr\Http\Message\ResponseInterface; | ||
use PhpSpec\ObjectBehavior; | ||
|
||
class BatchExceptionSpec extends ObjectBehavior | ||
{ | ||
function let(TransferException $e, ResponseInterface $response) | ||
{ | ||
$this->beConstructedWith([$e], [$response]); | ||
} | ||
|
||
function it_is_initializable() | ||
{ | ||
$this->shouldHaveType('Http\Client\Exception\BatchException'); | ||
} | ||
|
||
function it_is_a_transfer_exception() | ||
{ | ||
$this->shouldHaveType('Http\Client\Exception\TransferException'); | ||
} | ||
|
||
function it_has_exceptions(TransferException $e, TransferException $e2) | ||
{ | ||
$this->getExceptions()->shouldReturn([$e]); | ||
$this->hasException($e)->shouldReturn(true); | ||
$this->hasException($e2)->shouldReturn(false); | ||
$this->hasExceptions()->shouldReturn(true); | ||
} | ||
|
||
function it_has_responses(ResponseInterface $response, ResponseInterface $response2) | ||
{ | ||
$this->getResponses()->shouldReturn([$response]); | ||
$this->hasResponse($response)->shouldReturn(true); | ||
$this->hasResponse($response2)->shouldReturn(false); | ||
$this->hasResponses()->shouldReturn(true); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
<?php | ||
|
||
namespace spec\Http\Client\Exception; | ||
|
||
use Psr\Http\Message\RequestInterface; | ||
use Psr\Http\Message\ResponseInterface; | ||
use PhpSpec\ObjectBehavior; | ||
|
||
class ClientExceptionSpec extends ObjectBehavior | ||
{ | ||
function let(RequestInterface $request, ResponseInterface $response) | ||
{ | ||
$response->getStatusCode()->willReturn(400); | ||
|
||
$this->beConstructedWith('message', $request, $response); | ||
} | ||
|
||
function it_is_initializable() | ||
{ | ||
$this->shouldHaveType('Http\Client\Exception\ClientException'); | ||
} | ||
|
||
function it_is_http_exception() | ||
{ | ||
$this->shouldHaveType('Http\Client\Exception\HttpException'); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
<?php | ||
|
||
namespace spec\Http\Client\Exception; | ||
|
||
use Psr\Http\Message\RequestInterface; | ||
use Psr\Http\Message\ResponseInterface; | ||
use PhpSpec\ObjectBehavior; | ||
|
||
class HttpExceptionSpec extends ObjectBehavior | ||
{ | ||
function let(RequestInterface $request, ResponseInterface $response) | ||
{ | ||
$response->getStatusCode()->willReturn(400); | ||
|
||
$this->beConstructedWith('message', $request, $response); | ||
} | ||
|
||
function it_is_initializable() | ||
{ | ||
$this->shouldHaveType('Http\Client\Exception\HttpException'); | ||
} | ||
|
||
function it_is_request_exception() | ||
{ | ||
$this->shouldHaveType('Http\Client\Exception\RequestException'); | ||
} | ||
|
||
function it_has_a_response(ResponseInterface $response) | ||
{ | ||
$this->getResponse()->shouldReturn($response); | ||
} | ||
|
||
function it_creates_a_client_exception(RequestInterface $request, ResponseInterface $response) | ||
{ | ||
$request->getRequestTarget()->willReturn('/uri'); | ||
$request->getMethod()->willReturn('GET'); | ||
$response->getStatusCode()->willReturn(404); | ||
$response->getReasonPhrase()->willReturn('Not Found'); | ||
|
||
$e = $this->create($request, $response); | ||
|
||
$e->shouldHaveType('Http\Client\Exception\ClientException'); | ||
$e->getMessage()->shouldReturn('Client error [url] /uri [http method] GET [status code] 404 [reason phrase] Not Found'); | ||
} | ||
|
||
function it_creates_a_server_exception(RequestInterface $request, ResponseInterface $response) | ||
{ | ||
$request->getRequestTarget()->willReturn('/uri'); | ||
$request->getMethod()->willReturn('GET'); | ||
$response->getStatusCode()->willReturn(500); | ||
$response->getReasonPhrase()->willReturn('Internal Server Error'); | ||
|
||
$e = $this->create($request, $response); | ||
|
||
$e->shouldHaveType('Http\Client\Exception\ServerException'); | ||
$e->getMessage()->shouldReturn('Server error [url] /uri [http method] GET [status code] 500 [reason phrase] Internal Server Error'); | ||
} | ||
|
||
function it_creates_an_http_exception(RequestInterface $request, ResponseInterface $response) | ||
{ | ||
$request->getRequestTarget()->willReturn('/uri'); | ||
$request->getMethod()->willReturn('GET'); | ||
$response->getStatusCode()->willReturn(100); | ||
$response->getReasonPhrase()->willReturn('Continue'); | ||
|
||
$e = $this->create($request, $response); | ||
|
||
$e->shouldHaveType('Http\Client\Exception\HttpException'); | ||
$e->getMessage()->shouldReturn('Unsuccessful response [url] /uri [http method] GET [status code] 100 [reason phrase] Continue'); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
<?php | ||
|
||
namespace spec\Http\Client\Exception; | ||
|
||
use Psr\Http\Message\RequestInterface; | ||
use PhpSpec\ObjectBehavior; | ||
|
||
class NetworkExceptionSpec extends ObjectBehavior | ||
{ | ||
function let(RequestInterface $request) | ||
{ | ||
$this->beConstructedWith('message', $request); | ||
} | ||
|
||
function it_is_initializable() | ||
{ | ||
$this->shouldHaveType('Http\Client\Exception\NetworkException'); | ||
} | ||
|
||
function it_is_request_exception() | ||
{ | ||
$this->shouldHaveType('Http\Client\Exception\RequestException'); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
<?php | ||
|
||
namespace spec\Http\Client\Exception; | ||
|
||
use Http\Client\Exception\RequestException; | ||
use Psr\Http\Message\RequestInterface; | ||
use PhpSpec\ObjectBehavior; | ||
|
||
class RequestExceptionSpec extends ObjectBehavior | ||
{ | ||
function let(RequestInterface $request) | ||
{ | ||
$this->beConstructedWith('message', $request); | ||
} | ||
|
||
function it_is_initializable() | ||
{ | ||
$this->shouldHaveType('Http\Client\Exception\RequestException'); | ||
} | ||
|
||
function it_has_a_request(RequestInterface $request) | ||
{ | ||
$this->getRequest()->shouldReturn($request); | ||
} | ||
|
||
function it_wraps_an_exception(RequestInterface $request) | ||
{ | ||
$e = new \Exception('message'); | ||
|
||
$requestException = $this->wrapException($request, $e); | ||
|
||
$requestException->getMessage()->shouldReturn('message'); | ||
} | ||
|
||
function it_does_not_wrap_if_request_exception(RequestInterface $request, RequestException $requestException) | ||
{ | ||
$this->wrapException($request, $requestException)->shouldReturn($requestException); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
<?php | ||
|
||
namespace spec\Http\Client\Exception; | ||
|
||
use Psr\Http\Message\RequestInterface; | ||
use Psr\Http\Message\ResponseInterface; | ||
use PhpSpec\ObjectBehavior; | ||
|
||
class ServerExceptionSpec extends ObjectBehavior | ||
{ | ||
function let(RequestInterface $request, ResponseInterface $response) | ||
{ | ||
$response->getStatusCode()->willReturn(500); | ||
|
||
$this->beConstructedWith('message', $request, $response); | ||
} | ||
|
||
function it_is_initializable() | ||
{ | ||
$this->shouldHaveType('Http\Client\Exception\ServerException'); | ||
} | ||
|
||
function it_is_http_exception() | ||
{ | ||
$this->shouldHaveType('Http\Client\Exception\HttpException'); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
<?php | ||
|
||
namespace spec\Http\Client\Exception; | ||
|
||
use PhpSpec\ObjectBehavior; | ||
use Prophecy\Argument; | ||
|
||
class TransferExceptionSpec extends ObjectBehavior | ||
{ | ||
function it_is_initializable() | ||
{ | ||
$this->shouldHaveType('Http\Client\Exception\TransferException'); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,110 @@ | ||
<?php | ||
|
||
namespace Http\Client\Exception; | ||
|
||
use Psr\Http\Message\ResponseInterface; | ||
|
||
/** | ||
* @author GeLo <[email protected]> | ||
*/ | ||
final class BatchException extends TransferException | ||
{ | ||
/** | ||
* @var TransferException[] | ||
*/ | ||
private $exceptions; | ||
|
||
/** | ||
* @var ResponseInterface[] | ||
*/ | ||
private $responses; | ||
|
||
/** | ||
* @param TransferException[] $exceptions | ||
* @param ResponseInterface[] $responses | ||
*/ | ||
public function __construct(array $exceptions = [], array $responses = []) | ||
{ | ||
parent::__construct('An error occurred when sending multiple requests.'); | ||
|
||
foreach ($exceptions as $e) { | ||
if (!$e instanceof TransferException) { | ||
throw new InvalidArgumentException('Exception is not an instanceof Http\Client\Exception\TransferException'); | ||
} | ||
} | ||
|
||
foreach ($responses as $response) { | ||
if (!$response instanceof ResponseInterface) { | ||
throw new InvalidArgumentException('Response is not an instanceof Psr\Http\Message\ResponseInterface'); | ||
} | ||
} | ||
|
||
|
||
$this->exceptions = $exceptions; | ||
$this->responses = $responses; | ||
} | ||
|
||
/** | ||
* Returns all exceptions | ||
* | ||
* @return TransferException[] | ||
*/ | ||
public function getExceptions() | ||
{ | ||
return $this->exceptions; | ||
} | ||
|
||
/** | ||
* Checks if a specific exception exists | ||
* | ||
* @param TransferException $exception | ||
* | ||
* @return boolean TRUE if there is the exception else FALSE. | ||
*/ | ||
public function hasException(TransferException $exception) | ||
{ | ||
return array_search($exception, $this->exceptions, true) !== false; | ||
} | ||
|
||
/** | ||
* Checks if any exception exists | ||
* | ||
* @return boolean | ||
*/ | ||
public function hasExceptions() | ||
{ | ||
return !empty($this->exceptions); | ||
} | ||
|
||
/** | ||
* Returns all responses | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should explain what this is. is it all responses of the transaction? or only those that have an error? is there a way to associate the individual response with an exception and vice-versa? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is actually something that I ported from Ivory and Guzzle. In Ivory the array key (order of elements) identifies exception-response pairs. This method returns only the errored messages. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lets explain the semantics in the phpdoc to make it more useful and accessible for users. the doc says "Returns all responses" but if i understand you correctly, it should say "Returns all failed responses. The array keys are the same as in getExceptions". and we should also update the getExceptions phpdoc to say it has the same keys as getResponses. (though i expect most of those exceptions to carry the response in them again, no?). which brings me to wonder why we need this method at all? if there is an exception before we get a response, there is no response obviously. if we raise the exception because of a 4x or 5x response, we can put the response into that exception. what is the use case for having the responses separately? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, there is unnecessary complexity here. This thing is ported as is from Ivory. However this exception also handles other exceptions which does not even have a response. So I think we should just stick to exceptions. If a response is available, it should be carried in the exception, as you said. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another possibility is to store exceptions in an SplObjectStorage indexed by the request objects. This would be the best way to identify every exceptions. I am also thinking about adding a BatchResponse class as well, which also holds an SplObjectStorage internally, which would contain responses indexed by requests. WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, I would keep SplObjectStorage an internal thing. Also, about the batch response: I more and more have the feeling that it is necessary. Currently it is impossible to pair requests/responses if an exception is thrown, because array indexes are probably messed up that way. I am also thinking about if exceptions in case of a batch request should be thrown at all, or just returned in a general response object. Using this Batch exception, successful responses are unable to be retrieved. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But I think that should be resolved in a separate issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lets open a new issue for that. and check what guzzle and the others do
for this. (or even curl php).
and lets drop the getResponses from here as that definitely does not belong
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #47. I've already dropped the getResponses. Checking guzzle right now... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you still need to push dropping getResponses. though if we decide to drop BatchException we don't need this whole class. |
||
* | ||
* @return ResponseInterface[] | ||
*/ | ||
public function getResponses() | ||
{ | ||
return $this->responses; | ||
} | ||
|
||
/** | ||
* Checks if a specific response exists | ||
* | ||
* @param ResponseInterface $response | ||
* | ||
* @return boolean | ||
*/ | ||
public function hasResponse(ResponseInterface $response) | ||
{ | ||
return array_search($response, $this->responses, true) !== false; | ||
} | ||
|
||
/** | ||
* Checks if any response exists | ||
* | ||
* @return boolean | ||
*/ | ||
public function hasResponses() | ||
{ | ||
return !empty($this->responses); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
<?php | ||
|
||
namespace Http\Client\Exception; | ||
|
||
/** | ||
* Thrown when a client error (4xx) is encountered | ||
* | ||
* @author Márk Sági-Kazár <[email protected]> | ||
*/ | ||
class ClientException extends HttpException | ||
{ | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. empty line is not psr-2, sorry There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good to know, I always wondered if there is a standard for empty classes. |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indention is wrong