[HttpClient] fix handling of 3xx with no Location header - ignore Content-Length when no body is expected

nicolas-grekas · nicolas-grekas · commit 50a88c59f65c · 2019-10-28T16:45:56.000+01:00
diff --git a/src/Symfony/Component/HttpClient/Response/CurlResponse.php b/src/Symfony/Component/HttpClient/Response/CurlResponse.php
@@ -323,6 +323,7 @@ private static function parseHeaderLine($ch, string $data, array &$info, array &
 
         if (200 > $statusCode = curl_getinfo($ch, CURLINFO_RESPONSE_CODE)) {
             $multi->handlesActivity[$id][] = new InformationalChunk($statusCode, $headers);
+            $location = null;
 
             return \strlen($data);
         }
@@ -346,9 +347,7 @@ private static function parseHeaderLine($ch, string $data, array &$info, array &
             }
         }
 
-        $location = null;
-
-        if ($statusCode < 300 || 400 <= $statusCode || curl_getinfo($ch, CURLINFO_REDIRECT_COUNT) === $options['max_redirects']) {
+        if ($statusCode < 300 || 400 <= $statusCode || null === $location || curl_getinfo($ch, CURLINFO_REDIRECT_COUNT) === $options['max_redirects']) {
             // Headers and redirects completed, time to get the response's body
             $multi->handlesActivity[$id][] = new FirstChunk();
 
@@ -361,6 +360,8 @@ private static function parseHeaderLine($ch, string $data, array &$info, array &
             $logger->info(sprintf('Redirecting: "%s %s"', $info['http_code'], $info['redirect_url']));
         }
 
+        $location = null;
+
         return \strlen($data);
     }
 }
diff --git a/src/Symfony/Component/HttpClient/Response/MockResponse.php b/src/Symfony/Component/HttpClient/Response/MockResponse.php
@@ -257,7 +257,7 @@ private static function readResponse(self $response, array $options, ResponseInt
         $info = $mock->getInfo() ?: [];
         $response->info['http_code'] = ($info['http_code'] ?? 0) ?: $mock->getStatusCode() ?: 200;
         $response->addResponseHeaders($info['response_headers'] ?? [], $response->info, $response->headers);
-        $dlSize = isset($response->headers['content-encoding']) ? 0 : (int) ($response->headers['content-length'][0] ?? 0);
+        $dlSize = isset($response->headers['content-encoding']) || 'HEAD' === $response->info['http_method'] || \in_array($response->info['http_code'], [204, 304], true) ? 0 : (int) ($response->headers['content-length'][0] ?? 0);
 
         $response->info = [
             'start_time' => $response->info['start_time'],
diff --git a/src/Symfony/Component/HttpClient/Response/NativeResponse.php b/src/Symfony/Component/HttpClient/Response/NativeResponse.php
@@ -176,7 +176,7 @@ private function open(): void
 
         $this->multi->handlesActivity[$this->id] = [new FirstChunk()];
 
-        if ('HEAD' === $context['http']['method']) {
+        if ('HEAD' === $context['http']['method'] || \in_array($this->info['http_code'], [204, 304], true)) {
             $this->multi->handlesActivity[$this->id][] = null;
             $this->multi->handlesActivity[$this->id][] = null;
 
diff --git a/src/Symfony/Contracts/HttpClient/Test/Fixtures/web/index.php b/src/Symfony/Contracts/HttpClient/Test/Fixtures/web/index.php
@@ -82,6 +82,11 @@
         header('Location: ..', true, 302);
         break;
 
+    case '/304':
+        header('Content-Length: 10', true, 304);
+        echo '12345';
+        return;
+
     case '/307':
         header('Location: http://localhost:8057/post', true, 307);
         break;
diff --git a/src/Symfony/Contracts/HttpClient/Test/HttpClientTestCase.php b/src/Symfony/Contracts/HttpClient/Test/HttpClientTestCase.php
@@ -260,6 +260,17 @@ public function testBadRequestBody()
         $response->getStatusCode();
     }
 
+    public function test304()
+    {
+        $client = $this->getHttpClient(__FUNCTION__);
+        $response = $client->request('GET', 'http://localhost:8057/304', [
+            'headers' => ['If-Match' => '"abc"'],
+        ]);
+
+        $this->assertSame(304, $response->getStatusCode());
+        $this->assertSame('', $response->getContent(false));
+    }
+
     public function testRedirects()
     {
         $client = $this->getHttpClient(__FUNCTION__);

Original file line number	Diff line number	Diff line change
`@@ -323,6 +323,7 @@ private static function parseHeaderLine($ch, string $data, array &$info, array &`
`323`	`323`
`324`	`324`	`if (200 > $statusCode = curl_getinfo($ch, CURLINFO_RESPONSE_CODE)) {`
`325`	`325`	`$multi->handlesActivity[$id][] = new InformationalChunk($statusCode, $headers);`
	`326`	`+ $location = null;`
`326`	`327`
`327`	`328`	`return \strlen($data);`
`328`	`329`	`}`
`@@ -346,9 +347,7 @@ private static function parseHeaderLine($ch, string $data, array &$info, array &`
`346`	`347`	`}`
`347`	`348`	`}`
`348`	`349`
`349`		`- $location = null;`
`350`		`-`
`351`		`- if ($statusCode < 300 \|\| 400 <= $statusCode \|\| curl_getinfo($ch, CURLINFO_REDIRECT_COUNT) === $options['max_redirects']) {`
	`350`	`+ if ($statusCode < 300 \|\| 400 <= $statusCode \|\| null === $location \|\| curl_getinfo($ch, CURLINFO_REDIRECT_COUNT) === $options['max_redirects']) {`
`352`	`351`	`// Headers and redirects completed, time to get the response's body`
`353`	`352`	`$multi->handlesActivity[$id][] = new FirstChunk();`
`354`	`353`
`@@ -361,6 +360,8 @@ private static function parseHeaderLine($ch, string $data, array &$info, array &`
`361`	`360`	`$logger->info(sprintf('Redirecting: "%s %s"', $info['http_code'], $info['redirect_url']));`
`362`	`361`	`}`
`363`	`362`
	`363`	`+ $location = null;`
	`364`	`+`
`364`	`365`	`return \strlen($data);`
`365`	`366`	`}`
`366`	`367`	`}`