feat(Http): #[Version] attribute for URL-versioned routes + Deprecation middleware#68
Merged
Conversation
…on middleware
Recovers the convention router's `/v{N}/{controller}/{action}` URL
shape's value (automatic URL-versioned routing) without the
convention router. Apps declare versions via attribute; the Scanner
prefixes the route's path during registration.
#[Route('GET', '/products/{id:int}')]
#[Version('v1')]
public function showV1(int $id): array { /* … */ }
#[Route('GET', '/products/{id:int}')]
#[Version('v2')]
public function showV2(int $id): array { /* … */ }
After Scanner::register():
GET /v1/products/42 → showV1
GET /v2/products/42 → showV2
Class-level `#[Version]` applies to every route in the class;
method-level wins when both are present. routes:check sees
/v1/... and /v2/... as distinct paths and won't flag them.
- Rxn\Framework\Http\Attribute\Version: TARGET_METHOD | TARGET_CLASS.
`version` is the API label ('v1' / '1.0' / '2025-10-15');
Scanner adds the slash. Optional deprecatedAt / sunsetAt take
any DateTimeImmutable-parseable date string.
- Rxn\Framework\Http\Versioning\Deprecation: PSR-15 middleware
emitting RFC 8594 Deprecation: / Sunset: response headers as
IMF-fixdate. Unparseable inputs silently dropped — the contract
is best-effort signalling, not "die on bad config."
- Scanner integration: when an effective Version carries
deprecation metadata, the Scanner auto-attaches the Deprecation
middleware. No per-handler header boilerplate.
- Path-prefix idempotence: Scanner detects routes already
prefixed with the version (hand-written `/v1/old` +
`#[Version('v1')]`) and doesn't double-prefix to `/v1/v1/old`.
- 9 Scanner tests + 8 Deprecation middleware tests.
- docs/routing.md: new "API versioning — #[Version]" section.
- README: novelty bullet for "API versioning as a primitive."
- .gitignore: /.claude/ tooling artifacts.
Suite 618 → 635 / 1329 → 1368.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ce27a3dc6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Pull request overview
Adds first-class URL-based API versioning to the attribute-driven router by introducing a #[Version] attribute that prefixes registered route paths (e.g. /v1/...), plus an optional deprecation signalling middleware that emits RFC 8594 Deprecation / Sunset headers.
Changes:
- Introduces
Http\Attribute\Versionand extendsHttp\Attribute\Scannerto prefix route paths based on class/method version attributes (method-level overrides class-level). - Adds
Http\Versioning\DeprecationPSR-15 middleware and auto-attaches it whendeprecatedAt/sunsetAtare provided on#[Version]. - Adds new unit/integration tests and updates docs/README/CHANGELOG to document the feature.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Rxn/Framework/Http/Attribute/Scanner.php | Adds #[Version] support: path prefixing + optional auto-attached Deprecation middleware |
| src/Rxn/Framework/Http/Attribute/Version.php | New #[Version] attribute value object (method/class targets) |
| src/Rxn/Framework/Http/Versioning/Deprecation.php | New middleware emitting RFC 8594 Deprecation / Sunset headers |
| src/Rxn/Framework/Tests/Http/Versioning/DeprecationTest.php | Unit tests for date parsing/formatting and header emission behavior |
| src/Rxn/Framework/Tests/Http/Attribute/VersionScannerTest.php | Integration tests for Scanner version prefixing + middleware attachment |
| src/Rxn/Framework/Tests/Http/Attribute/Fixture/VersionedController.php | Fixture controller for method-level versioning tests |
| src/Rxn/Framework/Tests/Http/Attribute/Fixture/DeprecatedVersionController.php | Fixture controller for deprecated version middleware attachment tests |
| src/Rxn/Framework/Tests/Http/Attribute/Fixture/ClassVersionedController.php | Fixture controller for class-level versioning and method override tests |
| docs/routing.md | Documents #[Version] usage and deprecation signaling |
| README.md | Adds feature bullet and updates test counts |
| CHANGELOG.md | Adds “Unreleased” entry describing the new versioning primitive |
| .gitignore | Ignores /.claude/ |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Scanner now PREPENDS the auto-attached Deprecation middleware
to the route's stack instead of appending. Pipeline runs
middleware in registration order, so the first one wraps every
later one — Deprecation has to be outermost so it decorates
short-circuit responses too. Without the fix, an inner auth
middleware returning 401 (or rate-limit returning 429) would
bypass the Deprecation/Sunset headers entirely; clients hitting
a deprecated endpoint with a missing token wouldn't learn the
endpoint is on its way out.
- Scanner::prefixWithVersion() now `trim($version, '/')`s both
ends, not just leading. Stray-slash labels — `'v1/'`,
`'/v1/'` — used to yield `/v1//products` (double-slash; Router
treated the empty segment as literal and the route became
unmatchable). Empty trimmed labels (`#[Version('')]`) now
fail loud at scan time with InvalidArgumentException.
- Http\Attribute\Version docstring claimed deprecation dates
must be "ISO 8601 date strings". The middleware actually
accepts anything DateTimeImmutable can parse (bare dates, full
ISO with timezone, RFC 7231 IMF-fixdate). Updated the
docstring to match.
New test cases (3 in VersionScannerTest):
- Deprecation middleware decorates a short-circuit 401 response
end-to-end (drives the route's middleware chain through
Pipeline + a stub terminal that never runs).
- Stray-slash version labels — 'v1', '/v1', 'v1/', '/v1/' — all
produce the same /v1 prefix; eval'd controllers cover all four.
- Empty `#[Version('')]` is rejected with a clear message.
Plus one fixture middleware (AlwaysReject) standing in for an
auth-style 401 short-circuit.
Suite 635 → 638 / 1368 → 1380.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Owner
Author
|
@copilot apply changes based on the comments in this thread |
- toImfFixdate(): guard against empty/whitespace-only strings before DateTimeImmutable construction (empty string parses as 'now', which would emit a moving timestamp instead of being dropped) - VersionScannerTest: remove dead $controllers array and foreach loop that was never used (actual test cases driven by eval'd $cases loop) Agent-Logs-Url: https://github.com/davidwyly/rxn/sessions/0a276a00-b476-4ee8-8b5d-a03cdd0e0816 Co-authored-by: davidwyly <13552783+davidwyly@users.noreply.github.com>
Contributor
Done in commit
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The README/CHANGELOG/Version-docstring all promised that
`bin/rxn routes:check` treats `/v1/...` and `/v2/...` as
distinct paths. Three of the four unresolved threads pointed
out that the runtime promise wasn't met — `Routing\ConflictDetector::collect()`
only reflected `#[Route]` and didn't know about `#[Version]`,
so two methods with the same `#[Route('/products/{id:int}')]`
but different `#[Version('v1')]` / `#[Version('v2')]` would
still be flagged as conflicts.
This commit makes the docs honest:
- New `Version::applyTo(string $path): string` — single source
of truth for "what URL does this versioned route actually
register at." The `Scanner` and `ConflictDetector` both call
it, so runtime registration and static-analysis detection
stay in lockstep on path computation.
- `ConflictDetector::collect()` reads `#[Version]` (method-level
with class-level fallback, matching Scanner's resolution rule)
and applies the prefix during entry construction.
- Three new ConflictDetector tests + one fixture
(VersionedConflictController):
- collect() applies the version prefix
- cross-version same-pattern routes are NOT flagged
- same-version same-pattern routes ARE flagged (version
prefixing isn't an escape hatch from the detector's job)
Plus the fourth thread (Copilot, VersionScannerTest line 224):
- The eval'd controller classes in
`testVersionLabelTolerantOfStraySlashes()` now declare an
explicit `namespace ...;` inside the eval'd source. Without
it, PHP put the classes in the global namespace, and passing
the bare name to `scan()` happened to work via PHP's global-
fallback class lookup but read confusingly. Test now uses an
FQCN consistently and asserts class-existence before
scanning.
Scanner cleanup:
- The path-prefixing helpers `prefixWithVersion()` /
`hasDeprecation()` were duplicated between Scanner and the
new ConflictDetector path; pulled the prefix logic onto
Version itself (`Version::applyTo()`). Scanner now calls
`$effectiveVersion->applyTo(...)` instead of a private static.
Suite 638 → 641 / 1380 → 1389. Lint clean, composer valid.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Owner
Author
|
@copilot apply changes based on the comments in this thread |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
#[Route('/')] + #[Version('v1')] concatenated to '/v1/' — Router::compile
normalises that for matching but Route::$pattern and ConflictDetector
output retained the trailing slash. Special-case path === '/' to return
the bare prefix '/v1'. Adds testRootPathDoesNotGetTrailingSlash to cover
the regression.
Agent-Logs-Url: https://github.com/davidwyly/rxn/sessions/b8733f25-c2b2-422d-afb6-1d4a510decd6
Co-authored-by: davidwyly <13552783+davidwyly@users.noreply.github.com>
Contributor
Done in commit
The README numbers ( |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
`Version::applyTo('/')` + version `'v1'` was producing `/v1/`,
which Router::compile would normalise away during match — but
the stored Route::pattern would carry the trailing slash forever.
That meant Router::url() output and ConflictDetector reports
would surface `/v1/` while the runtime matched `/v1`. Two URLs
for the same logical route is the kind of inconsistency that
bites later (e.g., generated client SDKs picking the wrong form).
Special-cased $path === '/' / '' to return just the prefix.
New test (testRootPathPlusVersionDoesNotProduceTrailingSlash)
locks the contract by registering #[Route('/')] + #[Version('v1')]
and asserting both that /v1 matches AND that the stored pattern
is the canonical form, not /v1/.
Also tightened the empty-version exception message:
`"#[Version] label cannot be empty (got '$this->version')"` →
`"... (got '{$this->version}')"`. PHP's simple-syntax string
interpolation handles `$this->prop` correctly, but the explicit
brace form is unambiguous and harder to misread.
Suite 641 → 642 / 1389 → 1391. Test count breakdown: 13 Scanner
integration + 8 Deprecation middleware + 3 ConflictDetector.
CHANGELOG bumped to match.
…o feat/version-attribute # Conflicts: # src/Rxn/Framework/Http/Attribute/Version.php # src/Rxn/Framework/Tests/Http/Attribute/VersionScannerTest.php
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
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
Recovers the convention router's
/v{N}/{controller}/{action}value (automatic URL-versioned routing) without the convention router. Apps declare versions via attribute; the Scanner prefixes the route's path during registration.```php
class ProductsController
{
#[Route('GET', '/products/{id:int}')]
#[Version('v1')]
public function showV1(int $id): array { /* … */ }
}
```
After
Scanner::register():GET /v1/products/42→showV1GET /v2/products/42→showV2Class-level
#[Version]applies to every route in the class; method-level wins when both are present. `bin/rxn routes:check` (now also version-aware viaConflictDetector) sees/v1/...and/v2/...as distinct paths and won't flag them as conflicts. Same-version same-pattern is still flagged as the real ambiguity it is.What's in the PR
Http\Attribute\VersionTARGET_METHOD | TARGET_CLASS.Version::applyTo(string $path): stringis the single source of truth for path computation, called by both Scanner and ConflictDetector. OptionaldeprecatedAt/sunsetAttake anyDateTimeImmutable-parseable string.Http\Versioning\DeprecationDeprecation:/Sunset:headers as IMF-fixdate. Best-effort: empty / unparseable inputs silently dropped.Http\Attribute\Scanner#[Version]on method/class; prepends auto-attachedDeprecationmiddleware so it wraps the whole route stack (decorates short-circuit responses too). Path-prefix idempotent; stray-slash version labels normalised; empty labels rejected.Http\Routing\ConflictDetectorcollect()applies the same prefix the Scanner would, soroutes:checkdoesn't false-positive on cross-version routes.docs/routing.md#[Version]" section with deprecation exampleREADME.mdTest plan