Cache RequestValidator + make ApiExceptionSubscriber exception-safe#22
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR refactors RequestValidator to inject and reuse PSR HTTP message factories instead of inline instantiation, adds OpenAPI validator caching to avoid redundant generation, and makes ApiExceptionSubscriber resilient to normalizer failures via try/catch fallback handling that returns a generic RFC7807 error response. ChangesPSR Factory Injection and Request Validation with Caching
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/EventSubscriber/ApiExceptionSubscriber.php`:
- Around line 80-82: In ApiExceptionSubscriber where the JsonResponse is
constructed (the return new JsonResponse(...) call), the headers merge currently
lets $throwable->getHeaders() override 'Content-Type'; change the merge order so
the default 'Content-Type' => 'application/problem+json' takes precedence (e.g.,
merge throwable headers into the defaults rather than the other way around) to
enforce RFC7807 compliance and ensure ApiProblemException headers cannot
override Content-Type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 48280454-6ea8-4b78-889b-a7a0b7a9119e
📒 Files selected for processing (5)
config/validators.phpsrc/EventSubscriber/ApiExceptionSubscriber.phpsrc/Validator/RequestValidator.phptests/Unit/EventSubscriber/ApiExceptionSubscriberTest.phptests/Unit/Validator/RequestValidatorTest.php
Reverse the array_merge order in ApiExceptionSubscriber so the bundle's application/problem+json default takes precedence over headers carried by the throwable. Throwable headers like Allow (405) and Retry-After (503) are still preserved; only Content-Type is locked to the RFC 7807 contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary by CodeRabbit
Bug Fixes
Performance
Tests