Skip to content

[HttpClient] Fix handling thrown \Exception in \Generator in MockResponse #44438

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

fancyweb
Copy link
Contributor

@fancyweb fancyweb commented Dec 3, 2021

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

@carsonbot carsonbot added this to the 4.4 milestone Dec 3, 2021
@fancyweb fancyweb force-pushed the http-client/fix-mock-response-throw-exception branch from 810e8a6 to 6ba7548 Compare December 3, 2021 11:28
@fancyweb
Copy link
Contributor Author

fancyweb commented Dec 3, 2021

Failure is related, I need to investigate.

@fancyweb
Copy link
Contributor Author

fancyweb commented Dec 3, 2021

@nicolas-grekas The failure is related to the PHP version. It seems that the destructor of ErrorChunk is not triggered in the same way on 7.2 and above, I don't really know what should I do here. Keep the current behavior (direct throw) on 7.2? Modify the test so that it doesn't rely on the destructor? Something else?

@fancyweb fancyweb force-pushed the http-client/fix-mock-response-throw-exception branch 4 times, most recently from 3beaf09 to f2867f4 Compare December 11, 2021 10:34
@fancyweb fancyweb force-pushed the http-client/fix-mock-response-throw-exception branch from f2867f4 to c71265b Compare December 11, 2021 11:03
@@ -167,7 +210,7 @@ protected function getHttpClient(string $testCase): HttpClientInterface
case 'testResolve':
$responses[] = new MockResponse($body, ['response_headers' => $headers]);
$responses[] = new MockResponse($body, ['response_headers' => $headers]);
$responses[] = new MockResponse((function () { throw new \Exception('Fake connection timeout'); yield ''; })(), ['response_headers' => $headers]);
$responses[] = new MockResponse((function () { yield ''; })(), ['response_headers' => $headers]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to throw for the test to be effective. Yielding after throwing "breaks" PHP 7.2.

@nicolas-grekas
Copy link
Member

Thank you @fancyweb.

@nicolas-grekas nicolas-grekas merged commit 4b6e29d into symfony:4.4 Dec 11, 2021
@fancyweb fancyweb deleted the http-client/fix-mock-response-throw-exception branch December 11, 2021 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants