lvl0/incr

Fork 0

Add PHPStan + larastan static analysis #55

New issue

Closed

opened 2026-08-15 13:42:39 +02:00 by myrmidex · 1 comment

myrmidex commented

2026-08-15 13:42:39 +02:00

Owner

incr has no static analysis at all — no phpstan.neon, and neither phpstan/phpstan nor larastan/larastan in composer.json. ffr runs level 7 in CI.

Why this matters now

Every other v0.4.0 ticket lists "PHPStan clean" in its acceptance criteria, and container_analyse is a standard gate in the global workflow config. That gate does not currently exist in this project, so those checkboxes are unsatisfiable until this lands.

Scope

Mirror ffr's setup:

composer require --dev larastan/larastan phpstan/phpstan — match ffr's constraints (larastan ^3.5, phpstan ^2.1.32 <2.2). Skip phpstan-mockery unless mocks appear in the tests from #54.
phpstan.neon at level 7, paths: [app/, tests/], excluding bootstrap/*.php and storage/*
Generate phpstan-baseline.neon and include it
Add an analysis step to .forgejo/workflows/ci.yml

Sequencing

Land this after the deletion tickets (#47–#53), not before. A baseline generated now would be mostly noise about code that is about to be removed — and the whole point of a baseline is that it stays small and shrinks.

Practical consequence: earlier tickets' "PHPStan clean" criteria are aspirational until this lands. Treat them as "lint + tests clean" until then, and let this ticket be the one that makes the gate real.

Acceptance criteria

vendor/bin/phpstan analyse runs clean at level 7
Baseline is minimal (ideally empty, given how little code remains post-simplification)
CI fails on a new violation
container_analyse MCP tool works against this project

incr has **no static analysis at all** — no `phpstan.neon`, and neither `phpstan/phpstan` nor `larastan/larastan` in `composer.json`. ffr runs level 7 in CI. ## Why this matters now Every other v0.4.0 ticket lists "PHPStan clean" in its acceptance criteria, and `container_analyse` is a standard gate in the global workflow config. **That gate does not currently exist in this project**, so those checkboxes are unsatisfiable until this lands. ## Scope Mirror ffr's setup: - `composer require --dev larastan/larastan phpstan/phpstan` — match ffr's constraints (`larastan ^3.5`, `phpstan ^2.1.32 <2.2`). Skip `phpstan-mockery` unless mocks appear in the tests from #54. - `phpstan.neon` at level 7, `paths: [app/, tests/]`, excluding `bootstrap/*.php` and `storage/*` - Generate `phpstan-baseline.neon` and include it - Add an analysis step to `.forgejo/workflows/ci.yml` ## Sequencing Land this **after** the deletion tickets (#47–#53), not before. A baseline generated now would be mostly noise about code that is about to be removed — and the whole point of a baseline is that it stays small and shrinks. Practical consequence: earlier tickets' "PHPStan clean" criteria are aspirational until this lands. Treat them as "lint + tests clean" until then, and let this ticket be the one that makes the gate real. ## Acceptance criteria - [ ] `vendor/bin/phpstan analyse` runs clean at level 7 - [ ] Baseline is minimal (ideally empty, given how little code remains post-simplification) - [ ] CI fails on a new violation - [ ] `container_analyse` MCP tool works against this project

myrmidex added this to the v0.4.0 milestone 2026-08-15 13:42:39 +02:00

myrmidex added the

enhancement

label 2026-08-15 13:42:39 +02:00

myrmidex self-assigned this 2026-08-15 13:42:39 +02:00

myrmidex referenced this issue

2026-08-15 14:43:39 +02:00

Remove milestones, progress bar and stats box #49

myrmidex referenced this issue

2026-08-15 15:20:32 +02:00

Collapse entry ledger to a scalar count column #50

myrmidex referenced this issue

2026-08-15 21:34:56 +02:00

Remove asset and price-tracking subsystem #48

myrmidex referenced this issue

2026-08-15 21:43:42 +02:00

Collapse the fake user layer #53

myrmidex referenced this issue

2026-08-15 21:43:50 +02:00

Replace React/Inertia with Blade + Livewire 4 #52

myrmidex commented

2026-08-15 21:51:05 +02:00

Author

Owner

Done — `d2dfe44`

Static analysis exists now. Level 7, zero errors, no baseline file.

larastan/larastan ^3.5 (→ 3.9.6) and phpstan/phpstan ^2.1.32 <2.2 (→ 2.1.56), matching ffr
phpstan.neon — level 7, paths app/, database/, tests/
CI step between Lint and Tests, with --error-format=github for inline annotations
Skipped phpstan-mockery — the suite uses factories and real HTTP calls, no mocks

Deviation from the ticket: database/ is included in the analysed paths, which ffr does not do. The migrations now carry real logic (the SUM(quantity) backfill loop) and the factories back every test. All 12 migrations are clean.

The 17 errors it found

Real typing improvements, not suppressions:

Relation generics: @return BelongsTo<User, $this> on Tracker::user(), @return HasOne<Tracker, $this> on User::tracker()
CounterController::increment() — $tracker->refresh()->count tripped "undefined property Model::$count" because refresh() erases to Model. Split into $tracker->refresh(); then $tracker->count. Behaviour identical.

One genuine Laravel quirk, ignored deliberately: migration files return an anonymous class defining up()/down(), but the abstract Migration base declares neither, so the call cannot be typed. Narrowly-scoped ignoreErrors matched to tests/Feature/*MigrationTest.php and only that message.

Why not an interface — tested, not assumed

Review pushed back on the ignore rule and proposed a marker interface (extends Migration implements ReversibleMigration), asking why I had dismissed it.

I built it, and the tests passed — which is precisely the danger. Tests\ is under autoload-dev, and docker/production/Dockerfile:58 runs composer install --no-dev. Every migration would fatal with "Interface not found" on deploy, and the test suite could never catch it because tests always have dev autoload.

That reasoning is now a comment in phpstan.neon so the next person does not re-attempt it.

Gate verified against injected errors

$tracker->nonexistentProperty in app code → caught as App\Models\Tracker::$nonexistentProperty, not the vague Model:: seen before the generics were added, confirming inference genuinely improved
bogusMethod() inside the file the ignore rule covers → still caught, confirming the rule suppresses only up()/down()

Level 8

Tested rather than guessed: 8 errors across 4 files. Two of those files are deleted by #53 and #52, one of the errors being a real null-safety bug ($this->user()->id in an app with no auth). After those tickets, level 8 costs about five assertNotNull() calls in tests. Noted on #53 and #52.

Gates

PHPStan 0 errors (level 7) · Pint PASS 48 files · PHPUnit OK 31 tests, 81 assertions · container_analyse works against this project

## Done — `d2dfe44` Static analysis exists now. **Level 7, zero errors, no baseline file.** - `larastan/larastan ^3.5` (→ 3.9.6) and `phpstan/phpstan ^2.1.32 <2.2` (→ 2.1.56), matching ffr - `phpstan.neon` — level 7, paths `app/`, `database/`, `tests/` - CI step between Lint and Tests, with `--error-format=github` for inline annotations - Skipped `phpstan-mockery` — the suite uses factories and real HTTP calls, no mocks **Deviation from the ticket:** `database/` is included in the analysed paths, which ffr does not do. The migrations now carry real logic (the `SUM(quantity)` backfill loop) and the factories back every test. All 12 migrations are clean. ### The 17 errors it found **Real typing improvements, not suppressions:** - Relation generics: `@return BelongsTo<User, $this>` on `Tracker::user()`, `@return HasOne<Tracker, $this>` on `User::tracker()` - `CounterController::increment()` — `$tracker->refresh()->count` tripped "undefined property `Model::$count`" because `refresh()` erases to `Model`. Split into `$tracker->refresh();` then `$tracker->count`. Behaviour identical. **One genuine Laravel quirk, ignored deliberately:** migration files return an anonymous class defining `up()`/`down()`, but the abstract `Migration` base declares neither, so the call cannot be typed. Narrowly-scoped `ignoreErrors` matched to `tests/Feature/*MigrationTest.php` and only that message. ### Why not an interface — tested, not assumed Review pushed back on the ignore rule and proposed a marker interface (`extends Migration implements ReversibleMigration`), asking why I had dismissed it. **I built it, and the tests passed** — which is precisely the danger. `Tests\` is under `autoload-dev`, and `docker/production/Dockerfile:58` runs `composer install --no-dev`. Every migration would fatal with "Interface not found" on deploy, and **the test suite could never catch it** because tests always have dev autoload. That reasoning is now a comment in `phpstan.neon` so the next person does not re-attempt it. ### Gate verified against injected errors - `$tracker->nonexistentProperty` in app code → caught as `App\Models\Tracker::$nonexistentProperty`, not the vague `Model::` seen before the generics were added, confirming inference genuinely improved - `bogusMethod()` **inside the file the ignore rule covers** → still caught, confirming the rule suppresses only `up()`/`down()` ### Level 8 Tested rather than guessed: 8 errors across 4 files. Two of those files are deleted by #53 and #52, one of the errors being a real null-safety bug (`$this->user()->id` in an app with no auth). After those tickets, level 8 costs about five `assertNotNull()` calls in tests. Noted on #53 and #52. ### Gates PHPStan 0 errors (level 7) · Pint PASS 48 files · PHPUnit OK 31 tests, 81 assertions · `container_analyse` works against this project

myrmidex closed this issue

2026-08-15 21:51:09 +02:00