[HttpClient] Fix streaming and redirecting with NoPrivateNetworkHttpClient #59023
Conversation
nicolas-grekas
commented
Nov 28, 2024
| Q | A |
|---|---|
| Branch? | 5.4 |
| Bug fix? | yes |
| New feature? | no |
| Deprecations? | no |
| Issues | #59021 |
| License | MIT |
| $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)) { |
There was a problem hiding this comment.
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.
fix when 0 >= $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.
fix for the stream() method: the one provided by AsyncDecoratorTrait 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.
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.
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) |