Release 5.0.0#48
Open
turegjorup wants to merge 30 commits into
Open
Conversation
The public exception contract becomes a marker interface — `\ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface extends \Throwable` — instead of the abstract `ItkOpenIdConnectException` class. Every concrete exception is re-parented to the SPL type that best describes its failure category (`\RuntimeException` for transient/data-shape failures; `\LogicException` for programmer/config bugs; `\InvalidArgumentException` for invalid constructor input) and implements the marker. Consumers can now catch every OIDC failure with `catch (OpenIdConnectExceptionInterface $e)` or scope to a specific SPL parent. Concretes no longer extend the abstract `ItkOpenIdConnectException`; the abstract is kept for 5.x as a `@deprecated` alias that still implements the marker, but existing `catch (ItkOpenIdConnectException $e)` blocks will not match anything thrown by 5.0+ code. This is the user-visible BC break behind the MAJOR bump. The README "Exception handling" section walks through the consumer migration. Adds `ConfigurationException` (extending `\InvalidArgumentException`) for missing/invalid constructor options, replacing the raw `\InvalidArgumentException` previously thrown from the constructor. The new type still extends `\InvalidArgumentException`, so existing catches at that level keep matching. Narrows `getIdToken`'s `catch (\Exception $e)` boundary to `IdentityProviderException|ClientExceptionInterface|\JsonException` — the three actually-thrown families — so unexpected failures are no longer silently wrapped as `CodeException`. Cache failures during `getConfiguration` (called for the token-endpoint lookup) now propagate as `CacheException` rather than being re-wrapped. Adds `tests/Exception/ExceptionHierarchyTest.php` to lock the contract: every concrete is verified to implement the marker, extend the correct SPL parent, and be catchable via the marker. Failing this test class is the early warning that the public contract has drifted. Updates the README with a new "Exception handling" subsection (marker interface, SPL parent table, PSR-18 co-implementation note on `HttpException`, and 4.x → 5.0 catch-block migration) and updates the `validateIdToken` example to catch the marker interface. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`@phpstan-ignore deadCode.unreachable` now carries a parenthesized reason (PHPStan's documented format for ignore justifications) so a reviewer can tell at a glance why the suppression exists. The suppressed line is intentionally unreachable in the happy path; it serves as a fail-safe if a future regression breaks the catch-by-marker contract this test verifies. The anonymous `ClientExceptionInterface` implementer in `testGetIdTokenFailure` now has a one-line comment explaining the choice. Guzzle's real exception constructors require a RequestInterface we don't have in a unit test, and any PSR-18 implementer satisfies the contract `getIdToken`'s narrowed catch actually targets. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds PHPStan's `reportIgnoresWithoutComments: true` setting (available in `phpstan/phpstan` 2.1.41+, so bump the require-dev constraint). With this setting active, any `@phpstan-ignore` directive without a parenthesized justification fails CI — turning the documented convention into a hard rule that won't quietly drift back. To make the setting actually fire on the test where the original violation surfaced, `tests/` is added to PHPStan's `paths`. That surfaces 46 pre-existing static-analysis issues unrelated to the contract migration (mostly `string|false` flow into `json_decode`, Mockery stub method typing, and dynamic property access on `\stdClass` claims). Generating `phpstan-baseline.neon` grandfathers those so they don't block this PR while still letting future regressions in test code surface as fresh errors. The baseline is deliberately a temporary measure: it should shrink over time as the test-code static-analysis debt is paid down in follow-up PRs. The alternative (keeping `tests/` out of PHPStan scope) would leave the new ignore-comments rule unenforced in exactly the location where the original reviewer complaint landed — which defeats the point of adding the rule. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Marker-interface exception contract (5.0 — BREAKING)
`$this->provider` is initialized in `setUp()` and never assigned null, so the nullable type was misleading. Dropping the `?` removes 25 of the 46 grandfathered errors in `phpstan-baseline.neon` — every `Cannot call method X() on …|null` was a downstream consequence of that one type. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`shouldReceive(...)->andReturn(...)` is the Mockery fluent API. The return type of `shouldReceive()` is declared as `Mockery\ExpectationInterface|Mockery\HigherOrderMessage`, and the chained methods only exist on `ExpectationInterface` — PHPStan can't tell the call sites are safe without type stubs. `phpstan/phpstan-mockery` is the official PHPStan extension shipping those stubs. Adding it removes 8 entries from `phpstan-baseline.neon`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`validateIdToken()` is declared `: object` in the library — PHPStan
can't see the dynamic `nonce` / `aud` properties of the decoded JWT
payload. Annotate each call site's local `$claims` with an
`object{nonce, aud}` shape so the subsequent property reads type-
check. Removes 5 entries from `phpstan-baseline.neon`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`(string) file_get_contents(...)` silently coerces the `false` returned when a fixture is missing into an empty string, which then flows into `json_decode` and produces `null`. Tests downstream of that report confusing assertion failures instead of "the fixture isn't there". Similarly, `(string) parse_url(...)` masks malformed-URL failures. Replaces the three duplicate fixture loads with a single `loadMockFixture()` helper that uses `assertNotFalse` and `assertIsArray` to fail the test at the actual error boundary. The `parse_url` call adds an `assertIsString` guard for the same reason. Removes 4 entries from `phpstan-baseline.neon`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three small cleanups, each removing one entry from the baseline: - `testCheckResponseSuccess` swaps `assertTrue(true)` (which PHPStan correctly flags as a tautology) for `expectNotToPerformAssertions()`, the PHPUnit idiom for "this test verifies the call doesn't throw". - `testAbstractBaseImplementsMarker` was using a constant-folded `is_subclass_of(...)` call wrapped in `assertTrue`. Replaced with a runtime `ReflectionClass::getInterfaceNames()` check + `assertContains` — PHPStan can't fold the reflection result, and the assertion stays semantically meaningful (catches a future regression that removes the marker from the deprecated abstract). - `MockJWT::$leeway` declares a type (`?int`) so the property satisfies PHPStan's missingType.property rule. Baseline now empty — see follow-up commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The four predecessor commits cleared the 46 grandfathered errors that `phpstan-baseline.neon` was absorbing. With the baseline at zero entries the file serves no purpose, so drop it (and its include line from `phpstan.neon`) rather than keep an empty alias around. PHPStan still runs at level 8 across `src` and `tests` and remains clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- `php-cs-fixer` (@symfony ruleset) flagged a redundant `@param` PHPDoc on a typed parameter in `getMockHttpSuccessResponse`. Auto-fixed. - `composer normalize` reorders `phpstan-mockery` alphabetically within `require-dev` (it landed at the bottom after the manual `composer require`, between `phpunit/phpunit` and nothing). - The `changelog` CI step requires every PR to touch `CHANGELOG.md`. Added a "Tooling" subsection to `[Unreleased]` summarising what this PR's six commits did (PHPStan scope extension, mockery extension install, baseline cleanup). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Clear phpstan-baseline.neon by fixing the underlying test issues
Two `(string)` casts in `OpenIdConfigurationProvider` coerced JSON-decoded mixed values into strings: the JWK `kid` lookup and the OIDC discovery doc value lookup. Both upstream payloads specify the field as a string by spec (RFC 7517 §4.5 for `kid`; OIDC Discovery for the document values), but a malformed-payload case silently turned an int / bool / array into a useless string and produced confusing downstream failures rather than diagnosing the malformed input. Replace each cast with an `is_string` guard that throws an appropriate typed exception (`KeyException` and `CacheException` respectively). Both already implement `OpenIdConnectExceptionInterface`, so consumers catching the marker pick up the new throw paths without code changes. Adds two tests covering the new branches to preserve 100% coverage (151/151 lines). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`getConfiguration` has two distinct failure boundaries:
- `catch (InvalidArgumentException $e) → throw new CacheException(...)`
correctly maps PSR-6 cache-layer failures to `CacheException`.
- The inline `throw` for "required key missing" / "value is not a
string" was *also* using `CacheException`, but those validation
failures fire regardless of whether `$configuration` came from
cache or from a fresh `fetchJsonResource()` call — the IdP-
returned (or previously cached) JSON payload doesn't conform to
the OIDC Discovery schema. Calling that a "Cache" exception
misleads operators looking at logs.
Switch both validation throws to `JsonException`, the existing
concrete the library already uses for "JSON payload from the IdP is
malformed". The new check (added in this PR for the previously-
silent-coerced non-string value case) and the pre-existing
missing-key check now share the same type, since they're the same
failure category.
Both `CacheException` and `JsonException` implement
`OpenIdConnectExceptionInterface`, so consumers catching the marker
are unaffected. Consumers catching `CacheException` specifically for
the missing-key case will need to widen — flagged in the CHANGELOG.
Addresses the review comment on the original throw:
#42 (comment)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`JsonException` stretches its semantic when used for "the JSON parsed fine but the document doesn't conform to the OIDC Discovery spec" — the existing usage of `JsonException` is strictly for `json_decode` failures (the bytes aren't valid JSON). Mixing the two failure modes under one type means a consumer catching `JsonException` to retry on transient parse failures would incorrectly retry a malformed-discovery- document case (the IdP will keep returning the same bad payload — no retry will help). New `MetadataException extends \RuntimeException implements OpenIdConnectExceptionInterface` covers this distinct failure category. The two validation throws in `getConfiguration` (missing required key; non-string value at a required key) switch to it. `fetchJsonResource` still throws `JsonException` for actual parse failures, so the categories stay clean. Updates the `@throws` lists on `getBaseAuthorizationUrl`, `getEndSessionUrl`, `validateIdToken`, and `getConfiguration` to advertise the new transitively-thrown type. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`OpenIdConfigurationProvider::__construct(array $options, array $collaborators)` previously took untyped arrays — every `$options['cacheItemPool']`, `$options['cacheDuration']`, `$collaborators['jwt']`, `$options['openIDConnectMetadataUrl']` read flowed as `mixed` into the typed setters (`setCacheItemPool(CacheItemPoolInterface)`, `setCacheDuration(int)`, `setRequestFactory(RequestFactory)`, `setOpenIDConnectMetadataUrl(string)`), so PHPStan at `level: max` produced 4 `argument.type` errors. Add PHPDoc array shapes that declare the keys we read and their types. Keys are marked optional (`?:`) because the runtime `array_key_exists` + `ConfigurationException` checks still gate them — but their type is narrowed when present so the setter calls type-check. The `...` trailing entry lets the league/oauth2-client extra options (`clientId`, `clientSecret`, `redirectUri`, …) and Guzzle's forwarded options (`timeout`, `proxy`, `verify`) pass through without flagging. Drops a now-redundant `is_int($options['leeway'])` runtime check that became a `function.alreadyNarrowedType` tautology once the shape declared `leeway?: int`. Under `declare(strict_types=1)`, PHP itself enforces the type via `setLeeway(int $leeway)`. Behaviour unchanged — purely a static-analysis tightening at the constructor boundary. Removes 4 errors when PHPStan is run at `level: max`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two methods read fields out of JSON payloads (JWKS, token endpoint response) where PHPStan could only see `mixed`: - `getJwtVerificationKeys` walks `$jwks['keys']` and reads `kid`, `kty`, `e`, `n` on each entry without validating the structure. Spec-compliant payloads from real IdPs work, but a malformed payload silently produces garbage (a `Key` constructed from empty/wrong inputs) or trips a downstream type error in `XMLSecurityKey::convertRSA`. - `getIdToken` reads `$payload['id_token']` without checking that `$payload` is an array or that the field is a string, returning whatever it finds. Each access is now gated by an explicit `is_array` / `is_string` guard at the actual error boundary, throwing the appropriate typed exception with a precise message: - JWKS missing array `keys` → `KeyException`. - JWK entry not a JSON object → `KeyException`. - JWK entry missing string `kty` → `KeyException`. - RSA JWK missing string `e` / `n` → `KeyException`. - Token endpoint response missing string `id_token` → `CodeException`. Adds a `createProviderWithCustomJwks(string $jwksJson)` helper to the test and uses it for the four new JWKS test cases (the existing `testGetJwtVerificationKeysRejectsNonStringKid` / `testGetJwtVerificationKeysUnsupportedKeyType` cases keep their existing inline setup — out of scope to refactor). One additional test covers the new getIdToken throw. 100% coverage maintained (161/161 lines). Removes 7 errors when PHPStan is run at `level: max`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three related changes to the exception layer: 1. **Document every concrete.** Each of the 15 concretes in `src/Exception/` now has a class-level PHPDoc describing what it represents, when it's thrown, the rationale for its SPL parent (`\RuntimeException` / `\LogicException` / `\InvalidArgumentException`), and the boundary against related concrete types — most importantly: `DecodeException` vs `JwksException` (bytes-level vs shape-level failure inside the JWK); `JsonException` vs `MetadataException` (parse failure vs parsed-but-non-conformant document); `JwksException` vs `MetadataException` (JWKS vs OIDC-discovery document, both IdP-side spec violations at different layers); `CodeException` vs `ValidationException` vs `ClaimsException` (different stages of the auth flow: token-exchange vs signature vs claim-value). 2. **Rename `KeyException` → `JwksException`** for symmetry with `MetadataException` and clearer scope: the type fires for both document-level errors (`keys` array missing) and entry-level errors (missing `kid` / `kty` / `e` / `n`). Naming after the document type (JWKS) rather than the individual key is more accurate; matches Symfony's `JwkSet` PascalCase convention. 3. **Narrow JSON payload accesses** in `getJwtVerificationKeys` and `getIdToken`. Each previously-dynamic field read now has an explicit `is_array` / `is_string` guard at the actual error boundary, throwing the appropriate typed exception with a precise message: `JwksException` for malformed JWKS structure; `CodeException` for a token endpoint response missing string `id_token`. Adds a `createProviderWithCustomJwks(string)` test helper used by four new JWKS validation tests plus one inline getIdToken test. 100% coverage maintained (161/161 lines, up from 151). Audit confirms each of the 15 concretes covers a distinct failure category — no two would be handled identically by a reasonable consumer (per ADR 001's granularity rule). The overall count is higher than the ADR's "three to five per package" target; consolidation candidates are flagged via the cross-references but not collapsed (any rename or removal is a deferred MAJOR break). Static analysis: PHPStan at `level: max` drops 7 errors (was 26 on this branch tip, now 19). Remaining 19 covered by PR C. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three small narrowings to clear the remaining `mixed` flow that
PHPStan flags at `level: max`:
- `getJwtVerificationKeys()` declares `@return array<string, Key>`
matching the shape it actually builds. The cache-hit branch's
`(array) $item->get()` is annotated with the same shape via inline
`@var` — we only ever store this structure, so the trust is
consistent with the method's contract.
- `validateIdToken()`'s `$claims` is annotated with a
`\stdClass&object{aud: string|array<string>, iss: string, nonce:
string}` shape after `JWT::decode()`. The fields are required
string-typed claims per the OIDC spec; `firebase/php-jwt` already
enforces JWT validity before this point. No runtime change.
- `testCreateResourceOwner` adds an `assertInstanceOf(...)` guard so
the `$owner->getId()` call type-checks (the value comes back from
`ReflectionMethod::invoke()` which PHPStan sees as `mixed`).
- `loadMockFixture()`'s `@return` is relaxed from
`array<string, mixed>` to `array<mixed>` — `json_decode($content,
true)` doesn't statically guarantee string keys, and the callers
cast/narrow as needed for their specific fixture.
- 11 `$mockJWT = \Mockery::mock('overload:...')` sites in the test
file get an inline `@var \Mockery\MockInterface` annotation. The
`overload:` mock-syntax isn't recognised by `phpstan/phpstan-mockery`
(overload mocks are special-cased to never instantiate the target
class, so the extension's class-resolution doesn't fire); the
annotation tells PHPStan the value is a `MockInterface` so the
chained `shouldReceive()->...` calls type-check.
After this PR rebases onto develop (post-PR #43 / PR A merge), all
max-level errors are gone. The `phpstan.neon` `level: 8` → `level:
max` bump is left as a follow-up one-line PR so the bump and its
prerequisites land independently.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace (string) casts on JSON payload values with is_string guards
PR A: Type constructor $options / $collaborators array shapes
PR B: Document and tighten the exception system; narrow JSON payload accesses
`[Unreleased]` had two `### Documentation` subsections — one added in PR #44 for the README "Exception handling" section, one added later for the per-class PHPDoc audit. markdownlint flags duplicate sibling headings (MD024) and the CI step was failing. Merge the README bullet into the per-class section so there's a single `### Documentation` heading. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR C: Narrow remaining claim accesses and test types for PHPStan max
A few docblocks in `OpenIdConfigurationProvider` and the test suite
had their descriptions wrapped onto a continuation `*` line:
* @param int $length
* Length of the random string to be generated
*
* @return string
* The generated state
The `phpdoc_align: vertical` rule from the @symfony preset doesn't
create those wraps — it just aligns whatever multi-line structure
exists in the source. Flattening each description back onto its
`@param` / `@return` line lets the rule pad the columns into a
tidy table instead:
* @param int $length Length of the random string to be generated
* @return string The generated state
No `.php-cs-fixer.dist.php` change: the @symfony default produces
the clean form once the source isn't pre-wrapped.
Also consolidates two `### Documentation` subsections under
`[Unreleased]` (left over from PR #44) into one — markdownlint's
MD024 was flagging the duplicate heading.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collapse multi-line @param / @return descriptions onto single lines
The 5.0 series of PRs cleared every max-level error in turn: - #43 typed the constructor `$options` / `$collaborators` array shapes (4 errors). - #42 + #44 narrowed JSON payload accesses and introduced `MetadataException` / `JwksException` for malformed IdP payloads (~9 errors). - #45 declared `getJwtVerificationKeys` as `array<string, Key>`, annotated `validateIdToken`'s `$claims` with a `\stdClass & object{aud, iss, nonce}` shape, and added Mockery overload `@var` annotations on every test mock (the remaining ~7 errors). With zero max-level errors remaining, this commit flips the one line in `phpstan.neon`. Future contributions are now analyzed at PHPStan's strictest level; any regression that reintroduces a `mixed` flow will fail CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bump PHPStan to level: max
Roll the Unreleased section into a tagged [5.0.0] entry dated 2026-05-12 and add `UPGRADE-5.0.md` covering the consumer-visible migration: catch the new `OpenIdConnectExceptionInterface` marker instead of the deprecated `ItkOpenIdConnectException` abstract, the constructor's switch to typed `ConfigurationException` (still under `\InvalidArgumentException` — existing SPL catches keep matching), the new typed throws on malformed JWKS / discovery / token payloads, `getIdToken`'s narrowed boundary catch, and the catch-on-SPL semantic change. Tightened the changelog to the net 4.1.2 → 5.0.0 deltas. Items that were ephemeral inside the 5.0 PR series — the `phpstan-baseline.neon` introduced and removed within the cycle, and the `phpdoc_align: vertical` source-style flatten with no config change — are omitted since they aren't visible from the release boundary. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #48 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 62 71 +9
===========================================
Files 1 1
Lines 172 185 +13
===========================================
+ Hits 172 185 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jekuaitk
approved these changes
May 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
5.0.0 reworks the exception hierarchy and tightens IdP-payload validations. Runtime behaviour is unchanged for spec-compliant IdPs — every break is at the exception/catch contract level. See
UPGRADE-5.0.mdfor the consumer migration guide.Breaking
OpenIdConnectExceptionInterface(new marker). Concrete exceptions extend SPL types (\RuntimeException/\LogicException/\InvalidArgumentException) directly — not the deprecatedItkOpenIdConnectExceptionabstract. Existingcatch (ItkOpenIdConnectException $e)blocks no longer match.KeyException→JwksException(rename).ConfigurationException(still under\InvalidArgumentException).JwksException/MetadataException/CodeException) instead of silent(string)coercion.getIdTokenboundary catch narrowed from\ExceptiontoIdentityProviderException|ClientExceptionInterface|\JsonException; upstreamCacheException/HttpException/MetadataExceptionnow propagate as themselves.Added
OpenIdConnectExceptionInterfacemarker +ConfigurationException+MetadataException.tests/Exception/ExceptionHierarchyTest.phplocks the contract.Tooling
level: 8→max, pathssrc→src + tests,reportIgnoresWithoutComments: true,phpstan/phpstan-mockeryadded torequire-dev.Test plan
task test:matrixgreen across PHP 8.3 / 8.4 / 8.5 × stable / lowesttask analyze:php(PHPStan level max) cleantask lintclean (php-cs-fixer, composer normalize, markdownlint, prettier)5.0.0, back-merge todevelop🤖 Generated with Claude Code