Skip to content

Tighten static-analysis surface (strict-types everywhere, strict-rules, generics, sealed sum types, exhaustive enum match) #1291

Description

@rumblefrog

Problem

PHPStan runs at level 5 (per web/phpstan.neon:10) with the deprecation-rules plugin, the in-tree SmartyTemplateRule, and staabm/phpstan-dba for raw-SQL type-checking against the live schema. That's a real foundation, but the surface area where a bug becomes a static failure (red CI on the PR) instead of a runtime failure (500 in prod, or worse, silently wrong row inserted) is still narrow. A lot of patterns that Rust would refuse to compile slide through this codebase as silent coercions, untyped arrays, mixed returns, non-exhaustive match / switch, loose == comparisons, and unconstrained $_GET / $_POST reads.

#1101 covers the level climb (5 → 6 → 7 → 8, one step at a time). #1273 added the deprecation gate. #1290 covers the modernization (enums, namespaces, native types). This issue is the umbrella for the third leg of the static-analysis tripod: stricter rules, more expressive types, and project-specific custom rules. The bar is "what would Rust catch at compile time" — exhaustive matches, no silent coercions, no mixed in public APIs, sealed sum types, generics, purity annotations. PHP can't quite get there, but PHPStan + strict-rules + declare(strict_types=1) + custom in-tree rules close most of the gap.

This is opportunistic: each phase strictly increases the static-failure surface without changing wire format, schema, or behavior. The phases are independently revertable and compose with #1101 / #1290 — they don't block each other.

What's already in place (the baseline to build on)

