Skip to content
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

[HttpClient] exceptions carry response #30567

Merged
merged 1 commit into from Mar 19, 2019

Conversation

Projects
None yet
6 participants
@antonch1989
Copy link
Contributor

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
@nicolas-grekas
Copy link
Member

nicolas-grekas left a 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 */

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 14, 2019

Member

should be removed

This comment has been minimized.

@antonch1989

antonch1989 Mar 14, 2019

Author Contributor

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

This comment has been minimized.

@linaori

linaori Mar 14, 2019

Contributor

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

This comment has been minimized.

@antonch1989

antonch1989 Mar 14, 2019

Author Contributor

In my opinion the current naming is fine, let's wait for some other comments

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 16, 2019

Member

I'm fine with the name HttpExceptionInterface personnaly, but the interface should extend ExceptionInterface

Show resolved Hide resolved src/Symfony/Contracts/HttpClient/Exception/HttpExceptionInterface.php
use Symfony\Contracts\HttpClient\ResponseInterface;
interface HttpExceptionInterface

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 16, 2019

Member

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 16, 2019

Member

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 18, 2019

Member

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 18, 2019

Member

Base interface for HTTP-related exceptions.

@fabpot

fabpot approved these changes Mar 19, 2019

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Mar 19, 2019

Thank you @antonch1989.

@fabpot fabpot force-pushed the antonch1989:issue-30502-exceptions-carry-respons branch from bf99300 to 103448c Mar 19, 2019

@fabpot fabpot merged commit 103448c into symfony:master Mar 19, 2019

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor was unable to build non-mergeable pull request
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 19, 2019

feature #30567 [HttpClient] exceptions carry response (antonch1989)
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.