[#470] Enforce response-returning route dispatch#475
[#470] Enforce response-returning route dispatch#475armanist merged 1 commit intosoftberg:masterfrom
Conversation
📝 WalkthroughWalkthroughThis PR introduces a fluent Response API by refactoring response trait methods to return Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Dispatcher as RouteDispatcher
participant Handler as Handler (Closure/<br/>Controller)
participant Response
Client->>Dispatcher: dispatch(matched, request)
Dispatcher->>Handler: invoke handler
Handler->>Response: json(...) / html(...)
Response-->>Handler: returns self (fluent)
Handler->>Handler: return Response
Handler-->>Dispatcher: returns Response
alt Response valid
Dispatcher-->>Client: return Response
else Response invalid
Dispatcher->>Dispatcher: throw<br/>RouteException
Dispatcher-->>Client: exception
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #475 +/- ##
============================================
+ Coverage 82.48% 82.97% +0.48%
- Complexity 2902 2905 +3
============================================
Files 251 251
Lines 7814 7663 -151
============================================
- Hits 6445 6358 -87
+ Misses 1369 1305 -64 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
tests/Unit/Http/Traits/Response/HttpResponseHeaderTest.php (1)
23-25: Consider also assertingsetContentTypereturnsself.
src/Http/Traits/Response/Header.phpnow also makessetContentType()returnself, but there is no test asserting the fluent contract for that method. A one-lineassertSame($response, $response->setContentType(ContentType::JSON))would close the gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Http/Traits/Response/HttpResponseHeaderTest.php` around lines 23 - 25, Add a one-line fluent-interface assertion in HttpResponseHeaderTest to ensure setContentType returns self: in the same test that asserts setHeader's fluency, call setContentType with ContentType::JSON (or another ContentType) and assertSame($response, $response->setContentType(...)); this verifies the Header::setContentType() change in src/Http/Traits/Response/Header.php and ensures the method returns the response instance.tests/Unit/Router/RouteDispatcherTest.php (2)
65-65: DeadMockery::mock(Response::class)call.Since the CSRF test was updated to use
Di::setfor proper dependency injection, leftoverMockery::mock(Response::class);lines in this test (and Line 201) no longer serve a purpose — they create an unused mock that isn't wired into anything. Consider removing them for consistency with the new injection approach.🧹 Proposed cleanup
$request = Mockery::mock(Request::class); - Mockery::mock(Response::class); $dispatcher->dispatch($matched, $request);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Router/RouteDispatcherTest.php` at line 65, Remove the unused Mockery::mock(Response::class) calls left in the CSRF-related test in RouteDispatcherTest: they no longer serve any purpose since the test now injects the Response via Di::set; delete the standalone Mockery::mock(Response::class) invocations (including the one around line 201) so the test uses only the DI-provided Response and doesn't create dead mocks.
151-184: Solid negative-path coverage.Both new tests (
testDispatchThrowsWhenClosureRouteDoesNotReturnResponseandtestDispatchThrowsWhenControllerActionDoesNotReturnResponse) correctly exercise the newrequireResponseguard for closure and controller paths respectively, satisfying the "fail fast" acceptance criterion.One optional improvement: consider asserting the exception message contains the handler identifier (
'closure route'orControllerClass::post) so regressions inRouteException::invalidHandlerResponse(...)formatting are caught here as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Router/RouteDispatcherTest.php` around lines 151 - 184, Update the two tests to also assert the RouteException message contains the handler identifier so formatting regressions are caught: in testDispatchThrowsWhenClosureRouteDoesNotReturnResponse assert the thrown exception message includes "closure route" (or the exact phrase produced by RouteException::invalidHandlerResponse for closures), and in testDispatchThrowsWhenControllerActionDoesNotReturnResponse assert the message contains the controller method identifier (e.g., "ClassName::post" or the exact string produced by RouteException::invalidHandlerResponse for controller actions); keep the existing expectException(RouteException::class) and add an assertion on the exception message after capturing the exception from RouteDispatcher::dispatch for MatchedRoute and Request mocks.src/Http/Traits/Response/Body.php (2)
87-92: Optional: makedelete()fluent for API consistency.For full fluent consistency with
set(),json(),xml(),jsonp(), andhtml(),delete()could also returnself. Not required by the PR objectives, but would round out the builder API.♻️ Proposed refinement
- public function delete(string $key): void + public function delete(string $key): self { if ($this->has($key)) { unset($this->__response[$key]); } + return $this; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Http/Traits/Response/Body.php` around lines 87 - 92, The delete method in the Body trait currently returns void; make it fluent by changing the signature of Body::delete(string $key) to return self and have it return $this at the end (while retaining the existing unset logic that uses $this->__response and the has($key) check) so it matches the fluent API used by set(), json(), xml(), jsonp(), and html().
69-73: Fluent API updates look good — considerstaticoverselffor extensibility.Converting
set,json,jsonp,xml, andhtmlto return$thiswith aselfreturn type satisfies the fluent API goal. Minor optional refinement: since this is a trait, usingstaticrather thanselfwould preserve late static binding if/when subclasses override the return contract. GivenResponseis the primary consumer and likelyfinal,selfis fine — flagging only for awareness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Http/Traits/Response/Body.php` around lines 69 - 73, The trait Body currently types its fluent methods to return self; change the return type declarations to static for extensibility so late static binding is preserved — update the methods set, json, jsonp, xml, and html in the Body trait to return static instead of self, ensuring each method signature and any PHPDoc/annotations referencing the return type are adjusted accordingly.src/Router/RouteDispatcher.php (1)
64-71: Consider validatingResponsebefore running__after.Currently
__afterruns betweenrequireResponse(...)and thereturn $response;. If__aftermutates response state (e.g., via$response->json(...)again) or throws, the validated response can be altered or lost. This matches pre-existing behavior, so flagging as optional — but if the fluent API encourages hook handlers to also manipulate$response, the ordering may warrant an explicit design choice (e.g., document that__afterruns with the validated response already locked in).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Router/RouteDispatcher.php` around lines 64 - 71, The code calls requireResponse($this->invoke(...)) to validate the controller result and then calls $this->callHook($controller, '__after', $params) which may mutate or replace the already-validated $response; to prevent losing validation, revise the sequence so requireResponse(...) assigns to $response and then callHook is invoked in a way that cannot alter the validated response (either call the hook with a clone/copy of $response or run callHook before returning but after locking $response), e.g., ensure requireResponse, assign to $response, then invoke callHook('__after') in a non-mutating way (or explicitly document and enforce that __after handlers must not modify $response) referencing requireResponse, invoke, callHook and the __after hook for where to change behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Http/Traits/Response/Body.php`:
- Around line 87-92: The delete method in the Body trait currently returns void;
make it fluent by changing the signature of Body::delete(string $key) to return
self and have it return $this at the end (while retaining the existing unset
logic that uses $this->__response and the has($key) check) so it matches the
fluent API used by set(), json(), xml(), jsonp(), and html().
- Around line 69-73: The trait Body currently types its fluent methods to return
self; change the return type declarations to static for extensibility so late
static binding is preserved — update the methods set, json, jsonp, xml, and html
in the Body trait to return static instead of self, ensuring each method
signature and any PHPDoc/annotations referencing the return type are adjusted
accordingly.
In `@src/Router/RouteDispatcher.php`:
- Around line 64-71: The code calls requireResponse($this->invoke(...)) to
validate the controller result and then calls $this->callHook($controller,
'__after', $params) which may mutate or replace the already-validated $response;
to prevent losing validation, revise the sequence so requireResponse(...)
assigns to $response and then callHook is invoked in a way that cannot alter the
validated response (either call the hook with a clone/copy of $response or run
callHook before returning but after locking $response), e.g., ensure
requireResponse, assign to $response, then invoke callHook('__after') in a
non-mutating way (or explicitly document and enforce that __after handlers must
not modify $response) referencing requireResponse, invoke, callHook and the
__after hook for where to change behavior.
In `@tests/Unit/Http/Traits/Response/HttpResponseHeaderTest.php`:
- Around line 23-25: Add a one-line fluent-interface assertion in
HttpResponseHeaderTest to ensure setContentType returns self: in the same test
that asserts setHeader's fluency, call setContentType with ContentType::JSON (or
another ContentType) and assertSame($response, $response->setContentType(...));
this verifies the Header::setContentType() change in
src/Http/Traits/Response/Header.php and ensures the method returns the response
instance.
In `@tests/Unit/Router/RouteDispatcherTest.php`:
- Line 65: Remove the unused Mockery::mock(Response::class) calls left in the
CSRF-related test in RouteDispatcherTest: they no longer serve any purpose since
the test now injects the Response via Di::set; delete the standalone
Mockery::mock(Response::class) invocations (including the one around line 201)
so the test uses only the DI-provided Response and doesn't create dead mocks.
- Around line 151-184: Update the two tests to also assert the RouteException
message contains the handler identifier so formatting regressions are caught: in
testDispatchThrowsWhenClosureRouteDoesNotReturnResponse assert the thrown
exception message includes "closure route" (or the exact phrase produced by
RouteException::invalidHandlerResponse for closures), and in
testDispatchThrowsWhenControllerActionDoesNotReturnResponse assert the message
contains the controller method identifier (e.g., "ClassName::post" or the exact
string produced by RouteException::invalidHandlerResponse for controller
actions); keep the existing expectException(RouteException::class) and add an
assertion on the exception message after capturing the exception from
RouteDispatcher::dispatch for MatchedRoute and Request mocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2abdbbf4-d64f-46f4-be29-b67cd63bbc71
📒 Files selected for processing (11)
src/Http/Traits/Response/Body.phpsrc/Http/Traits/Response/Header.phpsrc/Http/Traits/Response/Status.phpsrc/Router/Enums/ExceptionMessages.phpsrc/Router/Exceptions/RouteException.phpsrc/Router/RouteDispatcher.phptests/Unit/Http/Traits/Response/HttpResponseBodyTest.phptests/Unit/Http/Traits/Response/HttpResponseHeaderTest.phptests/Unit/Http/Traits/Response/HttpResponseStatusTest.phptests/Unit/Router/RouteDispatcherTest.phptests/_root/modules/Test/Controllers/TestController.php
![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: 7cd8ab1211
ℹ️ 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".
2 participants
Closes #470
Summary by CodeRabbit
New Features
Bug Fixes