Skip to content

test+docs: cover ConnectionTransactionReturnTypeExtension + document prod illuminate/* deps#31

Open
Goosterhof wants to merge 1 commit into
mainfrom
engineer/transaction-extension-test-and-deps-doc
Open

test+docs: cover ConnectionTransactionReturnTypeExtension + document prod illuminate/* deps#31
Goosterhof wants to merge 1 commit into
mainfrom
engineer/transaction-extension-test-and-deps-doc

Conversation

@Goosterhof
Copy link
Copy Markdown
Contributor

Small quality bundle — two scoped tasks, one PR. No production-dependency or source changes (test + docs only).

Task A — direct tests for ConnectionTransactionReturnTypeExtension (Quartermaster F-2)

The extension resolves $connection->transaction(fn () => $x) to the closure's return type instead of mixed, but had no direct test — only implicit exercise via audit-snapshot rule fixtures, none of which asserted the resolved return type.

Added tests/Type/ConnectionTransactionReturnTypeExtensionTest.php (a PHPStan\Testing\TypeInferenceTestCase, mirroring the package's existing RuleTestCase conventions) + fixture tests/Fixtures/ConnectionTransactionReturnType/transaction-return-type.php. The test overrides getAdditionalConfigFiles() to load extension.neon — the same config consumers register — so the extension is genuinely active under test. The fixture assertType()s the inferred return type of transaction(...) for closures returning:

  • a constant scalar (fn (): int => 4242)
  • an object/DTO (fn (): TransactionResult => ...TransactionResult)
  • a nullable union (captured ?stringstring|null)
  • an array shape (['a', 'b']array{'a', 'b'})
  • a widened, non-constant scalar (captured intint)

The constant vs widened cases together prove the extension forwards whatever the closure acceptor resolves, not a hard-coded shape.

Task B — document the production illuminate/* dependency rationale (Sapper M1 Finding #4, Form A)

Added a README Production dependencies section (after Type extension) explaining why the illuminate/* chain lives in require, not require-dev: the rules and the type extension reflect against Illuminate contracts/classes at analysis time, so they are genuine analysis-time (runtime-for-the-extension) dependencies; moving them to require-dev would break consumers analysing non-Laravel or partial trees. This is Form A (keep + document), explicitly not Form B (the move that breaks standalone installs).

Gates (local)

  • composer format:check (Pint): pass
  • composer phpstan (self-analysis): pass[OK] No errors
  • composer test (PHPUnit): pass — 85 tests, 131 assertions, 0 deprecations/warnings/risky
  • composer coverage:check: pass — 87.98% ≥ 83 (unchanged by this PR; see note below)
  • composer mutation:ci (Infection): pass — Covered Code MSI 82% ≥ 75 (unchanged; no new source mutants)

symfony/console untouched (held at ^7.2 / installed v7.4.13, per the Infection-vs-Symfony-8 constraint). composer.json / composer.lock unchanged.

Note: TypeInferenceTestCase does not move PCOV line coverage on the extension

The extension still reports 0/12 covered statements in clover (same before and after this PR). TypeInferenceTestCase exercises the extension through PHPStan's own analysis container, which PCOV (measuring the PHPUnit process) does not attribute back to the source file. The test proves the extension works (5 type assertions resolve correctly, and would all fail to mixed without it) but does not register as PCOV line coverage. The coverage gate still passes comfortably on the other [Unreleased] rules' lines.

🤖 Generated with Claude Code

…prod illuminate/* deps

Task A (Quartermaster F-2): add a PHPStan TypeInferenceTestCase giving
ConnectionTransactionReturnTypeExtension direct type-inference coverage. The
extension previously had no direct test — only implicit exercise via
audit-snapshot rule fixtures, none of which asserted the resolved return type.

The new fixture loads extension.neon (the same config consumers register) and
assertType()s the inferred type of $connection->transaction(...) for closures
returning a constant scalar, an object/DTO, a nullable union, an array shape,
and a widened (non-constant) scalar — pinning that the extension forwards the
closure acceptor's return type rather than mixed.

Task B (Sapper M1 Finding #4, Form A): add a README "Production dependencies"
section documenting why the illuminate/* chain stays in require (not
require-dev) — the rules and the type extension reflect against Illuminate
contracts/classes at analysis time, so they are genuine analysis-time deps;
require-dev would break consumers analysing non-Laravel or partial trees.

Both changes are test/docs only — no production-dep or source changes.
CHANGELOG [Unreleased] updated under Added (tests) and Documentation (README).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@Goosterhof Goosterhof added the Agent Review Requested Requesting review of specialized AI review agents. label Jun 2, 2026
Copy link
Copy Markdown
Contributor Author

@Goosterhof Goosterhof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Approve-worthy

0 blockers · 0 concerns · 2 nits · 2 praise

Test-and-docs bundle: a direct TypeInferenceTestCase for ConnectionTransactionReturnTypeExtension (previously only exercised implicitly by audit-snapshot fixtures) plus a README section documenting why the illuminate/* chain lives in require. No source changes, no production-dependency changes — composer.json / composer.lock untouched. The test design is sound and the doc rationale is correct. Approve-worthy; landing as a COMMENT because GitHub blocks self-approval.

Nits

  • tests/Fixtures/ConnectionTransactionReturnType/transaction-return-type.php / README.md:94 — the README enumerates the production chain as database, contracts, cache, filesystem, log, mail but composer.json require also carries psr/log (and nikic/php-parser, phpstan/phpstan themselves). The list is illustrative of the Illuminate subset, which is the point being made, so this is fine — but a reader cross-checking require will find one more */log entry than the prose names. Optional: add "(plus psr/log)" or scope the sentence to "the Illuminate packages."

  • tests/Type/ConnectionTransactionReturnTypeExtensionTest.php:31public function testFileAsserts(string $assertType, string $file, ...$args) leaves $args untyped. This mirrors the upstream TypeInferenceTestCase examples verbatim so it's the conventional shape, but the package self-analyses clean at a strict level; if a future PHPStan tightening flags the variadic, an mixed ...$args annotation closes it. Not worth a change now.

