Merge branch '5.4' into 6.4

* 5.4: [HttpClient] Various cleanups after recent changes do not add child nodes to EmptyNode instances consider write property visibility to decide whether a property is writable add comment explaining why HttpClient tests are run separately
symfony · Nov 25, 2024 · cbeefef · cbeefef
2 parents 13b1d5b + 4e9ca20
commit cbeefef
Show file tree

Hide file tree

Showing 9 changed files with 53 additions and 76 deletions.
diff --git a/CurlHttpClient.php b/CurlHttpClient.php
@@ -417,9 +417,8 @@ private static function createRedirectResolver(array $options, string $host, int
             }
         }
 
-        return static function ($ch, string $location, bool $noContent, bool &$locationHasHost) use (&$redirectHeaders, $options) {
+        return static function ($ch, string $location, bool $noContent) use (&$redirectHeaders, $options) {
             try {
-                $locationHasHost = false;
                 $location = self::parseUrl($location);
                 $url = self::parseUrl(curl_getinfo($ch, \CURLINFO_EFFECTIVE_URL));
                 $url = self::resolveUrl($location, $url);
@@ -433,9 +432,7 @@ private static function createRedirectResolver(array $options, string $host, int
                 $redirectHeaders['with_auth'] = array_filter($redirectHeaders['with_auth'], $filterContentHeaders);
             }
 
-            $locationHasHost = isset($location['authority']);
-
-            if ($redirectHeaders && $locationHasHost) {
+            if ($redirectHeaders && isset($location['authority'])) {
                 $port = parse_url($location['authority'], \PHP_URL_PORT) ?: ('http:' === $location['scheme'] ? 80 : 443);
                 $requestHeaders = parse_url($location['authority'], \PHP_URL_HOST) === $redirectHeaders['host']  && $redirectHeaders['port'] === $port ? $redirectHeaders['with_auth'] : $redirectHeaders['no_auth'];
                 curl_setopt($ch, \CURLOPT_HTTPHEADER, $requestHeaders);

diff --git a/NativeHttpClient.php b/NativeHttpClient.php
@@ -155,6 +155,7 @@ public function request(string $method, string $url, array $options = []): Respo
 
                 $progressInfo = $info;
                 $progressInfo['url'] = implode('', $info['url']);
+                $progressInfo['resolve'] = $resolve;
                 unset($progressInfo['size_body']);
 
                 // Memoize the last progress to ease calling the callback periodically when no network transfer happens
@@ -167,7 +168,7 @@ public function request(string $method, string $url, array $options = []): Respo
                     $lastProgress = $progress ?: $lastProgress;
                 }
 
-                $onProgress($lastProgress[0], $lastProgress[1], $progressInfo, $resolve);
+                $onProgress($lastProgress[0], $lastProgress[1], $progressInfo);
             };
         } elseif (0 < $options['max_duration']) {
             $maxDuration = $options['max_duration'];
@@ -352,6 +353,7 @@ private static function dnsResolve(string $host, NativeClientState $multi, array
 
             $multi->dnsCache[$host] = $ip = $ip[0];
             $info['debug'] .= "* Added {$host}:0:{$ip} to DNS cache\n";
+            $host = $ip;
         } else {
             $info['debug'] .= "* Hostname was found in DNS cache\n";
             $host = str_contains($ip, ':') ? "[$ip]" : $ip;

diff --git a/NoPrivateNetworkHttpClient.php b/NoPrivateNetworkHttpClient.php
@@ -56,24 +56,20 @@ public function request(string $method, string $url, array $options = []): Respo
 
         $subnets = $this->subnets;
 
-        $options['on_progress'] = static function (int $dlNow, int $dlSize, array $info, ?\Closure $resolve = null) use ($onProgress, $subnets): void {
+        $options['on_progress'] = static function (int $dlNow, int $dlSize, array $info) use ($onProgress, $subnets): void {
             static $lastUrl = '';
             static $lastPrimaryIp = '';
 
             if ($info['url'] !== $lastUrl) {
-                $host = trim(parse_url($info['url'], PHP_URL_HOST) ?: '', '[]');
-                $resolve ??= static fn () => null;
+                $host = parse_url($info['url'], PHP_URL_HOST) ?: '';
+                $resolve = $info['resolve'] ?? static function () { return null; };
 
-                if (($ip = $host)
-                    && !filter_var($ip, \FILTER_VALIDATE_IP, \FILTER_FLAG_IPV6)
-                    && !filter_var($ip, \FILTER_VALIDATE_IP, \FILTER_FLAG_IPV4)
-                    && !$ip = $resolve($host)
+                if (($ip = trim($host, '[]'))
+                    && !filter_var($ip, \FILTER_VALIDATE_IP)
+                    && !($ip = $resolve($host))
+                    && $ip = @(gethostbynamel($host)[0] ?? dns_get_record($host, \DNS_AAAA)[0]['ipv6'] ?? null)
                 ) {
-                    if ($ip = @(dns_get_record($host, \DNS_A)[0]['ip'] ?? null)) {
-                        $resolve($host, $ip);
-                    } elseif ($ip = @(dns_get_record($host, \DNS_AAAA)[0]['ipv6'] ?? null)) {
-                        $resolve($host, '['.$ip.']');
-                    }
+                    $resolve($host, $ip);
                 }
 
                 if ($ip && IpUtils::checkIp($ip, $subnets ?? IpUtils::PRIVATE_SUBNETS)) {

diff --git a/Response/AmpResponse.php b/Response/AmpResponse.php
@@ -98,7 +98,8 @@ public function __construct(AmpClientState $multi, Request $request, array $opti
         $onProgress = $options['on_progress'] ?? static function () {};
         $onProgress = $this->onProgress = static function () use (&$info, $onProgress, $resolve) {
             $info['total_time'] = microtime(true) - $info['start_time'];
-            $onProgress((int) $info['size_download'], ((int) (1 + $info['download_content_length']) ?: 1) - 1, (array) $info, $resolve);
+            $info['resolve'] = $resolve;
+            $onProgress((int) $info['size_download'], ((int) (1 + $info['download_content_length']) ?: 1) - 1, (array) $info);
         };
 
         $pauseDeferred = new Deferred();

diff --git a/Response/AsyncContext.php b/Response/AsyncContext.php
@@ -161,8 +161,8 @@ public function replaceRequest(string $method, string $url, array $options = [])
         $this->info['previous_info'][] = $info = $this->response->getInfo();
         if (null !== $onProgress = $options['on_progress'] ?? null) {
             $thisInfo = &$this->info;
-            $options['on_progress'] = static function (int $dlNow, int $dlSize, array $info, ?\Closure $resolve = null) use (&$thisInfo, $onProgress) {
-                $onProgress($dlNow, $dlSize, $thisInfo + $info, $resolve);
+            $options['on_progress'] = static function (int $dlNow, int $dlSize, array $info) use (&$thisInfo, $onProgress) {
+                $onProgress($dlNow, $dlSize, $thisInfo + $info);
             };
         }
         if (0 < ($info['max_duration'] ?? 0) && 0 < ($info['total_time'] ?? 0)) {

diff --git a/Response/AsyncResponse.php b/Response/AsyncResponse.php
@@ -52,8 +52,8 @@ public function __construct(HttpClientInterface $client, string $method, string
 
         if (null !== $onProgress = $options['on_progress'] ?? null) {
             $thisInfo = &$this->info;
-            $options['on_progress'] = static function (int $dlNow, int $dlSize, array $info, ?\Closure $resolve = null) use (&$thisInfo, $onProgress) {
-                $onProgress($dlNow, $dlSize, $thisInfo + $info, $resolve);
+            $options['on_progress'] = static function (int $dlNow, int $dlSize, array $info) use (&$thisInfo, $onProgress) {
+                $onProgress($dlNow, $dlSize, $thisInfo + $info);
             };
         }
         $this->response = $client->request($method, $url, ['buffer' => false] + $options);
@@ -118,11 +118,20 @@ public function getHeaders(bool $throw = true): array
 
     public function getInfo(?string $type = null): mixed
     {
+        if ('debug' === ($type ?? 'debug')) {
+            $debug = implode('', array_column($this->info['previous_info'] ?? [], 'debug'));
+            $debug .= $this->response->getInfo('debug');
+
+            if ('debug' === $type) {
+                return $debug;
+            }
+        }
+
         if (null !== $type) {
             return $this->info[$type] ?? $this->response->getInfo($type);
         }
 
-        return $this->info + $this->response->getInfo();
+        return array_merge($this->info + $this->response->getInfo(), ['debug' => $debug]);
     }
 
     /**
@@ -253,6 +262,7 @@ public static function stream(iterable $responses, ?float $timeout = null, ?stri
                 return;
             }
 
+            $chunk = null;
             foreach ($client->stream($wrappedResponses, $timeout) as $response => $chunk) {
                 $r = $asyncMap[$response];
 
@@ -295,6 +305,9 @@ public static function stream(iterable $responses, ?float $timeout = null, ?stri
                 }
             }
 
+            if (null === $chunk) {
+                throw new \LogicException(\sprintf('"%s" is not compliant with HttpClientInterface: its "stream()" method didn\'t yield any chunks when it should have.', get_debug_type($client)));
+            }
             if (null === $chunk->getError() && $chunk->isLast()) {
                 $r->yieldedState = self::LAST_CHUNK_YIELDED;
             }

diff --git a/Response/CurlResponse.php b/Response/CurlResponse.php
@@ -131,7 +131,7 @@ public function __construct(CurlClientState $multi, \CurlHandle|string $ch, ?arr
                 try {
                     rewind($debugBuffer);
                     $debug = ['debug' => stream_get_contents($debugBuffer)];
-                    $onProgress($dlNow, $dlSize, $url + curl_getinfo($ch) + $info + $debug, $resolve);
+                    $onProgress($dlNow, $dlSize, $url + curl_getinfo($ch) + $info + $debug + ['resolve' => $resolve]);
                 } catch (\Throwable $e) {
                     $multi->handlesActivity[(int) $ch][] = null;
                     $multi->handlesActivity[(int) $ch][] = $e;
@@ -421,21 +421,11 @@ private static function parseHeaderLine($ch, string $data, array &$info, array &
                 $info['http_method'] = 'HEAD' === $info['http_method'] ? 'HEAD' : 'GET';
                 curl_setopt($ch, \CURLOPT_CUSTOMREQUEST, $info['http_method']);
             }
-            $locationHasHost = false;
 
-            if (null === $info['redirect_url'] = $resolveRedirect($ch, $location, $noContent, $locationHasHost)) {
+            if (null === $info['redirect_url'] = $resolveRedirect($ch, $location, $noContent)) {
                 $options['max_redirects'] = curl_getinfo($ch, \CURLINFO_REDIRECT_COUNT);
                 curl_setopt($ch, \CURLOPT_FOLLOWLOCATION, false);
                 curl_setopt($ch, \CURLOPT_MAXREDIRS, $options['max_redirects']);
-            } elseif ($locationHasHost) {
-                $url = parse_url($info['redirect_url']);
-
-                if (null !== $ip = $multi->dnsCache->hostnames[$url['host'] = strtolower($url['host'])] ?? null) {
-                    // Populate DNS cache for redirects if needed
-                    $port = $url['port'] ?? ('http' === $url['scheme'] ? 80 : 443);
-                    curl_setopt($ch, \CURLOPT_RESOLVE, ["{$url['host']}:$port:$ip"]);
-                    $multi->dnsCache->removals["-{$url['host']}:$port"] = "-{$url['host']}:$port";
-                }
             }
         }
 

diff --git a/Tests/NoPrivateNetworkHttpClientTest.php b/Tests/NoPrivateNetworkHttpClientTest.php
@@ -75,7 +75,7 @@ public function testExcludeByIp(string $ipAddr, $subnets, bool $mustThrow)
             $this->expectExceptionMessage(sprintf('IP "%s" is blocked for "%s".', $ipAddr, $url));
         }
 
-        $previousHttpClient = $this->getHttpClientMock($url, $ipAddr, $content);
+        $previousHttpClient = $this->getMockHttpClient($ipAddr, $content);
         $client = new NoPrivateNetworkHttpClient($previousHttpClient, $subnets);
         $response = $client->request('GET', $url);
 
@@ -91,14 +91,15 @@ public function testExcludeByIp(string $ipAddr, $subnets, bool $mustThrow)
     public function testExcludeByHost(string $ipAddr, $subnets, bool $mustThrow)
     {
         $content = 'foo';
-        $url = sprintf('http://%s/', str_contains($ipAddr, ':') ? sprintf('[%s]', $ipAddr) : $ipAddr);
+        $host = str_contains($ipAddr, ':') ? sprintf('[%s]', $ipAddr) : $ipAddr;
+        $url = sprintf('http://%s/', $host);
 
         if ($mustThrow) {
             $this->expectException(TransportException::class);
-            $this->expectExceptionMessage(sprintf('Host "%s" is blocked for "%s".', $ipAddr, $url));
+            $this->expectExceptionMessage(sprintf('Host "%s" is blocked for "%s".', $host, $url));
         }
 
-        $previousHttpClient = $this->getHttpClientMock($url, $ipAddr, $content);
+        $previousHttpClient = $this->getMockHttpClient($ipAddr, $content);
         $client = new NoPrivateNetworkHttpClient($previousHttpClient, $subnets);
         $response = $client->request('GET', $url);
 
@@ -119,7 +120,7 @@ public function testCustomOnProgressCallback()
             ++$executionCount;
         };
 
-        $previousHttpClient = $this->getHttpClientMock($url, $ipAddr, $content);
+        $previousHttpClient = $this->getMockHttpClient($ipAddr, $content);
         $client = new NoPrivateNetworkHttpClient($previousHttpClient);
         $response = $client->request('GET', $url, ['on_progress' => $customCallback]);
 
@@ -132,7 +133,6 @@ public function testNonCallableOnProgressCallback()
     {
         $ipAddr = '104.26.14.6';
         $url = sprintf('http://%s/', $ipAddr);
-        $content = 'bar';
         $customCallback = sprintf('cb_%s', microtime(true));
 
         $this->expectException(InvalidArgumentException::class);
@@ -142,38 +142,16 @@ public function testNonCallableOnProgressCallback()
         $client->request('GET', $url, ['on_progress' => $customCallback]);
     }
 
-    private function getHttpClientMock(string $url, string $ipAddr, string $content)
+    public function testConstructor()
     {
-        $previousHttpClient = $this
-            ->getMockBuilder(HttpClientInterface::class)
-            ->getMock();
-
-        $previousHttpClient
-            ->expects($this->once())
-            ->method('request')
-            ->with(
-                'GET',
-                $url,
-                $this->callback(function ($options) {
-                    $this->assertArrayHasKey('on_progress', $options);
-                    $onProgress = $options['on_progress'];
-                    $this->assertIsCallable($onProgress);
-
-                    return true;
-                })
-            )
-            ->willReturnCallback(function ($method, $url, $options) use ($ipAddr, $content): ResponseInterface {
-                $info = [
-                    'primary_ip' => $ipAddr,
-                    'url' => $url,
-                ];
-
-                $onProgress = $options['on_progress'];
-                $onProgress(0, 0, $info);
-
-                return MockResponse::fromRequest($method, $url, [], new MockResponse($content));
-            });
-
-        return $previousHttpClient;
+        $this->expectException(\TypeError::class);
+        $this->expectExceptionMessage('Argument 2 passed to "Symfony\Component\HttpClient\NoPrivateNetworkHttpClient::__construct()" must be of the type array, string or null. "int" given.');
+
+        new NoPrivateNetworkHttpClient(new MockHttpClient(), 3);
+    }
+
+    private function getMockHttpClient(string $ipAddr, string $content)
+    {
+        return new MockHttpClient(new MockResponse($content, ['primary_ip' => $ipAddr]));
     }
 }
diff --git a/TraceableHttpClient.php b/TraceableHttpClient.php
@@ -55,11 +55,11 @@ public function request(string $method, string $url, array $options = []): Respo
             $content = false;
         }
 
-        $options['on_progress'] = function (int $dlNow, int $dlSize, array $info, ?\Closure $resolve = null) use (&$traceInfo, $onProgress) {
+        $options['on_progress'] = function (int $dlNow, int $dlSize, array $info) use (&$traceInfo, $onProgress) {
             $traceInfo = $info;
 
             if (null !== $onProgress) {
-                $onProgress($dlNow, $dlSize, $info, $resolve);
+                $onProgress($dlNow, $dlSize, $info);
             }
         };