tests #7
tests #7
Conversation
📝 WalkthroughWalkthroughAdds RFC 9457 Problem Details API helpers and response class, introduces Http401/Http403 event types, extends Application error handling to emit events and route to new 401/403 pages, adds controller/view templates, example/demo, PHPUnit config, and multiple tests for these behaviors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant App as Application
participant Controller as Controller
participant EventBus as Event Registry
participant HttpCodes as HttpCodes Controller
participant View as View Renderer
rect rgba(200,230,255,0.5)
Note right of App: Normal request flow
end
Client->>App: HTTP request
App->>Controller: executeController()
alt Controller executes successfully
Controller-->>App: Response (200/2xx)
App-->>Client: HTTP response
else Controller throws Unauthorized/Forbidden/NotFound
Controller-->>App: throw Unauthorized/Forbidden/NotFound
App->>App: check recursion guard
App->>EventBus: emit Http401/Http403/Http404(route, realm/resource/permission)
EventBus->>EventBus: notify registered listeners
App->>App: log warning
App->>HttpCodes: forward to code401/code403/code404 with params
HttpCodes->>View: render corresponding template
View-->>HttpCodes: HTML body
HttpCodes-->>App: error response (401/403/404)
App-->>Client: HTTP error response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 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.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/Mvc/Api/ApiResponseTrait.php:
- Around line 186-202: rateLimitProblem currently builds an $extensions array
(with limit and remaining) but never uses it; update the method to merge those
extensions into the ProblemDetailsResponse before sending: create the response
via ProblemDetailsResponse::rateLimitExceeded($retryAfter, $detail), then attach
$extensions to that response's ProblemDetails (either by passing $extensions
into rateLimitExceeded if it supports an extensions arg, or by calling the
response's method to set/merge extensions on the ProblemDetails) and finally
call ->send(); keep the $extensions building logic and ensure limit/remaining
values are included in the sent problem details.
In @tests/Mvc/ApplicationTest.php:
- Around line 31-32: The captured initial output buffer level
($this->initialBufferLevel) in ApplicationTest::setUp() is unused; either remove
the property declaration and the assignment (the ob_get_level() call) to avoid
dead code, or implement buffer restoration in ApplicationTest::tearDown() by
comparing ob_get_level() to $this->initialBufferLevel and calling
ob_end_clean()/ob_end_flush() in a loop until levels match; update or remove the
$initialBufferLevel property and ensure setUp()/tearDown() are consistent.
- Line 22: The property initialBufferLevel is declared but never used; either
remove the unused property declaration or use it by storing the initial buffer
value in setUp() (as currently done) and asserting/restoring it in tearDown() —
e.g., reference initialBufferLevel and the buffer inspection/reset logic in
tearDown() to validate that buffer level returned to initialBufferLevel after
each test, or simply delete the private int $initialBufferLevel field if no
validation is required.
🧹 Nitpick comments (8)
tests/debug_output_buffer.php (1)
1-56: Consider converting this debug script to a proper PHPUnit test.This standalone debug script duplicates some functionality that could be covered by
SimpleOutputBufferTest. If it's meant for one-time debugging, consider removing it before merging. If it serves an ongoing purpose, document its usage in the file header or convert it to a proper test case.Also, the variable
$Inion line 19 uses PascalCase which is unconventional for PHP variables; consider renaming to$inior$configfor consistency.tests/Mvc/Events/EventsTest.php (1)
17-100: LGTM!Comprehensive test coverage for
Http401andHttp403events, including edge cases with null values and various property combinations. The tests properly verify both constructor behavior and property accessibility.One optional improvement: consider using PHPUnit's
@dataProviderannotation for the loop-based tests (e.g.,testHttp401EventWithDifferentRealms) to get better test isolation and more descriptive failure messages.src/Mvc/Application.php (1)
307-311: Simplify the error handler check - double-backslash variant is unnecessary.The check for both
'Neuron\Mvc\Controllers\HttpCodes'and'Neuron\\Mvc\\Controllers\\HttpCodes'is redundant. PHP class name comparisons use the normalized form (single backslash). The double-backslash is only needed in string literals for escaping, but the actual value stored is still single-backslash.♻️ Suggested simplification
// Check if we're already handling an error to prevent infinite recursion - $isErrorHandler = in_array( $controller, [ - 'Neuron\Mvc\Controllers\HttpCodes', - 'Neuron\\Mvc\\Controllers\\HttpCodes' - ] ); + $isErrorHandler = $controller === 'Neuron\Mvc\Controllers\HttpCodes';tests/Mvc/SimpleOutputBufferTest.php (2)
12-19: Removeerror_logcall from test.The
error_logon line 18 will write to stderr during test runs, adding noise to the test output. If debugging info is needed, use PHPUnit's--debugflag or$this->addToAssertionCount()pattern instead.♻️ Suggested fix
public function testInitialBufferLevel(): void { $level = ob_get_level(); $this->assertGreaterThanOrEqual( 0, $level ); - - // PHPUnit typically starts with level 1 - error_log( "Initial buffer level in test: $level" ); }
21-25: Remove the no-op test.
testNoOutputBufferassertsassertTrue(true)which provides no test value. If it's a placeholder, remove it; if it's meant to verify something specific about running without buffers, add meaningful assertions.♻️ Suggested fix
- public function testNoOutputBuffer(): void - { - // This test does nothing with output buffers - $this->assertTrue( true ); - } -tests/Api/ApiResponseTraitTest.php (1)
30-60: Unused$headersparameter in test helper method.The
$headersparameter on line 37 is declared but never used in the method body. This was flagged by PHPMD.Additionally, the test helper methods duplicate the logic from
ApiResponseTraitrather than delegating to the trait's protected methods. This means the tests verify the expected JSON structure but don't actually exercise the real trait implementation. Consider whether this is intentional (testing the contract) or if the helpers should call the trait methods directly.🔧 Suggested fix for unused parameter
public function testProblemResponse( ProblemType|string $type, ?string $title = null, ?int $status = null, ?string $detail = null, ?string $instance = null, - array $extensions = [], - array $headers = [] + array $extensions = [] ): string {src/Mvc/Api/ProblemDetailsResponse.php (1)
122-125: Handle potentialjson_encodefailure.
json_encode()can returnfalseon failure (e.g., for non-UTF-8 strings or resources). The current implementation doesn't handle this case, which could propagatefalseto callers expecting a string.🔧 Suggested fix
public function toJson(int $options = JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE): string { - return json_encode($this->problemDetails, $options); + $json = json_encode($this->problemDetails, $options); + if ($json === false) { + // Fallback to a minimal valid response + return json_encode([ + 'type' => '/errors/internal', + 'title' => 'Internal Server Error', + 'status' => 500, + 'detail' => 'Failed to encode error response' + ]); + } + return $json; }src/Mvc/Api/ApiResponseTrait.php (1)
7-7: Unused import.
HttpResponseStatusis imported but never used in this trait.🔧 Suggested fix
use Neuron\Core\ProblemDetails\ProblemDetails; use Neuron\Core\ProblemDetails\ProblemType; -use Neuron\Mvc\Responses\HttpResponseStatus; use Exception;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
examples/http-status-demo.phpphpunit-no-risky.xmlresources/views/http_codes/401.phpresources/views/http_codes/403.phpsrc/Mvc/Api/ApiResponseTrait.phpsrc/Mvc/Api/ProblemDetailsResponse.phpsrc/Mvc/Application.phpsrc/Mvc/Controllers/HttpCodes.phpsrc/Mvc/Events/Http401.phpsrc/Mvc/Events/Http403.phptests/Api/ApiResponseTraitTest.phptests/Mvc/ApplicationTest.phptests/Mvc/Events/EventsTest.phptests/Mvc/SimpleHttpStatusTest.phptests/Mvc/SimpleOutputBufferTest.phptests/debug_output_buffer.php
🧰 Additional context used
🧬 Code graph analysis (9)
tests/Mvc/SimpleHttpStatusTest.php (2)
src/Mvc/Events/Http401.php (1)
Http401(16-30)src/Mvc/Events/Http403.php (1)
Http403(16-33)
src/Mvc/Events/Http403.php (1)
src/Mvc/Events/Http401.php (1)
__construct(25-29)
tests/Mvc/Events/EventsTest.php (3)
src/Mvc/Events/Http401.php (1)
Http401(16-30)src/Mvc/Events/Http403.php (1)
Http403(16-33)examples/http-status-demo.php (1)
event(27-49)
src/Mvc/Events/Http401.php (1)
src/Mvc/Events/Http403.php (1)
__construct(27-32)
src/Mvc/Controllers/HttpCodes.php (3)
src/Mvc/Requests/Request.php (3)
Request(21-385)getRouteParameters(55-58)getRouteParameter(66-69)src/Mvc/Controllers/IController.php (1)
renderHtml(31-31)src/Mvc/Controllers/Base.php (1)
renderHtml(154-167)
src/Mvc/Application.php (3)
src/Mvc/Controllers/Factory.php (1)
Factory(15-56)src/Mvc/Events/Http401.php (1)
Http401(16-30)src/Mvc/Events/Http403.php (1)
Http403(16-33)
src/Mvc/Api/ApiResponseTrait.php (1)
src/Mvc/Api/ProblemDetailsResponse.php (6)
ProblemDetailsResponse(37-231)create(66-69)send(136-149)validationError(196-207)notFound(216-230)rateLimitExceeded(175-187)
tests/debug_output_buffer.php (1)
src/Mvc/Application.php (3)
Application(37-837)setCaptureOutput(93-97)getOutput(110-113)
examples/http-status-demo.php (4)
src/Mvc/Events/Http401.php (1)
Http401(16-30)src/Mvc/Events/Http403.php (1)
Http403(16-33)src/Mvc/Events/Http404.php (1)
Http404(11-19)src/Mvc/Application.php (1)
Application(37-837)
🪛 PHPMD (2.15.0)
tests/Api/ApiResponseTraitTest.php
37-37: Avoid unused parameters such as '$headers'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (23)
phpunit-no-risky.xml (1)
1-19: LGTM! Configuration is well-structured.The PHPUnit configuration appropriately relaxes strictness checks as indicated by the filename, includes a properly configured test suite, and sets up code coverage for the
srcdirectory. The bootstrap path and directory structure align with standard PHP project conventions.src/Mvc/Events/Http401.php (1)
1-30: LGTM! Event class is well-structured and documented.The
Http401event class follows a clean design pattern consistent with the relatedHttp403event. The documentation clearly explains when the event is emitted, the properties are appropriately typed, and the optional$realmparameter provides flexibility for WWW-Authenticate header scenarios.resources/views/http_codes/403.php (1)
1-14: LGTM! View template implements proper security measures.The template correctly uses
htmlspecialchars()withENT_QUOTESandUTF-8encoding on all dynamic values, providing solid XSS protection. The conditional rendering for optional$resourceand$permissionprevents displaying empty fields, and the null coalescing operator provides a sensible fallback for the route.resources/views/http_codes/401.php (1)
1-9: LGTM! View template follows secure coding practices.The template properly escapes all dynamic content using
htmlspecialchars()withENT_QUOTESandUTF-8encoding, preventing XSS vulnerabilities. The structure is consistent with the 403 view template, and the conditional rendering of the realm provides a clean user experience.tests/Mvc/ApplicationTest.php (1)
72-72: The/testroute is properly configured to accept POST requests inexamples/config/routes.yaml. The test change to use POST is correct and no issues exist.Likely an incorrect or invalid review comment.
src/Mvc/Controllers/HttpCodes.php (2)
24-37: LGTM!The
code401method follows the established pattern from existing error handlers. The implementation correctly renders the 401 view with the authentication realm context.
39-53: LGTM!The
code403method is consistent with the existing error handler pattern. It properly passes the resource and permission context to the view for meaningful error messages.tests/Mvc/SimpleHttpStatusTest.php (4)
84-92: Verify constructor argument order for exception chaining.The
Forbiddenconstructor call uses positional arguments('Access denied', null, null, 403, $previous). Ensure this matches the constructor signature:(message, resource, permission, code, previous). TheUnauthorizedcall('Auth failed', null, 401, $previous)should match(message, realm, code, previous).
17-64: LGTM!Good test coverage for exception and event property accessors, including edge cases with null values and inheritance verification.
75-82: Verify thatForbiddendefaults to code 403.The test calls
new Forbidden('Cannot delete')with only the message parameter but expects exception code 403. Since theForbiddenexception class is defined in the externalneuron-php/corepackage and is not accessible in this repository, the default exception code cannot be verified. Confirm that theForbiddenconstructor indeed defaults to code 403 when the code parameter is omitted.
66-73: The test correctly assumesUnauthorizeddefaults to code 401.Line 72 calls
new Unauthorized('Invalid token')with only the message parameter, and line 69 expects exception code 401. This test will only pass if the constructor defaults the code parameter to 401. The broader test patterns confirm this is the intended behavior: the constructors support no-argument calls (line 55-56), single message calls (lines 72, 81), and explicit parameter calls with all values specified (lines 17, 28, 87-88).tests/Mvc/Events/EventsTest.php (1)
5-6: LGTM!Appropriate imports added for the new event classes being tested.
src/Mvc/Application.php (3)
352-376: LGTM - Unauthorized exception handling.The implementation correctly:
- Checks the recursion guard to prevent infinite loops
- Logs the authentication failure
- Emits the
Http401event for observability- Redirects to the
code401controller method with realm contextThis follows the established pattern from the
NotFoundhandler.
377-403: LGTM - Forbidden exception handling.Consistent implementation with the
Unauthorizedhandler, properly passing resource and permission context to the error page.
404-424: LGTM - Recursion guard added to NotFound handler.Good enhancement to add the same recursion protection to the existing
NotFoundhandler for consistency.examples/http-status-demo.php (1)
1-143: LGTM!Well-structured demonstration script that clearly illustrates the HTTP 401/403/404 event system. The inline
EventLoggerclass and step-by-step demos effectively document the expected behavior for developers learning the framework.tests/Mvc/SimpleOutputBufferTest.php (1)
27-38: LGTM!Good test that properly validates the output buffering workflow - starts a buffer, captures content, cleans up, and verifies the buffer level is restored.
tests/Api/ApiResponseTraitTest.php (1)
176-425: Good test coverage for RFC 9457 Problem Details scenarios.The test suite provides comprehensive coverage for:
- Validation problems with/without custom detail
- Not-found problems with/without identifier
- Authentication/permission problems with optional extensions
- Bad request, conflict, and internal error scenarios
- Both
ProblemTypeenum and custom string types- Full parameter combinations including instance and extensions
src/Mvc/Events/Http403.php (1)
16-33: Clean event class implementation.The
Http403event class follows the same pattern asHttp401(as seen in the relevant code snippets), implementsIEvent, and provides appropriate context properties (route,resource,permission) for 403 error handling. The docblock clearly explains the event's purpose and timing.src/Mvc/Api/ProblemDetailsResponse.php (2)
136-149: Good defensive check forheaders_sent().The
send()method correctly guards against setting headers when they've already been sent, preventing PHP warnings. This makes the class suitable for both normal HTTP contexts and testing scenarios.
175-230: Well-designed static factory methods.The
rateLimitExceeded(),validationError(), andnotFound()helper methods provide convenient shortcuts for common error scenarios while maintaining RFC 9457 compliance. TheRetry-Afterheader handling inrateLimitExceeded()is particularly well done.src/Mvc/Api/ApiResponseTrait.php (2)
279-301: Good security consideration for debug info.The
internalErrorProblemmethod properly gates debug information behind the$includeDebugflag, with clear documentation warning about production usage. This prevents accidental exposure of sensitive exception details.
51-109: Well-structured trait with flexible problem response construction.The trait provides a clean API with
problemDetailsResponsefor pre-builtProblemDetailsobjects andproblemResponsefor inline construction. The handling of bothProblemTypeenum and string URIs offers good flexibility for both standard and custom error types.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @tests/Api/ApiResponseTraitTest.php:
- Around line 17-171: The anonymous test controller reimplements ProblemDetails
creation instead of invoking the trait, so replace each testX method
(testProblemDetailsResponse, testProblemResponse, testValidationProblem,
testNotFoundProblem, testAuthenticationProblem, testPermissionProblem,
testBadRequestProblem, testConflictProblem, testInternalErrorProblem) to simply
call and return the corresponding trait methods (e.g., return
$this->validationProblem(...), $this->notFoundProblem(...),
$this->authenticationProblem(...), $this->permissionProblem(...),
$this->badRequestProblem(...), $this->conflictProblem(...),
$this->internalErrorProblem(...), and $this->problemResponse() /
$this->problemDetailsResponse()) so tests exercise the actual ApiResponseTrait
behavior; expose any protected trait methods by adding public wrapper methods in
the anonymous class that delegate directly to the trait methods with the same
arguments and return values.
🧹 Nitpick comments (2)
tests/Api/ApiResponseTraitTest.php (1)
432-447: Inconsistent test pattern: closures defined in test methodsTest methods
testServiceUnavailableProblem,testServiceUnavailableWithRetryAfter,testMethodNotAllowedProblem, andtestUnsupportedMediaTypeProblemdynamically add closure properties to the controller object at runtime. This pattern is inconsistent with the methods defined insetUp()(lines 24-169) and makes the test structure harder to follow.♻️ Refactor to define methods in setUp
Move these closures into the anonymous class definition in
setUp():$this->controller = new class { use ApiResponseTrait; + public function testServiceUnavailableProblem(?string $detail = null, ?int $retryAfter = null): string + { + $extensions = []; + if ($retryAfter !== null) { + $extensions['retry_after'] = $retryAfter; + } + $problem = new ProblemDetails(/* ... */); + return json_encode($problem); + } // ... other methods };Then in test methods, call directly:
$json = $this->controller->testServiceUnavailableProblem();tests/Api/ProblemDetailsIntegrationTest.php (1)
22-228: LGTM: Proper integration testing approachThe anonymous controller correctly extends
Base, usesApiResponseTrait, and its action methods (show,create,update, etc.) properly delegate to the actual trait methods like$this->notFoundProblem(),$this->validationProblem(),$this->conflictProblem(), etc. TherenderJsonoverride at lines 223-226 appropriately returns JSON for test assertions rather than sending headers.This integration test validates real-world usage patterns and exercises the trait's actual implementation.
Note: Static analysis flags the unused
$responseCodeparameter in therenderJsonoverride (line 223). This is acceptable for a test override that simplifies the response, but you could optionally include it in the test output if HTTP status codes need validation.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/Api/ApiResponseTraitTest.phptests/Api/ProblemDetailsIntegrationTest.phptests/Api/ProblemDetailsResponseTest.php
✅ Files skipped from review due to trivial changes (1)
- tests/Api/ProblemDetailsResponseTest.php
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Api/ApiResponseTraitTest.php (1)
src/Mvc/Api/ProblemDetailsResponse.php (1)
toArray(159-166)
🪛 PHPMD (2.15.0)
tests/Api/ProblemDetailsIntegrationTest.php
223-223: Avoid unused parameters such as '$responseCode'. (undefined)
(UnusedFormalParameter)
tests/Api/ApiResponseTraitTest.php
37-37: Avoid unused parameters such as '$headers'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (1)
tests/Api/ProblemDetailsIntegrationTest.php (1)
230-496: Comprehensive integration test coverageThe test suite provides excellent coverage of Problem Details scenarios:
- CRUD operations with success and error paths
- Validation errors (simple and nested)
- Resource not found (404)
- Conflicts (409)
- Permission denied (403)
- Rate limiting (429) with proper sequential request testing
- Internal errors (500)
- Custom business errors with extensions
- Response structure validation ensuring RFC 9457 compliance
The
testResponseDistinctiontest (lines 425-441) is particularly valuable for ensuring clients can reliably differentiate between success responses and error responses.
Note
Adds standardized API error handling and 401/403 HTTP flow with events and views.
ApiResponseTraitandProblemDetailsResponsefor consistent JSON errors (validation, auth, permissions, rate limit, etc.)Applicationto catchUnauthorized/Forbidden, emitHttp401/Http403events, and render viaHttpCodes@code401/403(prevents recursion); retains existing 404/500 handlingHttp401/Http403events and viewsresources/views/http_codes/401.phpand403.phpexamples/http-status-demo.phpshowcasing events and handlingphpunit-no-risky.xmlconfigWritten by Cursor Bugbot for commit 83cd608. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.