feat(Http): CRUD scaffolding via Resource handlers (theme 1.6, core primitive)#69
Conversation
…rimitive)
Recovers the convention router's "extend a class, get five
endpoints" ergonomic without legacy AR baggage.
ResourceRegistrar::register(
$router,
'/products',
new ProductsCrud($repo),
create: CreateProduct::class,
update: UpdateProduct::class,
search: SearchProducts::class,
);
After registration the router carries the documented five-route
family with DTO binding + validation + status-code mapping
already wired:
POST /products create($dto) 201 / 422
GET /products search($filter) 200 (filter optional)
GET /products/{id:int} read($id) 200 / 404
PATCH /products/{id:int} update($id, $dto) 200 / 404 / 422
DELETE /products/{id:int} delete($id) 204 (empty body) / 404
Storage-agnostic: handler implements 5 methods over CrudHandler
interface. Apps using davidwyly/rxn-orm will get a base class
that reduces a relational handler to "extend a class, set TABLE
constant, done"; that ships in the rxn-orm repo as a follow-up.
Apps using Doctrine / raw PDO / remote APIs write their own ~50
LOC handler.
idType arg controls both the URL constraint AND the handler's id
type: default 'int' (cast to int before the handler sees it),
'uuid' / 'slug' / 'any' / custom keep the captured string.
Validation failures surface as 422 application/problem+json with
errors[] (the framework's standard RFC 7807 shape). DELETE
returns a true PSR-7 204 with empty body, bypassing the array
envelope mapper since 204 must have no content per HTTP spec.
Added:
- Rxn\Framework\Http\Resource\CrudHandler interface (5 methods)
- Rxn\Framework\Http\Resource\ResourceRegistrar::register() static
- 15 integration tests against an in-memory fixture covering
registration shape, idType=int reject, create round-trip, 422
on bad body, read 200/404, update partial merge, update 404 /
422, delete 204 (empty body) / 404, search 200 / list / filter
from query / 422 on bad query, null-search-DTO calls handler
with null filter, idType=uuid produces string IDs at the
handler.
Out of scope (deliberate, follow-ups):
- RxnOrmCrudHandler base in davidwyly/rxn-orm
- bin/rxn scaffold:from-table codegen step
- #[Resource] attribute for Scanner-driven discovery
Doc updates:
- docs/horizons.md theme 1.6 marked REALIZED (core primitive),
with explicit "what's left" pointing at the rxn-orm and
scaffold codegen follow-ups.
- docs/routing.md: new "CRUD resources — ResourceRegistrar"
section with the full route table + handler shape + idType.
- README.md: novelty bullet for CRUD scaffolding.
Suite 642 → 657 / 1391 → 1445.
![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: 8f2f225938
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
Adds a core HTTP primitive for CRUD resource routing by introducing a CrudHandler interface plus a ResourceRegistrar::register() helper that registers a standard five-route CRUD URL family and wires DTO binding/validation + common status mapping. This fits into the framework’s routing + Binder stack, aiming to recover the “one registration yields 5 endpoints” ergonomics without coupling to a specific storage layer.
Changes:
- Introduce
Rxn\Framework\Http\Resource\CrudHandlerandResourceRegistrarto register POST/GET/GET{id}/PATCH{id}/DELETE{id} routes with Binder-driven DTO binding and status handling. - Add integration tests + DTO fixtures and an in-memory CRUD handler to verify route registration, id constraints, validation → 422, missing rows → 404, and delete → 204.
- Update docs/README/CHANGELOG/horizons to document the new CRUD resource primitive.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Rxn/Framework/Http/Resource/CrudHandler.php | Defines the 5-method storage-agnostic CRUD handler contract. |
| src/Rxn/Framework/Http/Resource/ResourceRegistrar.php | Registers the five CRUD routes and performs DTO binding + response/status mapping. |
| src/Rxn/Framework/Tests/Http/Resource/ResourceRegistrarTest.php | Integration coverage for routing shape, binding/validation, and status behaviors. |
| src/Rxn/Framework/Tests/Http/Resource/Fixture/CreateWidget.php | Create DTO fixture with validation attributes. |
| src/Rxn/Framework/Tests/Http/Resource/Fixture/UpdateWidget.php | Patch/update DTO fixture (all-optional fields). |
| src/Rxn/Framework/Tests/Http/Resource/Fixture/SearchWidgets.php | Search/filter DTO fixture for query binding. |
| src/Rxn/Framework/Tests/Http/Resource/Fixture/InMemoryWidgetCrud.php | In-memory CrudHandler implementation for tests. |
| docs/routing.md | Documents ResourceRegistrar usage and resulting routes. |
| docs/horizons.md | Marks theme 1.6 CRUD scaffolding as realized via this primitive. |
| README.md | Adds a novelty bullet and updates test counts. |
| CHANGELOG.md | Adds an Unreleased entry describing the CRUD resource feature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- P1: middleware composition was impossible. register() took only
Router and returned void, so callers couldn't:
1. Register through a RouteGroup (where prefix + group
middleware get applied to every route the group adds)
2. Get Route handles back to attach middleware afterward
Both are needed for protected CRUD APIs (auth on all five,
stricter check on destructive ops only). API change:
- First arg is now Router|RouteGroup (RouteGroup support
gives group-prefix + group-middleware inheritance for
free, since RouteGroup::add already applies them).
- Return type is the new ResourceRoutes value object —
five public readonly Route fields plus a chainable
middleware() that fans the same stack across all five.
- Closures `(array $params)` (read, delete) → `(array $params,
ServerRequestInterface $request)` for accurate signatures.
Same false-alarm-but-clean-it-up pattern as the Version /
HealthCheck fixes.
- Docstring on validation-failure shape said "returns array
envelope (meta.status=422)"; implementation actually returns
a PSR-7 Problem Details response directly. Updated docstring
to match the implementation across create / update / search.
- docs/routing.md: bin/rxn scaffold:from-table is documented as
a future codegen step, not as if it exists today. Marked
explicitly under "horizons theme 1.6 follow-ups".
- docs/routing.md: new "Composing middleware" section covering
the three composition shapes (group-based, resource-wide,
per-op) so the API change is discoverable.
New ResourceRoutes value object lives at
Rxn\Framework\Http\Resource\ResourceRoutes — public readonly
fields (create / search / read / update / delete) plus
middleware(...) chainer + all() iterator helper.
4 new tests:
- testRegisterReturnsResourceRoutesValueObject — return type
- testMiddlewareAppliesToAllFiveRoutes — bag-level chainer
- testIndividualRouteCanCarryAdditionalMiddleware — public-field access
- testRegisterAcceptsRouteGroupAndInheritsPrefixPlusMiddleware
— RouteGroup integration end-to-end
Plus one fixture (TagMiddleware) standing in for an identity-
checkable middleware so tests can assertContains() the exact
instance back from Router::match() output.
Suite 657 → 661 / 1445 → 1474.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- ResourceRegistrar: remove unused `use Rxn\Framework\Http\Route`
import. The registrar collects Route handles into ResourceRoutes
but never references the class by name (the ResourceRoutes
constructor takes them by position).
- CrudHandler docblock: drop the "implementation may surface
pagination metadata in top-level meta" claim. The registrar
wraps search results as `{data: [...]}` only; there's no meta
slot at the response level. Pagination is the Pagination
middleware's concern (X-Total-Count + RFC 8288 Link headers
via `$routes->search->middleware(new Pagination(...))`),
separate from the handler return shape.
- No CHANGELOG / count change in this commit; PR description
updated separately to reflect the current 19-test / 661-1474
totals.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 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.
|
@copilot apply changes based on the comments in this thread |
…istration time - Normalize $path at top of register(): single leading slash, no trailing slash, so Router and RouteGroup behave identically regardless of caller conventions. - Validate $create / $update / $search class-strings implement RequestDto via is_subclass_of() at registration time; throw InvalidArgumentException rather than deferring to a runtime 500 on first request. - Add 4 new tests: path normalization (missing leading slash) + 3 invalid DTO class cases (create / update / search). Agent-Logs-Url: https://github.com/davidwyly/rxn/sessions/7508d7fb-5f5e-4347-a0a3-7283f8670e3e Co-authored-by: davidwyly <13552783+davidwyly@users.noreply.github.com>
Applied both suggestions from that review thread in commit 34baeee:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot apply changes based on the comments in this thread |
… via array_filter array_filter without a callback drops falsy values including empty strings. Validate $create and $update unconditionally; include $search only when non-null. Add two tests for empty-string $create/$update to cover the gap. Agent-Logs-Url: https://github.com/davidwyly/rxn/sessions/7e25d0c7-83ff-4554-b52e-d55d45c32056 Co-authored-by: davidwyly <13552783+davidwyly@users.noreply.github.com>
Fixed in commit 8794e83. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot apply changes based on the comments in this thread |
- routing.md: reword POST/PATCH rows from "body bound from" to "DTO bound via Binder (query + body, body wins)" to match Binder::bindRequest() semantics - ResourceRegistrar.php: same wording fix in create/update docblock entries - horizons.md: fix integration test count 15 → 19 Agent-Logs-Url: https://github.com/davidwyly/rxn/sessions/662b9e64-5636-40f0-b46a-e4dfa5e9a0a0 Co-authored-by: davidwyly <13552783+davidwyly@users.noreply.github.com>
Applied all three in commit e4938a7:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot apply changes based on the comments in this thread |
Replace Binder::bindRequest() with Binder::bind($search, $request->getQueryParams())
in the search handler so a GET request body cannot influence filter binding.
Aligns implementation with the $search docblock contract ("query shape for GET,
no body") and the routing docs.
All 25 ResourceRegistrar tests pass.
Agent-Logs-Url: https://github.com/davidwyly/rxn/sessions/014b0d35-e9c5-4758-8133-a6d689d9b18f
Co-authored-by: davidwyly <13552783+davidwyly@users.noreply.github.com>
Fixed in commit f1e8a45. The search handler now calls |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 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.
… tests
The Copilot SWE agent's pulled commits already addressed:
- path normalization for RouteGroup interop (PRRT_QQhf)
- DTO class-string validation at registration (PRRT_QQhn, PRRT_QWPm)
- doc wording: "query + body, body wins" not "request body" (PRRT_RNYs, PRRT_RNZB)
- horizons.md test count (PRRT_RNZN)
- search binds from query params only, not body (PRRT_YB-K)
Two threads still needed code work:
- PRRT_e-xI: existing DTO-class validation fell back to a single
`is_subclass_of` check + a misleading "must implement
RequestDto" message for typos / nonexistent class names.
Three distinct failure modes now produce three distinct
messages: empty string ("DTO class name cannot be empty"),
nonexistent ("class '<name>' does not exist"), exists-but-
doesn't-implement-RequestDto (existing message). Each says
which arg ($create / $update / $search) for fast triage.
- PRRT_e-xo: $idType was interpolated directly into the route
placeholder grammar. A label containing '-' or empty would
surface as a generic "Malformed route placeholder" pointing
at Router::compile, not the registrar. Now validated up
front against [a-zA-Z_][a-zA-Z0-9_]* with a targeted message
before interpolation.
Three new tests:
- testRegisterThrowsOnNonexistentDtoClass — exercises the new
class_exists check; asserts the message says "does not exist"
so operators know to fix the import vs the class hierarchy
- testRegisterThrowsOnIdTypeContainingInvalidCharacters — '-' rejected
- testRegisterThrowsOnEmptyIdType — empty rejected
CHANGELOG test count bumped 19 → 28 to reflect everything in
this PR (Copilot's added 6 path/DTO/idType tests + my 3 new
ones).
Suite 661 → 670 / 1474 → 1490.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Recovers the convention router's "extend a class, get five endpoints" ergonomic — without legacy AR baggage. One call wires a
CrudHandlerto the full five-route URL family; framework handles DTO binding, validation → 422, missing-row → 404, deleted → 204.```php
$routes = ResourceRegistrar::register(
$router,
'/products',
new ProductsCrud($repo),
create: CreateProduct::class,
update: UpdateProduct::class,
search: SearchProducts::class,
);
```
After registration the router carries:
/productscreate(\$dto)/productssearch(\$filter)/products/{id:int}read(\$id)/products/{id:int}update(\$id, \$dto)/products/{id:int}delete(\$id)Middleware composition
The returned
ResourceRoutesexposes the fiveRoutehandles so callers can attach middleware in three shapes:```php
// Group-based — RouteGroup inherits the group's prefix + middleware automatically.
$router->group('/v1', function (RouteGroup $g) use ($auth) {
$g->middleware($auth);
ResourceRegistrar::register($g, '/products', $crud, /* … */);
});
// Resource-wide — chainable.
ResourceRegistrar::register($router, '/products', $crud, /* … */)
->middleware($bearerAuth);
// Per-op — target the specific Route handle on the bag.
$routes = ResourceRegistrar::register($router, '/products', $crud, /* … */);
$routes->update->middleware($adminOnly);
$routes->delete->middleware($adminOnly);
```
register()accepts either aRouteror aRouteGroupas the first argument.Design
bin/rxn scaffold:from-table) writes them frominformation_schema— schema-as-truth without runtime DB-at-boot couplingRxnOrmCrudHandlerbase for relational, ~50 LOC for any other backendResourceRegistrar::register()— no magic URL convention. Routes are visible in the Router after registrationidType'int'by default (URL placeholder + cast).'uuid'/'slug'/'any'/ custom keeps the captured stringdeletereturns a true PSR-7 204 with empty body (per HTTP spec) — registrar bypasses the array envelope mapper for that one caseHttp\Middleware\Pagination's concern, not the handler's. Stack it on the search route via\$routes->search->middleware(...)for X-Total-Count + RFC 8288 Link headersWhat's NOT in this PR (deliberate scope cut, see horizons 1.6 follow-ups)
RxnOrmCrudHandlerabstract base — lives indavidwyly/rxn-orm. Reduces a relational handler to ~10 LOC. Follow-up PR on the rxn-orm repo.bin/rxn scaffold:from-table— codegen step that connects to the DB, readsinformation_schema, writes DTO files + handler stub. One-shot at scaffold time. Follow-up.#[Resource]attribute — alternative to explicit registration; would letScannerdiscover resources at scan time. Useful but out-of-scope for the primitive.Tests
19 integration tests against an in-memory fixture (
Tests/Http/Resource/):idType=intrejects non-integer ids at the route levelnull→ 404 Problem Details)nullfilteridType='uuid'produces string IDs at the handlerregister()returns aResourceRoutesvalue object (4 new tests)ResourceRoutes::middleware()applies to all five routesRouteGroupregistration inherits prefix + group middlewareFiles
```
src/Rxn/Framework/Http/Resource/
├── CrudHandler.php 5-method interface
├── ResourceRegistrar.php register() static + DTO binding + status-code mapping
└── ResourceRoutes.php value object holding 5 Route handles + middleware() chainer
src/Rxn/Framework/Tests/Http/Resource/
├── ResourceRegistrarTest.php
└── Fixture/
├── CreateWidget.php DTO with #[Required], #[Length], #[Min], #[InSet]
├── UpdateWidget.php all-nullable DTO for partial updates
├── SearchWidgets.php filter DTO with optional fields
├── InMemoryWidgetCrud.php test-only handler implementation
└── TagMiddleware.php identity-checkable middleware for assertions
docs/horizons.md theme 1.6 marked REALIZED (core)
docs/routing.md new "CRUD resources — ResourceRegistrar" section + "Composing middleware" section; scaffold:from-table noted as future
README.md novelty bullet
CHANGELOG.md full Unreleased entry
```
Test plan