Skip to content

chore: update to phpstan major version 2 #255

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
phil-davis wants to merge 3 commits into
base: master
Choose a base branch
from
Open

chore: update to phpstan major version 2 #255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aab45d0
chore: update to phpstan major version 2
phil-davis Jan 6, 2025
9634af2
chore: remove phpVersion from phpstan.neon
phil-davis Jan 6, 2025
8f32c6a
chore: update ignoreErrrors count
phil-davis Jan 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@
},
"require-dev" : {
"friendsofphp/php-cs-fixer": "^3.66",
"phpstan/phpstan": "^1.12",
"phpstan/phpstan-phpunit": "^1.4",
"phpstan/phpstan-strict-rules": "^1.6",
"phpstan/phpstan": "^2.1",
"phpstan/phpstan-phpunit": "^2.0",
"phpstan/phpstan-strict-rules": "^2.0",
"phpstan/extension-installer": "^1.4",
"phpunit/phpunit" : "^9.6"
},
Expand Down
5 changes: 1 addition & 4 deletions lib/Message.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,11 +82,8 @@ public function getBodyAsString(): string

return ob_get_clean();
}
/**
* @var string|int|null $contentLength
*/
$contentLength = $this->getHeader('Content-Length');
if (null !== $contentLength && (is_int($contentLength) || ctype_digit($contentLength))) {
if (null !== $contentLength && ctype_digit($contentLength)) {
Comment on lines 85 to +86
Copy link
Contributor Author

@phil-davis phil-davis Jan 6, 2025

Choose a reason for hiding this comment

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

getHeader can only return string or null. So is_int() is useless code.

return stream_get_contents($body, (int) $contentLength);
}

Expand Down
2 changes: 1 addition & 1 deletion lib/Sapi.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public static function sendResponse(ResponseInterface $response): void
$contentLength = $response->getHeader('Content-Length');
if (null !== $contentLength) {
$output = fopen('php://output', 'wb');
if (is_resource($body) && 'stream' == get_resource_type($body)) {
if (is_resource($body) && 'stream' === get_resource_type($body)) {
// a workaround to make PHP more possible to use mmap based copy, see https://github.com/sabre-io/http/pull/119
$left = (int) $contentLength;
// copy with 4MiB chunks
Expand Down
7 changes: 1 addition & 6 deletions phpstan.neon
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
parameters:
level: 6
phpVersion: 70430 # PHP 7.4.30
ignoreErrors:
-
message: "#^Negated boolean expression is always true.$#"
Expand All @@ -12,12 +11,8 @@ parameters:
path: lib/Client.php
-
message: "#^Left side of || is always false.$#"
count: 6
count: 19
path: lib/Client.php
-
Copy link
Member

@staabm staabm Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think we should drop the phpVersion from this config file. Since phpstan 2.x it will lookup the composer.json

see https://staabm.github.io/2024/11/14/phpstan-php-version-narrowing.html

Copy link
Contributor Author

@phil-davis phil-davis Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think we should drop the phpVersion from this config file. Since phpstan 2.x it will lookup the composer.json

That made it fail on PHP 8.4 https://github.com/sabre-io/http/actions/runs/12631715455/job/35193980626?pr=255

I suppose that I should have a look at the reported things?

Error: Ignored error pattern #^Left side of || is always false.$# in path /home/runner/work/http/http/lib/Client.php is expected to occur 6 times, but occurred 19 times.
Error: Parameter #1 $multi_handle of function curl_multi_select expects CurlMultiHandle, resource|null given.
Error: Method Sabre\HTTP\Client::addCurlSetting() has parameter $value with no type specified.
Error: Property Sabre\HTTP\Client::$curlHandle (resource|null) does not accept (CurlHandle|false).
Error: Parameter #1 $handle of function curl_reset expects CurlHandle, resource given.
Error: Property Sabre\HTTP\Client::$curlHandle (resource|null) is never assigned resource so it can be removed from the property type.
Error: Property Sabre\HTTP\Client::$curlMultiHandle (resource|null) is never assigned resource so it can be removed from the property type.
Error: Call to function array_key_exists() with 'size' and array{0: int, 1: int, 2: int, 3: int, 4: int, 5: int, 6: int, 7: int, ...} will always evaluate to true.
Error: Strict comparison using === between false and string will always evaluate to false.
Error: Property Sabre\HTTP\Client::$curlMultiHandle (resource|null) does not accept CurlMultiHandle.
Error: Parameter #1 $handle of function curl_exec expects CurlHandle, resource given.
Error: Parameter #1 $handle of function curl_getinfo expects CurlHandle, resource given.
Error: Parameter #1 $handle of function curl_errno expects CurlHandle, resource given.
Error: Parameter #1 $handle of function curl_error expects CurlHandle, resource given.

Copy link
Member

@staabm staabm Jan 6, 2025

Choose a reason for hiding this comment

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

I would ignore CurlHandle, resource given and such for now

Copy link
Contributor Author

@phil-davis phil-davis Jan 6, 2025

Choose a reason for hiding this comment

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

I changed the count for Left side of || is always false to 19.
Somehow that also makes all the other Errors go away ???

But it causes a fail running on PHP 7.4, because the count of that error is only 6 when phpstan runs on PHP 7.4

Is there a way to specify a different count for each PHP version?
Or just run phpstan on the latest PHP 8.4?
Or remove the count from the entry in phpstan.neon ?

Copy link
Member

@staabm staabm Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

by php-version errors are usually separated into separate baselines like in
https://github.com/phpstan/phpstan-src/blob/2f712479fe1aa86f618a49fe4f36a3531230484b/build/phpstan.neon#L10

if this sounds like too much work, we can revert to adding the lowest supported php version into phpstan.neon and wait until I am able to improve the phpstan multi phpversion story with https://staabm.github.io/2024/11/28/phpstan-php-version-in-scope.html

message: "#^Else branch is unreachable because ternary operator condition is always true.$#"
count: 1
path: lib/Auth/Digest.php
-
message: "#^Strict comparison using !== between '' and non-empty-string will always evaluate to true.$#"
count: 1
Expand Down
2 changes: 1 addition & 1 deletion tests/HTTP/Auth/AWSTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ public function test401(): void
{
$this->auth->requireLogin();
$test = preg_match('/^AWS$/', $this->response->getHeader('WWW-Authenticate'), $matches);
self::assertTrue(true == $test, 'The WWW-Authenticate response didn\'t match our pattern');
self::assertTrue(1 === $test, 'The WWW-Authenticate response didn\'t match our pattern');
Comment on lines 202 to +203
Copy link
Contributor Author

@phil-davis phil-davis Jan 6, 2025

Choose a reason for hiding this comment

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

preg_match returns (int) 1 not (bool) true.

}

/**
Expand Down
2 changes: 1 addition & 1 deletion tests/HTTP/Auth/DigestTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ private function getServerTokens(int $qop = Digest::QOP_AUTH): array
$test = preg_match('/Digest realm="'.self::REALM.'",qop="'.$qopstr.'",nonce="([0-9a-f]*)",opaque="([0-9a-f]*)"/',
$this->response->getHeader('WWW-Authenticate'), $matches);

self::assertTrue(true == $test, 'The WWW-Authenticate response didn\'t match our pattern. We received: '.$this->response->getHeader('WWW-Authenticate'));
self::assertTrue(1 === $test, 'The WWW-Authenticate response didn\'t match our pattern. We received: '.$this->response->getHeader('WWW-Authenticate'));
Copy link
Contributor Author

@phil-davis phil-davis Jan 6, 2025

Choose a reason for hiding this comment

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

preg_match returns (int) 1 not (bool) true.


$nonce = $matches[1];
$opaque = $matches[2];
Expand Down
6 changes: 3 additions & 3 deletions tests/HTTP/ClientTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,7 +537,7 @@ public function doRequest(RequestInterface $request): ResponseInterface
// If nothing modified $response, we're using the default behavior.
if (is_null($response)) {
return parent::doRequest($request);
} else { /* @phpstan-ignore-line phpstan thinks Else branch is unreachable */
} else {
return $response;
}
}
Expand All @@ -557,7 +557,7 @@ protected function curlStuff($curlHandle): array
// If nothing modified $return, we're using the default behavior.
if (is_null($return)) {
return parent::curlStuff($curlHandle);
} else { /* @phpstan-ignore-line phpstan thinks Else branch is unreachable */
} else {
return $return;
}
}
Expand All @@ -577,7 +577,7 @@ protected function curlExec($curlHandle): string
// If nothing modified $return, we're using the default behavior.
if (is_null($return)) {
return parent::curlExec($curlHandle);
} else { /* @phpstan-ignore-line phpstan thinks Else branch is unreachable */
} else {
return $return;
}
}
Expand Down
2 changes: 1 addition & 1 deletion tests/HTTP/RequestTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public function testGetQueryParametersNoData(): void
}

/**
* @backupGlobals
* @backupGlobals enabled
Copy link
Contributor Author

@phil-davis phil-davis Jan 6, 2025

Choose a reason for hiding this comment

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

backupGlobals should explicitly have the keyword "enabled"

Copy link
Member

@staabm staabm Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Did this annotation work as expected in phpunit without the suffix? If not we might just drop it

*/
public function testCreateFromPHPRequest(): void
{
Expand Down
Loading