Skip to content

bisect: advertise gzip,deflate encoding when supported#5434

Open
staabm wants to merge 7 commits intophpstan:2.1.xfrom
staabm:bisect-gzip
Open

bisect: advertise gzip,deflate encoding when supported#5434
staabm wants to merge 7 commits intophpstan:2.1.xfrom
staabm:bisect-gzip

Conversation

@staabm
Copy link
Copy Markdown
Contributor

@staabm staabm commented Apr 9, 2026

time to fetch the commits..

... before this PR

➜  phpstan-src git:(bisect-gzip) ✗ time php bin/phpstan bisect foo.php  --good=2.1.0 --bad=2.1.46

Fetching commits between 2.1.0 and 2.1.46...
--------------------------------------------

php bin/phpstan bisect foo.php --good=2.1.0 --bad=2.1.46  0.27s user 0.17s system 0% cpu 54.959 total

... after this PR

➜  phpstan-src git:(bisect-gzip) ✗ time php bin/phpstan bisect foo.php  --good=2.1.0 --bad=2.1.46

Fetching commits between 2.1.0 and 2.1.46...
--------------------------------------------

php bin/phpstan bisect foo.php --good=2.1.0 --bad=2.1.46  0.22s user 0.08s system 0% cpu 44.516 total

-> saves ~10 seconds when downloading big responses

@staabm staabm requested a review from ondrejmirtes April 9, 2026 08:05
return 1;
}

$headers = [
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we might instead create a PHPStan namespaced HttpClient and re-use the gzip support in all places we currently directly use GuzzleHttp\Client

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a namespaces child class, just a ClientFactory

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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\HttpClientFactory to create a Guzzle client and default Accept-Encoding: gzip,deflate when zlib is available.
  • Updated BisectCommand to create its HTTP client via the new factory.
  • Updated FixerApplication to create its HTTP client via the new factory (and removed the direct Client import).

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.

Comment on lines +11 to +16
/**
* @param array<mixed> $config
*
* @see \GuzzleHttp\RequestOptions
*/
public static function createClient(array $config): Client
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +18 to +21
if (
!isset($config['headers']['Accept-Encoding'])
&& extension_loaded('zlib')
) {
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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'] = [];
}

Copilot uses AI. Check for mistakes.
{
if (
!isset($config['headers']['Accept-Encoding'])
&& extension_loaded('zlib')
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
&& extension_loaded('zlib')
&& extension_loaded('zlib')
&& (
!\array_key_exists(\GuzzleHttp\RequestOptions::DECODE_CONTENT, $config)
|| $config[\GuzzleHttp\RequestOptions::DECODE_CONTENT] !== false
)

Copilot uses AI. Check for mistakes.
*
* @see \GuzzleHttp\RequestOptions
*/
public static function createClient(array $config): Client
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be a service, not a static function. So we can pass options from config parameters

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants