Skip to content

Commit 01697c8

Browse files
authored
Merge pull request #40 from itk-dev/feature/exception-contract-phpstan-rules
Lock the exception contract with custom PHPStan rules
2 parents 05b5375 + 2b00dec commit 01697c8

7 files changed

Lines changed: 374 additions & 0 deletions

CHANGELOG.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
3333

3434
- `ItkDev\OpenIdConnectBundle\Exception\OpenIdConnectBundleExceptionInterface`
3535
marker for catching all bundle-thrown OIDC failures.
36+
- Custom PHPStan rules (`ThrownExceptionImplementsBundleMarker`,
37+
`WrappedExceptionChainsPrevious`) that lock the exception contract on every
38+
CI run — thrown exceptions must implement the marker (with documented
39+
controller/authenticator carve-outs), and wraps inside a catch must chain
40+
the caught cause as `$previous`.
3641

3742
### Changed
3843

composer.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@
4444
},
4545
"autoload-dev": {
4646
"psr-4": {
47+
"ItkDev\\OpenIdConnectBundle\\PHPStan\\": "phpstan/",
4748
"ItkDev\\OpenIdConnectBundle\\Tests\\": "tests/"
4849
}
4950
},

phpstan.neon

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,14 @@ includes:
55
- vendor/phpstan/phpstan-phpunit/rules.neon
66
- vendor/phpstan/phpstan-symfony/extension.neon
77
- vendor/phpstan/phpstan-symfony/rules.neon
8+
- phpstan/exception-contract.neon
89

910
parameters:
1011
level: max
1112
paths:
1213
- src
1314
- tests
15+
- phpstan/Rule
1416
reportIgnoresWithoutComments: true
1517

1618
ignoreErrors:

