chore: update to phpstan major version 2 #255
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #255 +/- ##
============================================
- Coverage 94.21% 94.19% -0.02%
+ Complexity 261 260 -1
============================================
Files 15 15
Lines 882 879 -3
============================================
- Hits 831 828 -3
Misses 51 51 ☔ View full report in Codecov by Sentry. |
| $contentLength = $this->getHeader('Content-Length'); | ||
| if (null !== $contentLength && (is_int($contentLength) || ctype_digit($contentLength))) { | ||
| if (null !== $contentLength && ctype_digit($contentLength)) { |
There was a problem hiding this comment.
getHeader can only return string or null. So is_int() is useless code.
| $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'); |
There was a problem hiding this comment.
preg_match returns (int) 1 not (bool) true.
| 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')); |
There was a problem hiding this comment.
preg_match returns (int) 1 not (bool) true.
| * @backupGlobals | ||
| * @backupGlobals enabled |
There was a problem hiding this comment.
backupGlobals should explicitly have the keyword "enabled"
There was a problem hiding this comment.
Did this annotation work as expected in phpunit without the suffix? If not we might just drop it
| @@ -14,10 +14,6 @@ parameters: | |||
| message: "#^Left side of || is always false.$#" | |||
| count: 6 | |||
| path: lib/Client.php | |||
| - | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I would ignore CurlHandle, resource given and such for now
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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
Issue #254
After this is reviewed and merged, I will look at doing similar in the 6.0 branch also.