[HttpClient] exceptions carry response #30567
Conversation
antonch1989
commented
Mar 14, 2019
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #30502 |
| License | MIT |
| Doc PR |
There was a problem hiding this comment.
Thanks for starting this.
I think we should add the method in an interface in contracts.
If we add it to the base ExceptionInterface, it should be nullable, because the response object might not be instantiated yet when this is thrown.
We could alternatively add a new base HttpExceptionInterface that would add this method to Server/Client/RedirectionExceptionInterface - that's mean the method is not on TransportExceptionInterface.
Any preference?
| @@ -20,8 +20,12 @@ | |||
| */ | |||
| trait HttpExceptionTrait | |||
| { | |||
| /** @var ResponseInterface */ | |||
There was a problem hiding this comment.
the solution with a new interface looks cleaner, I'll do this way if there are no objections
|
|
||
| use Symfony\Contracts\HttpClient\ResponseInterface; | ||
|
|
||
| interface HttpExceptionInterface |
There was a problem hiding this comment.
What do you think of HttpResponseExceptionInterface? The current name doesn't imply it has anything to do with a response, while the getter implies there's always a response
There was a problem hiding this comment.
In my opinion the current naming is fine, let's wait for some other comments
There was a problem hiding this comment.
I'm fine with the name HttpExceptionInterface personnaly, but the interface should extend ExceptionInterface
|
|
||
| use Symfony\Contracts\HttpClient\ResponseInterface; | ||
|
|
||
| interface HttpExceptionInterface |
There was a problem hiding this comment.
I'm fine with the name HttpExceptionInterface personnaly, but the interface should extend ExceptionInterface
| @@ -18,6 +18,6 @@ | |||
| * | |||
| * @experimental in 1.1 | |||
| */ | |||
| interface RedirectionExceptionInterface extends ExceptionInterface | |||
| interface RedirectionExceptionInterface extends ExceptionInterface, HttpExceptionInterface | |||
There was a problem hiding this comment.
interface RedirectionExceptionInterface extends HttpExceptionInterface
please do the same ClientExceptionInterface and ServerExceptionInterface
| @@ -18,6 +18,6 @@ | |||
| * | |||
| * @experimental in 1.1 | |||
| */ | |||
| interface ClientExceptionInterface extends ExceptionInterface | |||
| interface ClientExceptionInterface extends ExceptionInterface, HttpExceptionInterface | |||
There was a problem hiding this comment.
ExceptionInterface should be removed as it's now embeded in HttpExceptionInterface
same in the 2 other exception interfaces
| use Symfony\Contracts\HttpClient\ResponseInterface; | ||
|
|
||
| /** | ||
| * Interface for getting response from an exception |
There was a problem hiding this comment.
Base interface for HTTP-related exceptions.
|
Thank you @antonch1989. |
bf99300 to
103448c
Compare
This PR was squashed before being merged into the 4.3-dev branch (closes #30567). Discussion ---------- [HttpClient] exceptions carry response | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | #30502 | License | MIT | Doc PR | Commits ------- 103448c [HttpClient] exceptions carry response