From 21a5931efe60ffd1e3a263f5561610730a165029 Mon Sep 17 00:00:00 2001 From: Karl Waldman Date: Sun, 13 Sep 2026 14:48:32 -0400 Subject: [PATCH] chore: run PHPStan level 9 in CI and clear the 14 real errors (#24) Adds phpstan/phpstan as a dev dependency, a phpstan.neon at level 9 over src/ with no baseline, a "phpstan" composer script, and a static-analysis job in the test workflow. The 17 errors quoted in #24 are stale: PR #27 removed six Price.php casts and #26 rewrote the transport. Re-measured on 0672df0, level 9 on src/ reports 14. All 14 are fixed here, so the level ships clean: - dataOrFail()/priceOrFail() rebuild decoded JSON objects with string keys, making the array the SDK declares everywhere actually true (json_decode turns a numeric JSON key into an int array key). - originOf() in Client and CurlTransport reads parse_url() parts through is_string()/is_int() instead of casting mixed, so an unnameable part cannot compare equal to a real origin. - errorMessage() narrows $body['error'] and $body['data'] to arrays before reaching into them. - CurlTransport rejects an empty method or URL, which is also what curl_setopt_array's non-empty-string contract requires. - Removed two provably dead checks: the api-key null test the guard above it already proved, and the assert()/null seed on a loop that always runs. No baseline file and no @phpstan-ignore comments. phpstan.neon is export-ignored and excluded from the composer archive - it is dev tooling, not package content. The PHP constraint stays >=8.1: it is pinned by an existing assertion in PublicClaimsTest and narrowing it to ^8.1 is a packaging decision with its own release note, not a CI change. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_015ao5paex73xXvuM424Libo --- .gitattributes | 1 + .github/workflows/test.yml | 22 +++++++ composer.json | 9 ++- phpstan.neon | 4 ++ src/Client.php | 68 ++++++++++++++++----- src/Http/CurlTransport.php | 27 ++++++++- tests/StaticAnalysisCiTest.php | 106 +++++++++++++++++++++++++++++++++ 7 files changed, 219 insertions(+), 18 deletions(-) create mode 100644 phpstan.neon create mode 100644 tests/StaticAnalysisCiTest.php diff --git a/.gitattributes b/.gitattributes index 4d825e5..2b44aa2 100644 --- a/.gitattributes +++ b/.gitattributes @@ -3,6 +3,7 @@ /.gitignore export-ignore /.phpunit.result.cache export-ignore /composer.lock export-ignore +/phpstan.neon export-ignore /phpunit.xml.dist export-ignore /scripts export-ignore /tests export-ignore diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 7711239..c37f6f3 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -38,6 +38,28 @@ jobs: - name: Run unit tests run: vendor/bin/phpunit + static-analysis: + name: PHPStan (level 9, no baseline) + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false + + - name: Setup PHP + uses: shivammathur/setup-php@f3e473d116dcccaddc5834248c87452386958240 # 2.37.2 + with: + php-version: "8.3" + extensions: curl, json + coverage: none + + - name: Install dependencies + run: composer update --no-interaction --prefer-dist --no-progress + + # No baseline, no --memory-limit escape hatch: a new error fails the build. + - name: Run PHPStan + run: vendor/bin/phpstan analyse --no-progress --error-format=github + clean-install: name: Packaged Composer example runs-on: ubuntu-latest diff --git a/composer.json b/composer.json index c9a4961..e84e700 100644 --- a/composer.json +++ b/composer.json @@ -37,6 +37,7 @@ "/.gitignore", "/.phpunit.result.cache", "/composer.lock", + "/phpstan.neon", "/phpunit.xml.dist", "/scripts", "/tests", @@ -50,6 +51,7 @@ "lib-curl": ">=7.58.0" }, "require-dev": { + "phpstan/phpstan": "^2.1", "phpunit/phpunit": "^10.5 || ^11.0 || ^12.0 || ^13.0" }, "autoload": { @@ -63,7 +65,12 @@ } }, "scripts": { - "test": "phpunit" + "test": "phpunit", + "phpstan": "phpstan analyse", + "check": [ + "@phpstan", + "@test" + ] }, "config": { "sort-packages": true diff --git a/phpstan.neon b/phpstan.neon new file mode 100644 index 0000000..3e639cd --- /dev/null +++ b/phpstan.neon @@ -0,0 +1,4 @@ +parameters: + level: 9 + paths: + - src diff --git a/src/Client.php b/src/Client.php index f81be4a..64acd07 100644 --- a/src/Client.php +++ b/src/Client.php @@ -214,7 +214,9 @@ private function historical(string $period, ?string $byCode): array */ private function dataOrFail(array $body, string $path): array { - if (($body['status'] ?? null) !== 'success' || !is_array($body['data'] ?? null)) { + $data = $body['data'] ?? null; + + if (($body['status'] ?? null) !== 'success' || !is_array($data)) { throw new ApiException( sprintf('Unexpected response shape from %s.', $path), 200, @@ -222,7 +224,7 @@ private function dataOrFail(array $body, string $path): array ); } - return $body['data']; + return self::stringKeyed($data); } /** @@ -262,7 +264,7 @@ private function priceOrFail(mixed $row, array $body, string $path): Price { if (is_array($row)) { try { - return Price::fromArray($row); + return Price::fromArray(self::stringKeyed($row)); } catch (ApiException $e) { throw new ApiException($this->malformedRowMessage($path, $e->getMessage()), 200, $body); } @@ -271,6 +273,29 @@ private function priceOrFail(mixed $row, array $body, string $path): Price throw new ApiException($this->malformedRowMessage($path, 'Price row is not an object.'), 200, $body); } + /** + * Restore JSON object keys to strings. + * + * json_decode(..., true) turns a numeric JSON key such as "2026" into an + * int array key, so a decoded object is array rather + * than array. Casting the keys back is not a cosmetic + * narrowing: it is the inverse of that decode step, and it makes the + * array this SDK declares everywhere actually true. + * + * @param array $decoded + * + * @return array + */ + private static function stringKeyed(array $decoded): array + { + $keyed = []; + foreach ($decoded as $key => $value) { + $keyed[(string) $key] = $value; + } + + return $keyed; + } + private function malformedRowMessage(string $path, string $detail): string { if ($path === '/v1/prices/latest') { @@ -311,12 +336,13 @@ private function request(string $rawPath, array $params): array 'Accept' => 'application/json', 'User-Agent' => 'oilpriceapi-php/' . self::VERSION, ]; - if (!$isDemo && $this->apiKey !== null) { + if (!$isDemo) { + // Not demo means the guard above already proved the key is present. $headers['Authorization'] = 'Token ' . $this->apiKey; } + // max() guarantees at least one iteration, so $response is always assigned. $attempts = max(1, $this->maxRetries + 1); - $response = null; for ($attempt = 0; $attempt < $attempts; $attempt++) { $response = $this->transport->request('GET', $url, $headers, $this->timeout); @@ -337,8 +363,6 @@ private function request(string $rawPath, array $params): array ($this->sleeper)($delay); } - assert($response instanceof HttpResponse); - return $this->handleResponse($response, $path); } @@ -411,14 +435,28 @@ private function assertSameOrigin(string $url, string $rawPath): void */ private static function originOf(array $parts): string { - $scheme = strtolower((string) ($parts['scheme'] ?? '')); - $host = strtolower((string) ($parts['host'] ?? '')); + $scheme = self::lowerPart($parts, 'scheme'); + $host = self::lowerPart($parts, 'host'); $defaultPorts = ['http' => 80, 'https' => 443]; $port = $parts['port'] ?? ($defaultPorts[$scheme] ?? null); // Any userinfo at all is a mismatch: the configured base URL carries none. $userInfo = isset($parts['user']) || isset($parts['pass']) ? 'userinfo@' : ''; - return $scheme . '://' . $userInfo . $host . ':' . ($port === null ? '' : (string) $port); + return $scheme . '://' . $userInfo . $host . ':' . (is_int($port) ? (string) $port : ''); + } + + /** + * A parse_url() part, lowercased. A non-string part is an origin we cannot + * name, which must not compare equal to one we can - hence '' rather than + * a cast that would turn null, 0 or false into something plausible. + * + * @param array $parts + */ + private static function lowerPart(array $parts, string $key): string + { + $value = $parts[$key] ?? null; + + return is_string($value) ? strtolower($value) : ''; } private function offOriginPath(string $path, string $reason): ApiException @@ -576,8 +614,9 @@ private function handleResponse(HttpResponse $response, string $path): array private function errorMessage(array $body, string $fallback): string { // Production error envelope: {"error": {"code": ..., "message": ...}} - if (isset($body['error']['message']) && is_string($body['error']['message']) && $body['error']['message'] !== '') { - return $body['error']['message']; + $error = $body['error'] ?? null; + if (is_array($error) && isset($error['message']) && is_string($error['message']) && $error['message'] !== '') { + return $error['message']; } foreach (['message', 'error', 'detail'] as $key) { @@ -586,8 +625,9 @@ private function errorMessage(array $body, string $fallback): string } } - if (isset($body['data']['message']) && is_string($body['data']['message'])) { - return $body['data']['message']; + $data = $body['data'] ?? null; + if (is_array($data) && isset($data['message']) && is_string($data['message'])) { + return $data['message']; } return $fallback; diff --git a/src/Http/CurlTransport.php b/src/Http/CurlTransport.php index 9008f7e..b5a2518 100644 --- a/src/Http/CurlTransport.php +++ b/src/Http/CurlTransport.php @@ -23,6 +23,13 @@ final class CurlTransport implements HttpTransport { public function request(string $method, string $url, array $headers, float $timeout): HttpResponse { + if ($url === '' || $method === '') { + throw new TransportException( + 'A transport request needs a non-empty HTTP method and URL; ' + . 'got method ' . var_export($method, true) . ' and URL ' . var_export($url, true) . '.', + ); + } + $headerLines = []; foreach ($headers as $name => $value) { $headerLines[] = $name . ': ' . $value; @@ -116,12 +123,26 @@ private static function sameOrigin(string $a, string $b): bool */ private static function originOf(array $parts): string { - $scheme = strtolower((string) ($parts['scheme'] ?? '')); - $host = strtolower((string) ($parts['host'] ?? '')); + $scheme = self::lowerPart($parts, 'scheme'); + $host = self::lowerPart($parts, 'host'); $defaultPorts = ['http' => 80, 'https' => 443]; $port = $parts['port'] ?? ($defaultPorts[$scheme] ?? null); $userInfo = isset($parts['user']) || isset($parts['pass']) ? 'userinfo@' : ''; - return $scheme . '://' . $userInfo . $host . ':' . ($port === null ? '' : (string) $port); + return $scheme . '://' . $userInfo . $host . ':' . (is_int($port) ? (string) $port : ''); + } + + /** + * A parse_url() part, lowercased. A non-string part is an origin we cannot + * name, which must not compare equal to one we can - hence '' rather than + * a cast that would turn null, 0 or false into something plausible. + * + * @param array $parts + */ + private static function lowerPart(array $parts, string $key): string + { + $value = $parts[$key] ?? null; + + return is_string($value) ? strtolower($value) : ''; } } diff --git a/tests/StaticAnalysisCiTest.php b/tests/StaticAnalysisCiTest.php new file mode 100644 index 0000000..129065b --- /dev/null +++ b/tests/StaticAnalysisCiTest.php @@ -0,0 +1,106 @@ + + */ + private static function composer(): array + { + $decoded = json_decode( + (string) file_get_contents(self::root() . '/composer.json'), + true, + flags: JSON_THROW_ON_ERROR, + ); + self::assertIsArray($decoded); + + return $decoded; + } + + public function testPhpstanIsADevDependency(): void + { + $composer = self::composer(); + + self::assertIsArray($composer['require-dev'] ?? null); + self::assertArrayHasKey( + 'phpstan/phpstan', + $composer['require-dev'], + 'phpstan/phpstan must be a dev dependency so `composer install` provides it.', + ); + } + + public function testComposerExposesAPhpstanScript(): void + { + $composer = self::composer(); + $scripts = $composer['scripts'] ?? null; + + self::assertIsArray($scripts); + self::assertArrayHasKey('phpstan', $scripts, 'composer.json needs a "phpstan" script.'); + } + + public function testPhpstanConfigTargetsSrcAtAHonestLevelWithNoBaseline(): void + { + $configPath = self::root() . '/phpstan.neon'; + self::assertFileExists($configPath); + + $config = (string) file_get_contents($configPath); + + self::assertMatchesRegularExpression( + '~^\s*level:\s*(9|max)\s*$~m', + $config, + 'phpstan.neon must declare the level it actually enforces.', + ); + self::assertMatchesRegularExpression('~^\s*-\s*src\s*$~m', $config, 'phpstan.neon must analyse src/.'); + + // A baseline is the documented way to ship a green check over real + // errors. If one is ever introduced, this test is the place to argue + // for it - not a silent file. + self::assertStringNotContainsString('baseline', $config); + self::assertFileDoesNotExist(self::root() . '/phpstan-baseline.neon'); + } + + public function testCiRunsStaticAnalysisOnPullRequests(): void + { + $workflow = (string) file_get_contents(self::root() . '/.github/workflows/test.yml'); + + self::assertStringContainsString('pull_request', $workflow); + self::assertMatchesRegularExpression( + '~(vendor/bin/phpstan|composer (run(-script)? )?phpstan)~', + $workflow, + 'The test workflow must invoke PHPStan, or the config is decoration.', + ); + } + + public function testPhpstanConfigIsNotShippedInTheComposerArchive(): void + { + $composer = self::composer(); + $exclude = $composer['archive']['exclude'] ?? null; + + self::assertIsArray($exclude); + self::assertContains('/phpstan.neon', $exclude, 'Dev tooling config must not ship to Packagist.'); + + $attributes = (string) file_get_contents(self::root() . '/.gitattributes'); + self::assertStringContainsString('/phpstan.neon export-ignore', $attributes); + } +}