chore: run PHPStan level 9 in CI and clear the 14 real errors (#24)

.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

.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

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

phpstan.neon

@@ -0,0 +1,4 @@
+parameters:
+    level: 9
+    paths:
+        - src

src/Client.php

@@ -214,15 +214,17 @@ 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,
                 $body,
             );
         }
 
-        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<array-key, mixed> rather
+     * than array<string, mixed>. Casting the keys back is not a cosmetic
+     * narrowing: it is the inverse of that decode step, and it makes the
+     * array<string, mixed> this SDK declares everywhere actually true.
+     *
+     * @param array<array-key, mixed> $decoded
+     *
+     * @return array<string, mixed>
+     */
+    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<string, mixed> $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;

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<string, mixed> $parts
+     */
+    private static function lowerPart(array $parts, string $key): string
+    {
+        $value = $parts[$key] ?? null;
+
+        return is_string($value) ? strtolower($value) : '';
     }
 }

tests/StaticAnalysisCiTest.php

@@ -0,0 +1,106 @@
+<?php
+
+declare(strict_types=1);
+
+namespace OilPriceAPI\Tests;
+
+use PHPUnit\Framework\TestCase;
+
+/**
+ * Static analysis is only worth having if it actually runs on every change,
+ * and only worth trusting if it is not silenced.
+ *
+ * A phpstan.neon in the repo proves nothing on its own: the file existing is
+ * not the same as the job running, and a baseline turns a failing check into
+ * a green one without fixing anything. These assertions pin all three - the
+ * tool is a dev dependency, CI invokes it on pull requests, and the config
+ * carries a real level with no baseline.
+ */
+final class StaticAnalysisCiTest extends TestCase
+{
+    private static function root(): string
+    {
+        return dirname(__DIR__);
+    }
+
+    /**
+     * @return array<string, mixed>
+     */
+    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);
+    }
+}