Refactor exceptions

[sagikazarmark]

This PR tries to address the following:

• Separated exceptions (Separate interfaces for client, server and networking exceptions #26)
• Multi/Batch exception is final (Make MultiException final #41)
• Better naming
• Exceptions are immutable (removed all setter/cleaner methods)

Feel free to start discussion about any of the above.

...let me start:

Setters are completely removed from exceptions (no setResponse, setRequest). While I think this is the better way, I can imagine a case when some kind of event driven logic wants to modify the exceptions. Any ideas? Should we preserve the setter logic or force any event-driven one to create a new exception?

Differences from Guzzle: Guzzle has a Request-Response exception pair, plus almost everything extends request exception. One difference is that the Network exception does NOT extend the request exception. It can be used in cases where the request does not get to a state where it makes sense to return it. Like a connection timed out. Another difference is that Response exception is called Http.

[sagikazarmark]

I am waiting for feedback from @hannesvdvreken @dbu

[dbu]

indention is wrong

[dbu]

looks good! i added some comments on the phpdoc and one visibility issue i suspect.

it will be tricky to ensure that all adapters actually catch all relevant exceptions and identify them to throw the right exception in turn.

[hannesvdvreken]

empty line is not psr-2, sorry

[sagikazarmark]

Good to know, I always wondered if there is a standard for empty classes.

[hannesvdvreken]

👍

For documentation: here's the inheritance tree for Exceptions:

\InvalidArgumentException
`- \Http\Client\Exception\InvalidArgumentException (implements \Http\Client\Exception)

\RuntimeException
`- \Http\Client\Exception\RuntimeException (implements \Http\Client\Exception)
   `- \Http\Client\Exception\TransferException
      |- \Http\Client\Exception\BatchException
      `- \Http\Client\Exception\RequestException
         |- \Http\Client\Exception\NetworkException
         `- \Http\Client\Exception\HttpException
            |- \Http\Client\Exception\ServerException
            `- \Http\Client\Exception\ClientException

Now copied here: php-http/documentation#7 to be incorporated in documentation.

[dbu]

not sure if we want to put that into the doc. there is one mistake here, ServerException extends HttpException, not RequestException. (the code is correct, but the tree in above post not)

[hannesvdvreken]

@dbu You're right. Edited the comment.

[sagikazarmark]

Hey guys,

Thanks for all the comments, I really appreciate it.

@dbu thanks for helping out with descriptions, I am not really good at it.

About ensuring that the correct exceptions are thrown: the interface declares that exceptions must implement out interface, so even if the exception type is not correctly determined, it is still possible to catch all exceptions thrown by the client.

I wonder if we should be strict about private types. Actually, I am not against it. We could even make Client and Server exceptions final, since those are thrown by the HttpException itself.

@hannesvdvreken Thanks for the inheritance tree, looks good. We should definitely put it in the docs once we get there (or at least something similar. I admit that the inheritance tree is a bit complex and it is needed to be coved in the docs).

[dbu]

yes, the doc should definitely explain the exception concept. the tree is imho nice but the most important is explaining in what situation which exception will be thrown. is there an issue about for doc already where we can add this?

[sagikazarmark]

Not yet. The main reason I neglected documentation so far is the domain: @egeloen owns it and we cannot use it.

[dbu]

php-http/documentation#7

[sagikazarmark]

Network, Client and Server exceptions are final. Although I am not sure about the Network exception. Client and Server can be used in limited number of cases (namely 4xx and 5xx errors). Network exception might be used in cases like timeout, connection refused. While I don't think we should include those in the contract package, should we allow extending the NetworkException in such cases?

[dbu]

i am not that worried with people extending that exception so lets not make the network one final.