Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[HttpClient] Fix streaming and redirecting with NoPrivateNetworkHttpClient#59023
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas commentedNov 28, 2024
| Q | A |
|---|---|
| Branch? | 5.4 |
| Bug fix? | yes |
| New feature? | no |
| Deprecations? | no |
| Issues | #59021 |
| License | MIT |
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.
With test cases to cover the regressions found on the happy path (aka no matches for the provided$subnets)
| $onProgress = $options['on_progress'] ?? null; | ||
| $subnets = $this->subnets; | ||
| $ipFlags = $this->ipFlags; | ||
| $lastPrimaryIp = ''; | ||
| $options['on_progress'] = static function (int $dlNow, int $dlSize, array $info) use ($onProgress, $subnets, $ipFlags, &$lastPrimaryIp): void { | ||
| if (($info['primary_ip'] ?? '') !== $lastPrimaryIp) { | ||
| if (!\in_array($info['primary_ip'] ?? '', ['', $lastPrimaryIp], true)) { |
nicolas-grekasNov 28, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
fix for the empty ip address error (it didn't happen before, but now that we redirect on our own, it can)
| self::ipCheck($info['primary_ip'], $subnets, $ipFlags, null, $info['url']); | ||
| $lastPrimaryIp = $info['primary_ip']; | ||
| } | ||
| null !== $onProgress && $onProgress($dlNow, $dlSize, $info); | ||
| }; | ||
| if (0 >= $maxRedirects = $options['max_redirects']) { | ||
| return new AsyncResponse($this->client, $method, $url, $options); |
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.
fix when0 >= $maxRedirects: the passthru callback shouldn't be used in this case
| */ | ||
| public function stream($responses, ?float $timeout = null): ResponseStreamInterface | ||
| { | ||
| return $this->client->stream($responses, $timeout); |
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.
fix for thestream() method: the one provided byAsyncDecoratorTrait is fine instead (I forgot to remove this one when adding the trait)
| @@ -24,11 +24,6 @@ public static function setUpBeforeClass(): void | |||
| TestHttpServer::start(); | |||
| } | |||
| public static function tearDownAfterClass(): void | |||
| { | |||
| TestHttpServer::stop(); | |||
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.
one test server instance can handle all test cases, no need to start/stop it all the time
| @@ -36,7 +36,6 @@ public static function tearDownAfterClass(): void | |||
| { | |||
| TestHttpServer::stop(8067); | |||
| TestHttpServer::stop(8077); | |||
| TestHttpServer::stop(8087); | |||
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.
no need to stop this port, it can handle many test cases
(8067 and 8077 are special because they test situations where the webserver hangs)
(Failures on deps=high are expected: this job checks out 4.4 and runs it with recent test cases) |
7a2d66a intosymfony:5.4Uh oh!
There was an error while loading.Please reload this page.