phpstan/README.md

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
# PHPStan exception-contract rules
2+
3+
Two custom PHPStan rules lock the exception contract documented in
4+
[ADR 001](../docs/adr/001-marker-interface-exception-hierarchy.md). They run
5+
automatically as part of `task analyze:php` (and therefore `task pr:actions`).
6+
7+
## `ThrownExceptionImplementsBundleMarker`
8+
9+
Every literal `throw new X(...)` site under `src/` must throw an exception
10+
that implements:
11+
12+
- `ItkDev\OpenIdConnectBundle\Exception\OpenIdConnectBundleExceptionInterface`, or
13+
- `ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface` (the upstream marker).
14+
15+
Two framework-boundary carve-outs apply, matching ADR 001:
16+
17+
| Path | Additionally allowed | Why |
18+
| ----------------------------------- | ----------------------------------------------------------------------- | ------------------------------------------------------------------ |
19+
| `src/Controller/*` | `Symfony\Component\HttpKernel\Exception\HttpExceptionInterface` | Symfony's kernel renders the HTTP status off these. |
20+
| `src/Security/*Authenticator.php` | `Symfony\Component\Security\Core\Exception\AuthenticationException` | Symfony Security catches these to render the auth-failure response.|
21+
22+
Escape hatch for an intentional outlier:
23+
24+
```php
25+
// @phpstan-ignore throw.notMarkerInterface
26+
throw new MyOddException(...);
27+
```
28+
29+
Pair the annotation with a one-line justification comment.
30+
31+
## `WrappedExceptionChainsPrevious`
32+
33+
Any `throw new Y(...)` inside a `catch (... $e)` block must pass `$e`
34+
as the new exception's `$previous`, either as a named argument
35+
(`previous: $e`) or as a direct positional argument value. This preserves
36+
`getPrevious()` traversal for logs and debugging — the lesson from the
37+
4.1 → 4.2 bug-fix PR that surfaced four `CliLoginHelper` wrap sites
38+
discarding the original cause.
39+
40+
The check is intentionally lenient: it accepts any positional argument
41+
whose value is the catch variable, not just the 3rd slot. This covers
42+
Symfony's HTTP exception subclasses (which bake the status into the
43+
class and place `$previous` at index 1) without needing constructor
44+
reflection.
45+
46+
Escape hatch when the rethrow is intentionally unrelated to the cause:
47+
48+
```php
49+
} catch (\Throwable $e) {
50+
// @phpstan-ignore throw.unchainedPrevious
51+
throw new UnrelatedDomainException('...');
52+
}
53+
```
54+
55+
Annotate with a justification.
56+
57+
## Verifying the rules
58+
59+
The rules are exercised on every CI run because PHPStan analyses `src/`
60+
and `tests/`. If a refactor produces a contract violation, the build
61+
breaks — which is the point. To verify a rule manually after editing,
62+
clear the cache and re-run:
63+
64+
```shell
65+
docker compose exec phpfpm vendor/bin/phpstan clear-result-cache
66+
task analyze:php
67+
```
68+
69+
## Why not php-cs-fixer
70+
71+
php-cs-fixer's rule model is syntactic. Exception-type enforcement is
72+
a type-level concern — what a thrown class extends or implements — and
73+
PHPStan has the reflection machinery to express it. Future work could
74+
add a php-cs-fixer rule for the narrower "no `throw new \Exception(…)`
75+
anywhere" check, but the inheritance graph belongs to PHPStan.
Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
<?php
2+
3+
namespace ItkDev\OpenIdConnectBundle\PHPStan\Rule;
4+
5+
use ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface;
6+
use ItkDev\OpenIdConnectBundle\Exception\OpenIdConnectBundleExceptionInterface;
7+
use PhpParser\Node;
8+
use PhpParser\Node\Expr\New_;
9+
use PhpParser\Node\Expr\Throw_;
10+
use PHPStan\Analyser\Scope;
11+
use PHPStan\Reflection\ReflectionProvider;
12+
use PHPStan\Rules\Rule;
13+
use PHPStan\Rules\RuleErrorBuilder;
14+
use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface;
15+
use Symfony\Component\Security\Core\Exception\AuthenticationException;
16+
17+
/**
18+
* Every exception thrown from `src/` must implement the bundle or library marker.
19+
*
20+
* Two framework-boundary carve-outs (documented in ADR 001):
21+
*
22+
* - HTTP controllers may throw Symfony `HttpExceptionInterface` subclasses; the
23+
* kernel reads the status code off them to render the response.
24+
* - Authenticators may throw Symfony `AuthenticationException` subclasses; the
25+
* security framework catches those to render the failure response.
26+
*
27+
* Escape hatch: per-line "phpstan-ignore throw.notMarkerInterface" with a
28+
* justification comment.
29+
*
30+
* @implements Rule<Throw_>
31+
*/
32+
final class ThrownExceptionImplementsBundleMarker implements Rule
33+
{
34+
public function __construct(
35+
private readonly ReflectionProvider $reflectionProvider,
36+
) {
37+
}
38+
39+
public function getNodeType(): string
40+
{
41+
return Throw_::class;
42+
}
43+
44+
public function processNode(Node $node, Scope $scope): array
45+
{
46+
$file = $scope->getFile();
47+
48+
// Only enforce inside src/ — tests, fixtures, and tooling are not part of the contract.
49+
if (!str_contains($file, '/src/')) {
50+
return [];
51+
}
52+
53+
$thrownExpr = $node->expr;
54+
if (!$thrownExpr instanceof New_) {
55+
// `throw $variable` or `throw functionCall()`: type may still be checked elsewhere,
56+
// but this rule only inspects literal `throw new X(...)` sites.
57+
return [];
58+
}
59+
60+
$classNode = $thrownExpr->class;
61+
if (!$classNode instanceof Node\Name) {
62+
return [];
63+
}
64+
65+
$className = $scope->resolveName($classNode);
66+
if (!$this->reflectionProvider->hasClass($className)) {
67+
return [];
68+
}
69+
70+
$reflection = $this->reflectionProvider->getClass($className);
71+
$inController = str_contains($file, '/src/Controller/');
72+
$inAuthenticator = str_contains($file, '/src/Security/') && str_ends_with($file, 'Authenticator.php');
73+
74+
if (
75+
$reflection->implementsInterface(OpenIdConnectBundleExceptionInterface::class)
76+
|| $reflection->implementsInterface(OpenIdConnectExceptionInterface::class)
77+
) {
78+
return [];
79+
}
80+
81+
if ($inController && $reflection->implementsInterface(HttpExceptionInterface::class)) {
82+
return [];
83+
}
84+
85+
if (
86+
$inAuthenticator
87+
&& (
88+
AuthenticationException::class === $reflection->getName()
89+
|| $reflection->isSubclassOfClass($this->reflectionProvider->getClass(AuthenticationException::class))
90+
)
91+
) {
92+
return [];
93+
}
94+
95+
$expected = match (true) {
96+
$inController => sprintf(
97+
'%s, %s, or %s (controller carve-out)',
98+
OpenIdConnectBundleExceptionInterface::class,
99+
OpenIdConnectExceptionInterface::class,
100+
HttpExceptionInterface::class,
101+
),
102+
$inAuthenticator => sprintf(
103+
'%s, %s, or %s (authenticator carve-out)',
104+
OpenIdConnectBundleExceptionInterface::class,
105+
OpenIdConnectExceptionInterface::class,
106+
AuthenticationException::class,
107+
),
108+
default => sprintf('%s or %s', OpenIdConnectBundleExceptionInterface::class, OpenIdConnectExceptionInterface::class),
109+
};
110+
111+
return [
112+
RuleErrorBuilder::message(sprintf(
113+
'Thrown exception %s does not satisfy the bundle exception contract; expected an implementation of %s. See docs/adr/001-marker-interface-exception-hierarchy.md.',
114+
$className,
115+
$expected,
116+
))
117+
->identifier('throw.notMarkerInterface')
118+
->build(),
119+
];
120+
}
121+
}
Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,161 @@
1+
<?php
2+
3+
namespace ItkDev\OpenIdConnectBundle\PHPStan\Rule;
4+
5+
use PhpParser\Node;
6+
use PhpParser\Node\Arg;
7+
use PhpParser\Node\Expr\New_;
8+
use PhpParser\Node\Expr\Throw_;
9+
use PhpParser\Node\Expr\Variable;
10+
use PhpParser\Node\Stmt\Catch_;
11+
use PHPStan\Analyser\Scope;
12+
use PHPStan\Rules\Rule;
13+
use PHPStan\Rules\RuleErrorBuilder;
14+
15+
/**
16+
* When a catch block rethrows by constructing a new exception, the caught
17+
* exception must be chained as `$previous` (positional 3rd argument or named
18+
* `previous:`). This enforces the "wrap at the boundary" rule in ADR 001,
19+
* preserving `getPrevious()` traversal for logs and debugging.
20+
*
21+
* Escape hatch: add "phpstan-ignore throw.unchainedPrevious" on the throw line
22+
* with a justification comment when the rethrow is intentionally unrelated to
23+
* the caught cause.
24+
*
25+
* @implements Rule<Catch_>
26+
*/
27+
final class WrappedExceptionChainsPrevious implements Rule
28+
{
29+
public function getNodeType(): string
30+
{
31+
return Catch_::class;
32+
}
33+
34+
public function processNode(Node $node, Scope $scope): array
35+
{
36+
if (null === $node->var) {
37+
return [];
38+
}
39+
40+
$catchVarName = $node->var->name;
41+
if (!is_string($catchVarName)) {
42+
return [];
43+
}
44+
45+
$errors = [];
46+
foreach ($this->findThrows($node->stmts) as $throw) {
47+
$newExpr = $throw->expr;
48+
if (!$newExpr instanceof New_) {
49+
continue;
50+
}
51+
52+
if ($this->chainsCaughtAsPrevious($newExpr, $catchVarName)) {
53+
continue;
54+
}
55+
56+
$thrownLabel = $this->describeNewExpr($newExpr);
57+
$errors[] = RuleErrorBuilder::message(sprintf(
58+
'Exception %s thrown inside catch($%s) does not chain the caught exception as `previous`. Pass `previous: $%s` (or as the 3rd positional argument), or annotate with `@phpstan-ignore throw.unchainedPrevious` when the unrelated throw is intentional.',
59+
$thrownLabel,
60+
$catchVarName,
61+
$catchVarName,
62+
))
63+
->identifier('throw.unchainedPrevious')
64+
->line($throw->getStartLine())
65+
->build();
66+
}
67+
68+
return $errors;
69+
}
70+
71+
/**
72+
* Collect every `throw` node reachable from these statements, but stop at
73+
* nested catch blocks — those bind a different catch variable and are
74+
* inspected by their own rule invocation.
75+
*
76+
* @param Node[] $stmts
77+
*
78+
* @return list<Throw_>
79+
*/
80+
private function findThrows(array $stmts): array
81+
{
82+
$found = [];
83+
foreach ($stmts as $stmt) {
84+
$this->collect($stmt, $found);
85+
}
86+
87+
return $found;
88+
}
89+
90+
/**
91+
* @param list<Throw_> $found
92+
*/
93+
private function collect(Node $node, array &$found): void
94+
{
95+
if ($node instanceof Catch_) {
96+
// A nested catch owns its own catch variable scope; its rethrows are
97+
// checked when that catch is processed.
98+
return;
99+
}
100+
101+
if ($node instanceof Throw_) {
102+
$found[] = $node;
103+
}
104+
105+
foreach ($node->getSubNodeNames() as $subName) {
106+
$sub = $node->{$subName}; // @phpstan-ignore property.dynamicName (walking an AST node's children requires a dynamic subnode name lookup; the names come from getSubNodeNames() and are intrinsically dynamic)
107+
if ($sub instanceof Node) {
108+
$this->collect($sub, $found);
109+
} elseif (is_array($sub)) {
110+
foreach ($sub as $item) {
111+
if ($item instanceof Node) {
112+
$this->collect($item, $found);
113+
}
114+
}
115+
}
116+
}
117+
}
118+
119+
/**
120+
* Accept either a named `previous: $catchVar` argument, or any argument whose
121+
* value is directly the catch variable. The latter covers conventional PHP
122+
* exceptions (positional $previous at index 2) and Symfony's HTTP exception
123+
* subclasses (which bake the status code into the type and place $previous
124+
* at index 1). Passing the catch variable as any other slot is unusual in
125+
* practice; the looser check trades a rare false-negative for not depending
126+
* on constructor reflection.
127+
*/
128+
private function chainsCaughtAsPrevious(New_ $new, string $catchVarName): bool
129+
{
130+
foreach ($new->args as $arg) {
131+
if (!$arg instanceof Arg) {
132+
continue;
133+
}
134+
135+
if (null !== $arg->name && 'previous' === $arg->name->name) {
136+
return $this->valueIsVariable($arg->value, $catchVarName);
137+
}
138+
139+
if (null === $arg->name && $this->valueIsVariable($arg->value, $catchVarName)) {
140+
return true;
141+
}
142+
}
143+
144+
return false;
145+
}
146+
147+
private function valueIsVariable(Node\Expr $expr, string $name): bool
148+
{
149+
return $expr instanceof Variable && is_string($expr->name) && $expr->name === $name;
150+
}
151+
152+
private function describeNewExpr(New_ $new): string
153+
{
154+
$cls = $new->class;
155+
if ($cls instanceof Node\Name) {
156+
return $cls->toString();
157+
}
158+
159+
return 'anonymous/dynamic exception';
160+
}
161+
}

0 commit comments

Comments
 (0)