Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpClient] exceptions carry response#30567
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[HttpClient] exceptions carry response#30567
Uh oh!
There was an error while loading.Please reload this page.
Conversation
antonch1989 commentedMar 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 |
nicolas-grekas left a comment
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.
Thanks for starting this.
I think we should add the method in an interface in contracts.
If we add it to the baseExceptionInterface, it should be nullable, because the response object might not be instantiated yet when this is thrown.
We could alternatively add a new baseHttpExceptionInterface that would add this method toServer/Client/RedirectionExceptionInterface - that's mean the method is not onTransportExceptionInterface.
Any preference?
| */ | ||
| trait HttpExceptionTrait | ||
| { | ||
| /** @var ResponseInterface */ |
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.
should be removed
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.
the solution with a new interface looks cleaner, I'll do this way if there are no objections
| useSymfony\Contracts\HttpClient\ResponseInterface; | ||
| interface HttpExceptionInterface |
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.
What do you think ofHttpResponseExceptionInterface? The current name doesn't imply it has anything to do with a response, while the getter implies there'salways a response
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.
In my opinion the current naming is fine, let's wait for some other comments
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.
I'm fine with the nameHttpExceptionInterface personnaly, but the interface should extendExceptionInterface
Uh oh!
There was an error while loading.Please reload this page.
| useSymfony\Contracts\HttpClient\ResponseInterface; | ||
| interface HttpExceptionInterface |
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.
I'm fine with the nameHttpExceptionInterface personnaly, but the interface should extendExceptionInterface
| * @experimental in 1.1 | ||
| */ | ||
| interface RedirectionExceptionInterfaceextends ExceptionInterface | ||
| interface RedirectionExceptionInterfaceextends ExceptionInterface, HttpExceptionInterface |
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.
interface RedirectionExceptionInterface extends HttpExceptionInterface
please do the sameClientExceptionInterface andServerExceptionInterface
| * @experimental in 1.1 | ||
| */ | ||
| interface ClientExceptionInterfaceextends ExceptionInterface | ||
| interface ClientExceptionInterfaceextends ExceptionInterface, HttpExceptionInterface |
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.
ExceptionInterface should be removed as it's now embeded in HttpExceptionInterface
same in the 2 other exception interfaces
| useSymfony\Contracts\HttpClient\ResponseInterface; | ||
| /** | ||
| * Interface for getting response from an exception |
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.
Base interface for HTTP-related exceptions.
fabpot commentedMar 19, 2019
Thank you@antonch1989. |
bf99300 to103448cCompareThis 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 <!-- seehttps://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