Praise

  • The constant-vs-widened pairing (fn (): int => 4242 alongside captured fn (): int => $valueint) is the load-bearing decision here. It's the difference between a test that proves "the extension returns something int-shaped" and one that proves "the extension forwards whatever the closure acceptor resolves, constant-folded or not." A single happy-path assertion would have passed against a hard-coded return type; this pair would not.

  • getAdditionalConfigFiles() loading the real extension.neon rather than hand-registering the extension means the test exercises the exact registration consumers get — including the phpstan.broker.dynamicMethodReturnTypeExtension tag wiring, not just the class in isolation. That's the registration surface most likely to silently rot.

One honest note carried in the PR body — that TypeInferenceTestCase does not move PCOV line coverage on the extension because the source runs inside PHPStan's analysis container, not the PHPUnit process — is the right disclosure to make rather than claiming a coverage win the clover report won't show. The five assertType()s are the real proof of life; the 0/12 clover figure is a measurement artifact, correctly labeled as such.

Automated war-room agent review — posted because this PR carries the Agent Review Requested label.

@jasperboerhof
Copy link
Copy Markdown
Contributor

PR Reviewer · claimed

@jasperboerhof
Copy link
Copy Markdown
Contributor

PR Reviewer · claimed

@jasperboerhof
Copy link
Copy Markdown
Contributor

PR Reviewer · 9/10 · PASS

phpstan-warroom-rules #31 · AC anchor: none
Scores: acceptance 9 · simplicity 9 · surface 9 · silent-failure – · efficiency –

No findings — all reviewers clean.

Action

merge-ready

Copy link
Copy Markdown
Contributor

@jasperboerhof jasperboerhof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-approved — review verdict is PASS, CI is green, and no human blocker is outstanding. See the verdict comment for the breakdown.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Agent Review Requested Requesting review of specialized AI review agents.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants