Skip to content

Commit d049cab

Browse files
CopilotThavarshan
andcommitted
Fix code review issues: separate curl multi options, fix URI in exception, remove error_log
Co-authored-by: Thavarshan <10804999+Thavarshan@users.noreply.github.com>
1 parent 94ae7e2 commit d049cab

4 files changed

Lines changed: 33 additions & 9 deletions

File tree

src/Fetch/Pool/DnsCache.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,7 @@ protected function performDnsLookup(string $hostname): array
179179
if (empty($addresses)) {
180180
throw new NetworkException(
181181
"Failed to resolve hostname: {$hostname}",
182-
new \GuzzleHttp\Psr7\Request('GET', $hostname)
182+
new \GuzzleHttp\Psr7\Request('GET', "https://{$hostname}/")
183183
);
184184
}
185185

src/Fetch/Pool/HostConnectionPool.php

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -169,9 +169,8 @@ protected function warmupConnections(): void
169169
$connection = $this->createConnection();
170170
$connection->decrementActiveRequests(); // Was incremented in createConnection
171171
$this->availableConnections->enqueue($connection);
172-
} catch (\Throwable $e) {
173-
// Log warmup failure but continue
174-
error_log('Connection warmup failed: '.$e->getMessage());
172+
} catch (\Throwable) {
173+
// Warmup failure is not critical - stop warming up but continue
175174
break;
176175
}
177176
}

src/Fetch/Pool/Http2Configuration.php

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -119,13 +119,26 @@ public function getCurlOptions(): array
119119
$options = [];
120120

121121
if ($this->enabled) {
122-
// Use HTTP/2
122+
// Use HTTP/2 with automatic fallback
123123
$options[CURLOPT_HTTP_VERSION] = CURL_HTTP_VERSION_2_0;
124+
}
125+
126+
return $options;
127+
}
128+
129+
/**
130+
* Get cURL multi options for HTTP/2 multiplexing.
131+
*
132+
* These options should be used with curl_multi_setopt().
133+
*
134+
* @return array<int, mixed>
135+
*/
136+
public function getCurlMultiOptions(): array
137+
{
138+
$options = [];
124139

125-
// Enable multiplexing
126-
if (defined('CURLPIPE_MULTIPLEX')) {
127-
$options[CURLMOPT_PIPELINING] = CURLPIPE_MULTIPLEX;
128-
}
140+
if ($this->enabled && defined('CURLPIPE_MULTIPLEX')) {
141+
$options[CURLMOPT_PIPELINING] = CURLPIPE_MULTIPLEX;
129142
}
130143

131144
return $options;

tests/Unit/ConnectionPoolTest.php

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -295,6 +295,18 @@ public function test_http2_curl_options(): void
295295
$this->assertEquals(CURL_HTTP_VERSION_2_0, $curlOptions[CURLOPT_HTTP_VERSION]);
296296
}
297297

298+
public function test_http2_curl_multi_options(): void
299+
{
300+
$config = new Http2Configuration(enabled: true);
301+
$curlMultiOptions = $config->getCurlMultiOptions();
302+
303+
// CURLMOPT_PIPELINING should be in multi options, not regular curl options
304+
if (defined('CURLPIPE_MULTIPLEX')) {
305+
$this->assertArrayHasKey(CURLMOPT_PIPELINING, $curlMultiOptions);
306+
$this->assertEquals(CURLPIPE_MULTIPLEX, $curlMultiOptions[CURLMOPT_PIPELINING]);
307+
}
308+
}
309+
298310
public function test_http2_disabled_curl_options(): void
299311
{
300312
$config = new Http2Configuration(enabled: false);

0 commit comments

Comments
 (0)