Gate Source What it catches
PHPStan level 5 web/phpstan.neon:10 Standard reachability + parameter-count + obvious type bugs
Deprecation rules phpstan/phpstan-deprecation-rules (web/phpstan.neon:7) PHP 8.1 null-into-scalar surface, deprecated function calls
Raw-SQL type check staabm/phpstan-dba (web/phpstan.neon:3) Column types in Database::query("SELECT …") validated against the live MariaDB schema
Custom Smarty bridge Sbpp\PHPStan\SmartyTemplateRule (web/includes/PHPStan/SmartyTemplateRule.php) Each Sbpp\View\…View cross-checked against the .tpl it renders — declared-but-unused properties, used-but-undeclared variables
Custom prefix-aware SQL rule Sbpp\PhpStan\SbppSyntaxErrorInQueryMethodRule (web/phpstan.neon:36-47) :prefix_ placeholder rewriting in Database::query's first arg
Runtime deprecation trap web/tests/integration/Php82DeprecationsTest.php (#1273) Marquee page handlers fail PHPUnit if they raise E_DEPRECATED (catches what PHPStan can't see — auth/openid.php, runtime-null values that look non-null to the type system)

web/phpstan-baseline.neon is 781 lines (wc -l). The dominant identifiers are variable.undefined (global $userbank invisible to PHPStan), class.notFound (Smarty bridging), empty.variable, booleanNot.alwaysFalse / notEqual.alwaysTrue (already-known dead branches), phpDoc.parseError (old comment style). Most of these collapse naturally under one or more of the phases below.

What we're missing (Rust-comparable)

A. declare(strict_types=1) everywhere

Currently 81 files declare it (every Sbpp\…-namespaced file). The legacy core (web/includes/CUserManager.php, Database.php, Auth.php, Host.php, Log.php, Config.php, JWT.php, CSRF.php, Crypto.php, Api.php, ApiError.php, AdminTabs.php, auth/handler/*.php, system-functions.php, page-builder.php, init.php, every web/pages/*.php, every web/api/handlers/*.php) does NOT.

Without declare(strict_types=1), calling function foo(int $x) with '42' silently coerces to 42. Calling it with '42abc' coerces to 42 and emits a Notice PHP increasingly treats as Warning and PHP 9 will turn into a TypeError. With it, every coercion at a function boundary becomes a TypeError immediately — exactly what Rust does at compile time, except PHP has to do it at the call site because there's no compile step.

This is a pure-additive runtime gate. It interacts with #1290 phase A (typing the legacy method signatures) — declare(strict_types=1) only matters once parameters are typed. Order: ship it as the last commit in each #1290 phase A class-by-class PR, after the parameters are typed, so the strict gate has something to enforce.

Closing invariant: rg -L 'declare\(strict_types=1\)' web/includes web/pages web/api web/install web/updater --type php returns only the documented third-party exceptions (auth/openid.php, the SteamID library).

B. phpstan/phpstan-strict-rules

The official strict-rules plugin (phpstan/phpstan-strict-rules) adds the rule pack PHP itself doesn't enforce:

  • Forbid loose comparisons (== / != flagged; require === / !==). Today: 39 sites in page.banlist.php, 33 in admin.bans.php, 34 in page.commslist.php, 17 in admin.edit.admindetails.php, 41 in admin.edit.group.php, … The codebase is full of if ($_GET['mode'] == "delete") — those work, but $_GET['mode'] == 0 is true (string "delete" coerces to 0 via the (int) rule), so the same shape applied to a numeric route param ships a real bug. Strict-rules forces every == site to either be promoted to === (exact match — what the author meant) or wrapped in an explicit cast.
  • Forbid empty()empty('0') is true, empty(null) is true, empty(0) is true, empty([]) is true, empty('false') is false. The function is the source of every "why is this empty / not empty?" stack-overflow question. 200+ sites across the codebase (rough count from rg -c '\bempty\('); each one becomes either === '' / === null / === 0 / === [] per the actual intent.
  • Disallow switch without default — pairs with Adopt modern PHP features across legacy core (enums, native types, namespacing, match, str_contains, …) #1290 phase C (switch → match), since match always requires exhaustiveness anyway.
  • Require @throws — for every throw site, the calling chain has to declare or catch the exception in @throws. This is the closest PHP gets to Java's checked exceptions / Rust's Result<T, E> propagation; PHPStan tracks the throws-graph and flags un-caught un-declared exceptions.
  • Forbid uninitialized variable use — already largely covered by level 5, strict-rules makes it ironclad.
  • Forbid static calls on instance methods, forbid dynamicCallOnStaticMethod, etc. — small wins, mechanical.

The baseline will balloon — probably another 500-800 entries on top of the current 781. That's expected; the AGENTS.md "raise one step at a time" rule applies. Strategy: enable strict-rules, regenerate the baseline, and carve out follow-up PRs to drain the baseline category by category (one PR per identifier — equal.notAllowed, empty.notAllowed, switch.noDefault, etc.). #1101's level climb stays on its own track.

C. PHPStan level 9 on the new layer (carved out from excludePaths)

#1101 ramps the global level one notch at a time and "stops at whatever level produces a baseline we can't make sense of" — likely 7 or 8 for the codebase as a whole. That's the right call for the legacy core, but the modern layer (web/includes/View/*, Mail/*, Markup/*, Util/*, Theme.php, Version.php, Renderer.php, phpstan/* rules) is already strict-types + typed everywhere — it can carry level 9 today.

PHPStan supports per-path level overrides via parametersSchema.level per-include. Concretely, web/phpstan.neon would add:

parameters:
    level: 5
    # … existing config …

# Per-path overrides for the modern layer that already meets a higher bar.
includes:
    - phpstan-modern.neon

# phpstan-modern.neon:
parameters:
    level: 9
    paths:
        - includes/View
        - includes/Mail
        - includes/Markup
        - includes/Util
        - includes/Theme.php
        - includes/Version.php
        - phpstan

Level 9 disallows mixed everywhere. The view DTOs already type every property; the $theme / $userbank globals don't reach into includes/View, so this is tractable. The win: a regression that introduces a mixed return into the modern layer fails the build immediately, instead of waiting for the layer-wide level climb to catch it years later.

D. Generics via @template

PHPStan supports generics in docblocks (@template T, @template-implements, @phpstan-template-covariant). PHP doesn't have them natively but the analyzer treats them with full Rust-like rigor.

Concrete wins:

  • Database::resultset(?array $inputParams = null, $fetchType = PDO::FETCH_ASSOC) returns array<int, array<string, mixed>> today. With a @template TRow of array<string, mixed> on a hypothetical Repository<TRow> wrapper, the result type carries the row shape — BansRepository::activeBans() returns array<int, BanRow> where BanRow = array{bid: int, authid: string, name: string, ...} and PHPStan checks every $row['authid'] access against the actual schema. (SbppSyntaxErrorInQueryMethodRule + phpstan-dba already give us this for raw SQL; lifting it into a typed-result wrapper makes the call sites type-safe too.)
  • Api::register($action, $fn, …) accepts callable $fn today. With @template TParams of array, @template TResult on a HandlerRegistration<TParams, TResult>, the callable's input + output shapes are pinned at registration time and the dispatcher's invocation site is checked against them.
  • A Result<T, E> shape via @template-implements on a sealed interface (see phase F below) — Result<BanRow, ApiError> becomes an honest type.

This is purely additive — no runtime change. Rollout: pick one repository / collection / dispatcher at a time and add the generics; PHPStan immediately starts catching every shape-mismatch downstream. Reference: Sbpp\View\Perms::for already uses array<string, bool> shape, but a PermissionSnapshot value object with @template-style shape would be tighter.

E. Typed DTOs replacing array<string, mixed> for DB rows

Today, $row['authid'] reads from a mixed slot of a mixed array — PHPStan can prove the array exists, but not what's in 'authid'. phpstan-dba partially closes this for SQL strings it can introspect, but as soon as the row leaves Database::single() / ::resultset(), the shape is lost.

Pattern (matches the View DTO shape already in place):

final readonly class BanRow {
    public function __construct(
        public int $bid,
        public string $authid,
        public string $name,
        public ?string $ip,
        public int $created,
        public int $length,
        // …
    ) {}

    /** @param array<string, mixed> $row */
    public static function fromRow(array $row): self {
        return new self(
            bid: (int) $row['bid'],
            authid: (string) $row['authid'],
            // … one cast per column, scoped to one place …
        );
    }
}

The (int) / (string) casts move from every consumer to the DTO factory — surgical instead of scattered. Every consumer reads $ban->authid (typed string, statically narrowed) instead of $row['authid'] (typed mixed, runtime-coerced). A column rename in struc.sql becomes one PHPStan error instead of seven Notices in production.

Scope per PR: one row shape per PR (BanRow, CommRow, AdminRow, ServerRow, LogRow, NoteRow, …). Each PR replaces the consumers in lockstep so the array form stops being read in the same commit the DTO factory lands.

F. Sealed sum types — Result<T, E> shape via interfaces

PHP doesn't have sealed classes natively (PHP 8.4 added #[\AllowDynamicProperties] but not sealed hierarchies). PHPStan supports sealed-tagged-union via the @phpstan-sealed-tagged-union annotation on interfaces:

/**
 * @phpstan-sealed-tagged-union BanLookup\Found|BanLookup\NotFound|BanLookup\Forbidden
 */
interface BanLookupResult {}

namespace BanLookup;

final readonly class Found implements BanLookupResult {
    public function __construct(public BanRow $ban) {}
}

final readonly class NotFound implements BanLookupResult {}

final readonly class Forbidden implements BanLookupResult {
    public function __construct(public string $reason) {}
}

Then the consumer:

$result = $repo->findBan($bid);
return match (true) {
    $result instanceof BanLookup\Found    => $this->renderBan($result->ban),
    $result instanceof BanLookup\NotFound => $this->render404(),
    $result instanceof BanLookup\Forbidden => $this->render403($result->reason),
};

PHPStan checks the match is exhaustive against the sealed set — adding a new BanLookup\AwaitingApproval case fails the build at every call site that hasn't handled it. This is the closest PHP gets to Rust's match on enum/Result, and it composes with #1290's WebPermission enum work.

Where this lands big: API handlers that today either return data, throw ApiError, or return ['__redirect' => '...'] — three return shapes shoved through one signature. Modeling them as a HandlerResult = Data | Error | Redirect sealed union makes the dispatcher's branching exhaustive.

G. Exhaustive match over enums (auto-gain on top of #1290 phase D)

PHPStan already flags non-exhaustive match (Enum::*) { … } when the input is typed as a backed enum and a case is missing — no plugin needed. Today this catches nothing because there's exactly one enum (Sbpp\Mail\EmailType, used in two match blocks both already exhaustive).

#1290 phase D adds LogType, BanType, BanRemoval, WebPermission. The moment those land, every Log::add(LogType::Error, …) / BanType::Steam / etc. match site is checked against the enum's full case set. Forgetting to handle LogType::Warning in a future log-rendering switch is a static failure — same shape as Rust's exhaustiveness check.

This issue's contribution: codify in AGENTS.md that enums must be matched, not switched (matching gives exhaustiveness; switching doesn't), and add a paragraph in the "Conventions" section pointing at this property as the reason enum is the preferred shape over class constants.

H. Custom in-tree PHPStan rules for project invariants

Sbpp\PHPStan\SmartyTemplateRule is the precedent: a small focused rule that knows something specific about this codebase (View DTO ↔ template binding) that no generic rule could catch. Same pattern, more rules:

  • LogTypeIsEnumRule — fail when Log::add(...)'s first arg is anything other than a LogType::* case. Currently 'm' / 'e' / 'w' are passed everywhere; once Adopt modern PHP features across legacy core (enums, native types, namespacing, match, str_contains, …) #1290 phase D.1 lands the enum, this rule prevents reintroducing the magic-char form.
  • SuperGlobalAccessRule — fail when $_GET[...] / $_POST[...] / $_SESSION[...] / $_COOKIE[...] is read outside an explicit allowlist (page-builder.php, init.php, the Request reader once it exists). Today these are scattered across every page handler and API handler; centralizing the read forces every input through a typed-validation seam.
  • GlobalKeywordRule — fail when global $userbank / global $theme appears outside a documented allowlist of files. Same "the legacy pattern is contained, not spreading" enforcement as the super-global rule.
  • ApiHandlerSignatureRule — fail when an api_<topic>_<action> function in web/api/handlers/*.php doesn't have @param / @return with concrete shapes (an array<string, mixed> @param is too loose).
  • PrefixedTableInQueryRule — extension of the existing SbppSyntaxErrorInQueryMethodRule that flags inline 'sb_' table-name usage in any string passed to Database::query (today only the syntax of the :prefix_ placeholder is checked; inline-prefix is the anti-pattern AGENTS.md calls out).
  • NoFilterRequiresCommentRule — Smarty {$foo nofilter} requires the {* nofilter: <why> *} comment immediately above it, per AGENTS.md. The convention is documented; a rule turns it into a build failure if anyone forgets.

Each rule is a single PHP class under web/includes/PHPStan/, a single registration block in web/phpstan.neon, and a unit test. Pattern is established (SmartyTemplateRule); incremental cost per rule is bounded.

I. Typed input boundary — Sbpp\Http\Request reader

Today every page / API handler does:

$bid    = (int) ($_GET['bid'] ?? 0);
$action = (string) ($_GET['a'] ?? '');
$mode   = (string) ($_GET['mode'] ?? '');
if ($mode == "delete") { … }

That (int) / (string) cast + ?? default + bare comparison shape is repeated hundreds of times. Each repetition is a chance to forget the cast or the default and ship a null-into-scalar deprecation (#1273) or a loose-comparison bug.

A small Sbpp\Http\Request value object centralizes the boundary:

final readonly class Request {
    public function __construct(
        /** @var array<string, mixed> */ public array $get,
        /** @var array<string, mixed> */ public array $post,
        /** @var array<string, mixed> */ public array $session,
    ) {}

    public static function fromGlobals(): self { /* … */ }

    public function intGet(string $key, int $default = 0): int { /* … */ }
    public function stringGet(string $key, string $default = ''): string { /* … */ }
    public function enumGet(string $key, string $enumClass): ?\BackedEnum { /* … */ }
    // … one method per typed shape …
}

