Skip to content

[HttpClient] Add a canceled state to the ResponseInterface #34044

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 21, 2019
Merged

[HttpClient] Add a canceled state to the ResponseInterface #34044

merged 1 commit into from
Oct 21, 2019

Conversation

Toflar
Copy link
Contributor

@Toflar Toflar commented Oct 21, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets
License MIT
Doc PR none yet

As discussed in terminal42/escargot#7 with @nicolas-grekas.
However, I'm not quite sure about the tests here because it looks like the MockResponse always throws a TransportException when I cancel a response, even if I call $response->getContent(false). Is that a desired behaviour? :) Need some guidance here to finish that PR maybe.

Note: I've also sorted the info keys alphabetically and added their types because it feld incomplete and a bit random otherwise. Hope that's okay :)

@Toflar Toflar mentioned this pull request Oct 21, 2019
Merged
2 tasks
@nicolas-grekas nicolas-grekas added this to the next milestone Oct 21, 2019
@nicolas-grekas
Copy link
Member

Thanks!
Could you please add a check in HttpClientTestCase?

@Toflar
Copy link
Contributor Author

Toflar commented Oct 21, 2019

I tried but:

However, I'm not quite sure about the tests here because it looks like the MockResponse always throws a TransportException when I cancel a response, even if I call $response->getContent(false). Is that a desired behaviour? :) Need some guidance here to finish that PR maybe.

@nicolas-grekas
Copy link
Member

throws a TransportException when I cancel a response, even if I call $response->getContent(false)

Yes, that's desired behavior, for all clients that didn't get the content before canceling: false is only about HttpExceptionInterface (3/4/5xx), not transport errors.

But the check just needs to assert the result of ->getInfo('canceled'), so not sure why we care about the content?

@Toflar
Copy link
Contributor Author

Toflar commented Oct 21, 2019

Ah, I guess I was thinking way too complicated :) Test added.
I noticed that there are some case statements in MockHttpClientTest::getHttpClient() that don't seem to be used anymore. Want me to clean them up although not related to that PR?

nicolas-grekas
nicolas-grekas reviewed Oct 21, 2019
View reviewed changes
nicolas-grekas
nicolas-grekas reviewed Oct 21, 2019
View reviewed changes
@Toflar Toflar force-pushed the response-canceled-info branch from d237179 to 22e7123 Compare October 21, 2019 09:41
@Toflar
Copy link
Contributor Author

Toflar commented Oct 21, 2019

Appveyor failures look unrelated :)

@nicolas-grekas nicolas-grekas changed the title Add a canceled state to the ResponseInterface [HttpClient] Add a canceled state to the ResponseInterface Oct 21, 2019
@nicolas-grekas
Copy link
Member

Thank you @Toflar.

nicolas-grekas added a commit that referenced this pull request Oct 21, 2019
@nicolas-grekas
feature #34044 [HttpClient] Add a canceled state to the ResponseInter…
3f759d7 
…face (Toflar)

This PR was squashed before being merged into the 4.4 branch.

Discussion
----------

[HttpClient] Add a canceled state to the ResponseInterface

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       |
| License       | MIT
| Doc PR        | none yet

As discussed in terminal42/escargot#7 with @nicolas-grekas.
However, I'm not quite sure about the tests here because it looks like the `MockResponse` always throws a `TransportException` when I cancel a response, even if I call `$response->getContent(false)`. Is that a desired behaviour? :) Need some guidance here to finish that PR maybe.

Note: I've also sorted the info keys alphabetically and added their types because it feld incomplete and a bit random otherwise. Hope that's okay :)

Commits
-------

bbb8ed1 [HttpClient] Add a canceled state to the ResponseInterface
@nicolas-grekas nicolas-grekas merged commit bbb8ed1 into symfony:4.4 Oct 21, 2019
@nicolas-grekas nicolas-grekas mentioned this pull request Oct 21, 2019
Closed
@Toflar Toflar deleted the response-canceled-info branch October 21, 2019 15:54
@Toflar Toflar mentioned this pull request Oct 21, 2019
Merged
@nicolas-grekas nicolas-grekas mentioned this pull request Oct 21, 2019
Merged
nicolas-grekas added a commit that referenced this pull request Oct 22, 2019
@nicolas-grekas
minor #34052 [HttpClient] skip tests implemented in 4.4 (nicolas-grekas)
5da67f9 
This PR was merged into the 4.3 branch.

Discussion
----------

[HttpClient] skip tests implemented in 4.4

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Needed after #34051 and #34044

Commits
-------

ae86ab1 [HttpClient] skip tests implemented in 4.4
OskarStark added a commit to symfony/symfony-docs that referenced this pull request Oct 23, 2019
@OskarStark
feature #12526 Documented $response->getInfo('canceled') (Toflar)
c7ecd24 
This PR was squashed before being merged into the 4.4 branch (closes #12526).

Discussion
----------

Documented $response->getInfo('canceled')

Documented the new `canceled` information introduced in symfony/symfony#34044.

Reading the docs, I wonder if we need to catch any `Throwable` and also set `canceled` to `true` @nicolas-grekas?

Commits
-------

71cff04 Documented $response->getInfo('canceled')
@nicolas-grekas nicolas-grekas removed this from the next milestone Oct 27, 2019
@nicolas-grekas nicolas-grekas added this to the 4.4 milestone Oct 27, 2019
This was referenced Nov 12, 2019
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Feature HttpClient Status: Reviewed
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
3 participants
@Toflar @nicolas-grekas @carsonbot