bisect: advertise gzip,deflate encoding when supported#5434
bisect: advertise gzip,deflate encoding when supported#5434staabm wants to merge 7 commits intophpstan:2.1.xfrom
Conversation
src/Command/BisectCommand.php
Outdated
| return 1; | ||
| } | ||
|
|
||
| $headers = [ |
There was a problem hiding this comment.
we might instead create a PHPStan namespaced HttpClient and re-use the gzip support in all places we currently directly use GuzzleHttp\Client
There was a problem hiding this comment.
Not a namespaces child class, just a ClientFactory
There was a problem hiding this comment.
Pull request overview
This PR improves download performance for bisect (and other HTTP fetches that reuse the same client setup) by advertising gzip,deflate support to allow servers to return compressed responses when PHP can decode them.
Changes:
- Added
PHPStan\Internal\HttpClientFactoryto create a Guzzle client and defaultAccept-Encoding: gzip,deflatewhenzlibis available. - Updated
BisectCommandto create its HTTP client via the new factory. - Updated
FixerApplicationto create its HTTP client via the new factory (and removed the directClientimport).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/Internal/HttpClientFactory.php |
New factory that sets default Accept-Encoding and instantiates GuzzleHttp\Client. |
src/Command/FixerApplication.php |
Switches to using HttpClientFactory for download checks and removes direct client construction. |
src/Command/BisectCommand.php |
Switches to using HttpClientFactory for GitHub API requests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * @param array<mixed> $config | ||
| * | ||
| * @see \GuzzleHttp\RequestOptions | ||
| */ | ||
| public static function createClient(array $config): Client |
There was a problem hiding this comment.
PHPDoc type array<mixed> implies an int-keyed list; this config is an associative options array (RequestOptions keys). Consider changing to array<string, mixed> (or a more specific shape) so static analysis keeps key type information.
| if ( | ||
| !isset($config['headers']['Accept-Encoding']) | ||
| && extension_loaded('zlib') | ||
| ) { |
There was a problem hiding this comment.
isset($config['headers']['Accept-Encoding']) assumes headers is an array and only checks one exact casing. If a caller passes headers as a non-array (or uses a different header key casing), this can throw or result in duplicate Accept-Encoding values. Consider validating headers is an array and performing a case-insensitive presence check before setting the default.
| if ( | |
| !isset($config['headers']['Accept-Encoding']) | |
| && extension_loaded('zlib') | |
| ) { | |
| $hasAcceptEncodingHeader = false; | |
| if (isset($config['headers']) && is_array($config['headers'])) { | |
| foreach (array_keys($config['headers']) as $headerName) { | |
| if (is_string($headerName) && strcasecmp($headerName, 'Accept-Encoding') === 0) { | |
| $hasAcceptEncodingHeader = true; | |
| break; | |
| } | |
| } | |
| } | |
| if ( | |
| !$hasAcceptEncodingHeader | |
| && extension_loaded('zlib') | |
| ) { | |
| if (!isset($config['headers']) || !is_array($config['headers'])) { | |
| $config['headers'] = []; | |
| } |
| { | ||
| if ( | ||
| !isset($config['headers']['Accept-Encoding']) | ||
| && extension_loaded('zlib') |
There was a problem hiding this comment.
This factory forces Accept-Encoding whenever zlib is available, even if the caller explicitly disables decompression via RequestOptions::DECODE_CONTENT => false. In that case the response may be compressed but not decoded. Consider only advertising gzip/deflate when decode_content is not set to false (or when it’s true).
| && extension_loaded('zlib') | |
| && extension_loaded('zlib') | |
| && ( | |
| !\array_key_exists(\GuzzleHttp\RequestOptions::DECODE_CONTENT, $config) | |
| || $config[\GuzzleHttp\RequestOptions::DECODE_CONTENT] !== false | |
| ) |
| * | ||
| * @see \GuzzleHttp\RequestOptions | ||
| */ | ||
| public static function createClient(array $config): Client |
There was a problem hiding this comment.
Should be a service, not a static function. So we can pass options from config parameters
time to fetch the commits..
... before this PR
... after this PR
-> saves ~10 seconds when downloading big responses