-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
[HttpClient] Update http_client.rst #14499
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
Conversation
I started from scratch after symfony#14174 * The most important change is that the old text "your code is expected to handle it" didn't make it clear *how* people are supposed to handle it. * I omitted the note that a `Symfony\Component\HttpClient\Exception\ClientException` is thrown. Is this important? Or is the existing `Symfony\\Contracts\\HttpClient\\Exception\\HttpExceptionInterface` enough?
@nicolas-grekas Please review |
http_client.rst
Outdated
|
||
$response = $client->request('GET', 'https://httpbin.org/status/403'); | ||
if (403 === $response->getStatusCode()) { | ||
$response->getContent(); |
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.
This will throw.
Checking the status code disables throwing by the destructor only.
The behavior of the other methods is always predictable and controlled by the $throw
argument.
I agree we should find a better way to explain all this :)
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.
Damn, I checked for string '403'
, while in fact it's int
...
Please take a look again!
And if you still have some nerves left: :-)
Maybe it would be easier to get across when we could explain the idea behind it! The $throw
argument is straightforward. But why did you do the destructor / getStatusCode()
thing? What use cases did you have in mind?
And one more: For those 3 methods together (getHeaders()
, getContent()
and toArray()
), do you have some internal (nick) name we could use? Something like "contentFetchers" or so?
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.
Ping @nicolas-grekas :-)
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.
@nicolas-grekas Could you please merge this, before it ages away? :-)
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.
Here are some comment to hint ppl when they would need this.
@nicolas-grekas OK, thanks, included everything :-) |
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.
LGTM, just one note (and a rebase is needed)
@nicolas-grekas Identical copy of symfony#14499
Closing in favor of #16141, since I don't know how to rebase ;-) |
I started from scratch after #14174
Symfony\Component\HttpClient\Exception\ClientException
is thrown. Is this important? Or is the existingSymfony\\Contracts\\HttpClient\\Exception\\HttpExceptionInterface
enough?