Page handlers consume the typed reader; the SuperGlobalAccessRule from phase H locks the boundary down so $_GET[...] accesses outside the reader are a static failure. This is the "all input crosses one type-checking gate" pattern — Rust does it via serde::Deserialize derive macros; PHP needs it explicit.

Strategy: ship the reader as an additive utility (zero call-site changes), then migrate handlers one at a time, then enable the rule. Same shape as #1290 phase B's class_alias shim.

J. @phpstan-pure / @phpstan-impure annotations

PHP isn't pure-by-default like Rust, but PHPStan supports @phpstan-pure to mark functions that have no side effects. The analyzer then checks the body — any I/O, global write, exception, or call to an impure function fails the build. The win: the obvious helpers (trunc, sizeFormat, checkExtension, BitToString, parseRconStatus, Sbpp\Util\Duration::*, Sbpp\View\PermissionCatalog::groupedDisplayFromMask, …) get the annotation and a future regression that adds a Log::add(...) call in the middle of one of them fails CI immediately.

This is purely additive (annotations only — runtime behavior unaffected). Pick a handful of obviously-pure helpers per PR; let PHPStan tell you which ones aren't actually pure.

K. Typed class constants (PHP 8.3, gated on #1289)

Once #1289 lands the 8.5 floor, every const FOO = 'bar'; in the codebase can become const string FOO = 'bar';. A future change to const FOO = 0; is a static failure. Examples:

  • CSRF::FIELD_NAME = 'csrf_token'const string FIELD_NAME = 'csrf_token'
  • Mailer::DEFAULT_FROM_NAME = 'SourceBans++'const string DEFAULT_FROM_NAME = 'SourceBans++'
  • Every const TEMPLATE = 'page_*.tpl' on the View subclasses

Mechanical sweep, single PR after #1289.

Phasing

Order to minimize churn pile-up and so each phase lands on a stable predecessor:

  1. C — per-path level 9 for the modern layer. Smallest scope (one phpstan-modern.neon), highest signal-to-noise; locks the modern layer at its current strictness so Adopt modern PHP features across legacy core (enums, native types, namespacing, match, str_contains, …) #1290 doesn't accidentally regress it.
  2. Adeclare(strict_types=1) everywhere. Bundle commit-by-commit with Adopt modern PHP features across legacy core (enums, native types, namespacing, match, str_contains, …) #1290 phase A (each legacy class gets its parameters typed AND its strict-types declaration in the same PR — the two are useless apart).
  3. J@phpstan-pure on the obvious helpers. Single PR.
  4. G — codify the "enums must be matched, not switched" convention in AGENTS.md. Single small PR; bulk of the win happens automatically once Adopt modern PHP features across legacy core (enums, native types, namespacing, match, str_contains, …) #1290 phase D enums land.
  5. H — first three custom rules: LogTypeIsEnumRule, SuperGlobalAccessRule, GlobalKeywordRule. One PR per rule. Land after Adopt modern PHP features across legacy core (enums, native types, namespacing, match, str_contains, …) #1290 phase D.1 (LogType enum) for the first rule.
  6. Bphpstan/phpstan-strict-rules. Big-bang PR adds the dependency + regenerates the baseline; follow-up PRs drain the baseline category by category (equal.notAllowed, empty.notAllowed, switch.noDefault, missing.throws, …).
  7. ISbpp\Http\Request value object. One PR for the class, then one per page/handler family for the migration, then one PR enabling SuperGlobalAccessRule's tight allowlist.
  8. D — generics on Database::resultset / Api::register / new Repository<TRow> wrappers. One PR per surface.
  9. E — typed DB-row DTOs. One PR per row shape (BanRow, CommRow, AdminRow, …).
  10. F — sealed sum types where they replace array{ok: bool, error?: …, redirect?: …} shapes. Land after a handful of E PRs prove the row-DTO pattern works; the API handler HandlerResult is the marquee target.
  11. K — typed class constants. Single PR after Bump minimum PHP version to 8.5 #1289.

Each phase composes with the level climb (#1101); none of them block it. The only ordering dependency on other issues is #1290 phase D for the enum rules and #1289 for typed class constants.

Acceptance criteria

This is an umbrella, not a single-PR ticket. Marking it complete requires:

  • Every file under web/includes/ (excluding auth/openid.php, the SteamID library), web/pages/, web/api/handlers/ declares declare(strict_types=1). The rg -L 'declare\(strict_types=1\)' invariant passes.
  • phpstan/phpstan-strict-rules is in web/composer.json (dev-dep), wired in web/phpstan.neon, and the baseline doesn't grow over the strict-rules-only entries from the initial bring-up.
  • Per-path level-9 override for the modern layer is in web/phpstan.neon (or a sibling include) and CI fails on any new mixed in includes/View / includes/Mail / includes/Markup / includes/Util / includes/Theme.php / includes/Version.php / phpstan/.
  • At least three custom in-tree rules from phase H are landed (LogTypeIsEnumRule, SuperGlobalAccessRule, GlobalKeywordRule). Each has a unit test that rejects the bad shape and accepts the good shape.
  • Sbpp\Http\Request exists and is the only entry point reading $_GET / $_POST / $_SESSION / $_COOKIE outside the documented allowlist. The SuperGlobalAccessRule's allowlist is the source of truth.
  • At least one repository / collection uses @template generics with the typed-row DTO (phase D + E proven on one shape).
  • At least one sealed sum type replaces an array{ok: bool, …} return shape (phase F proven on one shape).
  • AGENTS.md "Conventions" grew rows for: declare(strict_types=1) everywhere; "enums are matched, not switched"; the Request reader as the input boundary; the per-path level-9 override as the modern-layer guardrail.
  • AGENTS.md "Anti-patterns" grew rows for: loose == / != comparison; empty() (replaced by per-shape explicit checks); $_GET / $_POST access outside Request; mixed returns in the modern layer.
  • No regression on the wire-format snapshots or the API-contract snapshot. None of this changes behavior — if a phase ever does, that phase is mis-scoped.

Out of scope (file separately if pursued)

  • The level climb itself (Raise PHPStan level from 4 toward 8 #1101). This issue makes the analyzer smarter; Raise PHPStan level from 4 toward 8 #1101 makes it stricter. They run in parallel.
  • Schema migrations. Phase E's typed DB-row DTOs wrap around the existing schema. Adding actual ENUM(...) columns (e.g. for :prefix_log.type to back the LogType enum at the DB level) is a paired migration with an updater script — different blast radius.
  • vimeo/psalm as a second analyzer. PHPStan is already the established gate; running both costs more in CI time and contributor cognitive overhead than the marginal bug-catch is worth. Pick one and push it hard. (PHPStan already dominates in this codebase via phpstan-dba + the custom rules.)
  • Removing global $userbank; / $GLOBALS['PDO']. Phase H's GlobalKeywordRule contains the spread — the existing global-state pattern stays. Killing it is a real DI migration (out of scope in Adopt modern PHP features across legacy core (enums, native types, namespacing, match, str_contains, …) #1290 too; same reasoning).
  • JS-side type tightening. web/scripts/*.js is // @ts-check on vanilla JS today. The TS-side equivalent (turn on noImplicitAny, strictNullChecks, etc.) is a separate ticket.

web/install/ and web/updater/ (wrapper + data scripts) are both in scope — strict-types, typed signatures, sealed-union refactors, custom rules, all of it. They're live code on the main path for self-hosters. The one practical wrinkle, same as in #1290: a strict-types pass on a shipped web/updater/data/<N>.php is fine (the script's effect doesn't change), but if the typing exposes a real behavior bug whose fix would alter the SQL the script runs, land that fix as a new <N+1>.php instead of editing in place. See AGENTS.md "Updater migrations".

Notes

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions