Repository navigation
Conversation
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThe PR refactors exception handling from code-based registration to configuration-driven passthrough, and systematically converts nullable optional constructor dependencies to required non-nullable ones across Admin, Member, Auth, and public controllers, along with corresponding test updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/Unit/BootstrapTest.php (2)
179-202: Add error handling for shell_exec and consider PHPUnit's process isolation.The shell_exec approach for process isolation has several reliability concerns:
- No error handling if shell_exec returns null/empty (e.g., in environments without shell access)
- Depends on php binary being in PATH
- Could fail silently in restricted CI/CD environments
Consider using PHPUnit's
@runInSeparateProcessannotation or add error handling:🔎 Recommended error handling
$output = shell_exec("php -r " . escapeshellarg($code)); + $this->assertNotNull($output, 'Failed to execute PHP in separate process. Shell access may be restricted.'); + $this->assertNotEmpty($output, 'PHP process returned empty output'); + $passthroughExceptions = json_decode($output, true); + $this->assertNotNull($passthroughExceptions, 'Failed to decode JSON output: ' . $output);Alternatively, consider PHPUnit's built-in process isolation:
/** * @runInSeparateProcess * @preserveGlobalState disabled */ public function testBootLoadsPassthroughExceptionsFromConfig() { $app = Boot('examples/config'); $passthroughExceptions = Registry::getInstance()->get('PassthroughExceptions'); // ... assertions }
207-230: Apply same error handling improvements.This test has the same reliability concerns as
testBootLoadsPassthroughExceptionsFromConfig. Please apply similar error handling for shell_exec and json_decode, or consider using PHPUnit's@runInSeparateProcessannotation.src/Cms/Controllers/Blog.php (1)
126-126: Redundant null-safe operator on non-nullable property.Since
$_rendereris now a required non-nullable dependency, the null-safe operator (?->) is unnecessary. Consider simplifying to a direct method call.🔎 Suggested fix
- $renderedContent = $this->_renderer?->render( $content ) ?? (is_array($content) ? json_encode($content) : $content); + $renderedContent = $this->_renderer->render( $content );
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
examples/config/neuron.yamlresources/config/neuron.yamlsrc/Bootstrap.phpsrc/Cms/Controllers/Admin/Categories.phpsrc/Cms/Controllers/Admin/Dashboard.phpsrc/Cms/Controllers/Admin/EventCategories.phpsrc/Cms/Controllers/Admin/Events.phpsrc/Cms/Controllers/Admin/Media.phpsrc/Cms/Controllers/Admin/Pages.phpsrc/Cms/Controllers/Admin/Posts.phpsrc/Cms/Controllers/Admin/Profile.phpsrc/Cms/Controllers/Admin/Tags.phpsrc/Cms/Controllers/Admin/Users.phpsrc/Cms/Controllers/Auth/Login.phpsrc/Cms/Controllers/Auth/PasswordReset.phpsrc/Cms/Controllers/Blog.phpsrc/Cms/Controllers/Calendar.phpsrc/Cms/Controllers/Member/Profile.phpsrc/Cms/Controllers/Member/Registration.phpsrc/Cms/Controllers/Pages.phptests/Unit/BootstrapTest.phptests/Unit/Cms/Auth/CsrfFilterTest.phptests/Unit/Cms/BlogControllerTest.phptests/Unit/Cms/Controllers/Admin/CategoriesTest.phptests/Unit/Cms/Controllers/Admin/PostsTest.phptests/Unit/Cms/Controllers/Admin/UsersTest.phptests/Unit/Cms/Controllers/Auth/PasswordResetTest.phptests/Unit/Cms/Controllers/Member/RegistrationTest.php
💤 Files with no reviewable changes (1)
- src/Bootstrap.php
🧰 Additional context used
🧬 Code graph analysis (12)
tests/Unit/Cms/BlogControllerTest.php (1)
src/Cms/Models/Post.php (1)
getSlug(96-99)
src/Cms/Controllers/Admin/Dashboard.php (2)
src/Cms/Controllers/Content.php (1)
getSessionManager(233-247)src/Cms/Auth/SessionManager.php (1)
getFlash(170-176)
src/Cms/Controllers/Admin/Posts.php (8)
src/Cms/Controllers/Admin/Categories.php (1)
__construct(42-56)src/Cms/Controllers/Admin/Dashboard.php (1)
__construct(31-38)src/Cms/Controllers/Admin/EventCategories.php (1)
__construct(43-57)src/Cms/Controllers/Admin/Media.php (1)
__construct(42-54)src/Cms/Controllers/Admin/Pages.php (1)
__construct(46-60)src/Cms/Controllers/Admin/Tags.php (1)
__construct(40-52)src/Cms/Controllers/Auth/Login.php (1)
__construct(35-45)src/Cms/Controllers/Blog.php (1)
__construct(43-61)
src/Cms/Controllers/Admin/Profile.php (2)
src/Cms/Controllers/Admin/Users.php (1)
__construct(44-60)src/Cms/Controllers/Member/Profile.php (1)
__construct(39-53)
src/Cms/Controllers/Auth/Login.php (1)
src/Cms/Services/Auth/Authentication.php (1)
attempt(42-133)
src/Cms/Controllers/Calendar.php (2)
src/Cms/Controllers/Admin/Events.php (1)
__construct(47-63)src/Cms/Controllers/Pages.php (1)
__construct(41-53)
src/Cms/Controllers/Admin/Users.php (6)
src/Cms/Controllers/Admin/Dashboard.php (1)
__construct(31-38)src/Cms/Controllers/Admin/Media.php (1)
__construct(42-54)src/Cms/Controllers/Admin/Profile.php (1)
__construct(40-54)src/Cms/Controllers/Admin/Tags.php (1)
__construct(40-52)src/Cms/Controllers/Auth/Login.php (1)
__construct(35-45)src/Cms/Controllers/Member/Profile.php (1)
__construct(39-53)
src/Cms/Controllers/Member/Profile.php (2)
src/Cms/Controllers/Admin/Profile.php (1)
__construct(40-54)src/Cms/Controllers/Admin/Users.php (1)
__construct(44-60)
src/Cms/Controllers/Admin/EventCategories.php (3)
src/Cms/Controllers/Admin/Dashboard.php (1)
__construct(31-38)src/Cms/Controllers/Admin/Events.php (1)
__construct(47-63)src/Cms/Controllers/Admin/Pages.php (1)
__construct(46-60)
src/Cms/Controllers/Member/Registration.php (18)
src/Cms/Controllers/Admin/Categories.php (1)
__construct(42-56)src/Cms/Controllers/Admin/Dashboard.php (1)
__construct(31-38)src/Cms/Controllers/Admin/EventCategories.php (1)
__construct(43-57)src/Cms/Controllers/Admin/Events.php (1)
__construct(47-63)src/Cms/Controllers/Admin/Media.php (1)
__construct(42-54)src/Cms/Controllers/Admin/Pages.php (1)
__construct(46-60)src/Cms/Controllers/Admin/Posts.php (1)
__construct(54-74)src/Cms/Controllers/Admin/Profile.php (1)
__construct(40-54)src/Cms/Controllers/Admin/Tags.php (1)
__construct(40-52)src/Cms/Controllers/Admin/Users.php (1)
__construct(44-60)src/Cms/Controllers/Auth/Login.php (1)
__construct(35-45)src/Cms/Controllers/Auth/PasswordReset.php (1)
__construct(37-46)src/Cms/Controllers/Blog.php (1)
__construct(43-61)src/Cms/Controllers/Member/Profile.php (1)
__construct(39-53)src/Cms/Controllers/Content.php (1)
__construct(77-110)src/Cms/Controllers/Member/Dashboard.php (1)
__construct(32-39)src/Cms/Auth/SessionManager.php (1)
SessionManager(13-216)src/Cms/Services/Security/ResendVerificationThrottle.php (1)
ResendVerificationThrottle(19-195)
src/Cms/Controllers/Auth/PasswordReset.php (11)
src/Cms/Controllers/Admin/Categories.php (1)
__construct(42-56)src/Cms/Controllers/Admin/EventCategories.php (1)
__construct(43-57)src/Cms/Controllers/Admin/Events.php (1)
__construct(47-63)src/Cms/Controllers/Admin/Media.php (1)
__construct(42-54)src/Cms/Controllers/Admin/Pages.php (1)
__construct(46-60)src/Cms/Controllers/Admin/Profile.php (1)
__construct(40-54)src/Cms/Controllers/Admin/Tags.php (1)
__construct(40-52)src/Cms/Controllers/Admin/Users.php (1)
__construct(44-60)src/Cms/Controllers/Auth/Login.php (1)
__construct(35-45)src/Cms/Controllers/Member/Profile.php (1)
__construct(39-53)src/Cms/Controllers/Content.php (1)
__construct(77-110)
src/Cms/Controllers/Blog.php (3)
src/Cms/Models/Post.php (2)
isPublished(268-271)incrementViewCount(326-330)src/Cms/Models/Page.php (2)
isPublished(282-285)incrementViewCount(332-336)src/Cms/Repositories/DatabasePostRepository.php (1)
incrementViewCount(300-310)
🔇 Additional comments (33)
src/Cms/Controllers/Auth/Login.php (1)
105-105: LGTM! Good defensive programming.The null coalescing operator ensures a boolean value is always passed to the authentication method, even if
$dto->rememberis null. While theattemptmethod already has a default parameter, this explicit handling at the call site improves type safety and makes the intent clearer.examples/config/neuron.yaml (1)
58-72: LGTM! Clear exception passthrough configuration.The new exception handling configuration is well-documented and provides a clear mechanism for routing CMS-specific exceptions to application-level handlers. The chosen exceptions (authentication, email verification, CSRF) are appropriate candidates for custom handling flows.
tests/Unit/Cms/Auth/CsrfFilterTest.php (1)
266-278: LGTM! Test now correctly verifies POST token precedence.The updated test assertions and comments accurately reflect that the implementation now uses
Neuron\Data\Filters\Postto read POST data, which works correctly in both unit tests and real requests. The assertion correctly expects the POST token to take precedence over the header token.tests/Unit/Cms/Controllers/Admin/PostsTest.php (1)
88-88: LGTM! Correct exception type for non-nullable parameters.The change to expect
TypeErrorcorrectly reflects the updated controller constructor that now requires non-nullable dependencies. PHP's type system throwsTypeErrorwhen null is passed to non-nullable typed parameters.tests/Unit/Cms/Controllers/Admin/UsersTest.php (1)
81-81: LGTM! Correct exception type for non-nullable parameters.The change to expect
TypeErrorcorrectly reflects the updated controller constructor that now requires non-nullable dependencies. This aligns with the broader refactoring to enforce dependency presence at construction time.tests/Unit/Cms/Controllers/Auth/PasswordResetTest.php (1)
112-120: LGTM!The test correctly expects
TypeErrorwhen passingnullto a non-nullable constructor parameter. This aligns with PHP's type-hint enforcement and the broader refactoring to non-nullable dependencies.tests/Unit/Cms/Controllers/Member/RegistrationTest.php (1)
127-185: LGTM!All four tests correctly expect
TypeErrorwhen passingnullto non-nullable constructor parameters. The changes are consistent and align with the refactoring to enforce required dependencies via type hints.tests/Unit/Cms/Controllers/Admin/CategoriesTest.php (1)
85-100: LGTM!The test correctly expects
TypeErrorwhen passingnullto non-nullable constructor parameters. This change is consistent with the refactoring pattern throughout the PR.src/Cms/Controllers/Admin/Dashboard.php (2)
7-7: LGTM!The
FlashMessageTypeimport is correctly added to support the flash message retrieval below.
58-59: LGTM!The flash message retrieval implementation is correct. Using
FlashMessageTypeenum values ensures type safety, and the view will receive either the flash message content ornullif no message was set.resources/config/neuron.yaml (1)
85-98: Configuration-driven exception handling is correct.All three exception classes referenced in the
exceptions.passthroughconfiguration exist and are properly defined:
Neuron\Cms\Exceptions\UnauthenticatedExceptionNeuron\Cms\Exceptions\EmailVerificationRequiredExceptionNeuron\Cms\Exceptions\CsrfValidationExceptionThe configuration appropriately centralizes these CMS-specific exceptions for passthrough handling to support custom redirects and error pages.
tests/Unit/Cms/BlogControllerTest.php (3)
389-389: LGTM!Route parameter correctly updated to use
slugkey, aligning with the tag model'sgetSlug()method and the controller's route parameter expectations.
407-407: LGTM!Route parameter correctly updated to use
slugkey for category filtering, consistent with the pattern established for tags.
425-430: LGTM!The mock expectation correctly reflects the renamed route parameter from
authortousername, which is semantically more accurate for user identification.src/Cms/Controllers/Auth/PasswordReset.php (1)
35-46: LGTM!The constructor now requires a non-nullable
IPasswordResetterdependency, which is the correct approach when relying on a DI container for dependency resolution. This removes redundant runtime null checks and aligns with the consistent pattern across other controllers in this PR (e.g.,Login.php,Media.php,Tags.php).src/Cms/Controllers/Admin/Pages.php (1)
42-60: LGTM!The constructor signature correctly requires non-nullable dependencies for
IPageRepository,IPageCreator, andIPageUpdater. This approach:
- Leverages PHP's type system for enforcement (TypeError on null)
- Reduces boilerplate by removing manual null checks
- Is consistent with the DI container pattern used throughout the codebase
The implementation aligns with other admin controllers in this PR.
src/Cms/Controllers/Admin/Categories.php (1)
38-56: LGTM!Constructor correctly updated to require non-nullable
ICategoryRepository,ICategoryCreator, andICategoryUpdaterdependencies. This maintains consistency with the DI pattern established across all admin controllers in this PR.src/Cms/Controllers/Admin/Users.php (1)
39-60: LGTM!Constructor correctly updated to require non-nullable dependencies for all four user service interfaces (
IUserRepository,IUserCreator,IUserUpdater,IUserDeleter). The implementation is consistent with the DI pattern used across other admin controllers.src/Cms/Controllers/Admin/Events.php (1)
42-63: LGTM!Constructor correctly updated to require non-nullable dependencies for
IEventRepository,IEventCategoryRepository,IEventCreator, andIEventUpdater. This is consistent with the DI pattern established across all admin controllers and aligns with the publicCalendarcontroller which uses the same repository interfaces.src/Cms/Controllers/Calendar.php (1)
33-49: LGTM!Constructor correctly updated to require non-nullable
IEventRepositoryandIEventCategoryRepositorydependencies. This is consistent with theAdmin/Eventscontroller which uses the same repository interfaces, maintaining a unified DI pattern across both public and admin event-related controllers.src/Cms/Controllers/Admin/Media.php (1)
38-39: LGTM! Constructor dependencies now strictly enforced.The refactoring from nullable optional dependencies to required non-null parameters is a good practice. This moves validation from runtime (InvalidArgumentException) to type-check time (TypeError), leveraging PHP's type system and ensuring the DI container provides all required dependencies.
Also applies to: 46-47, 52-53
src/Cms/Controllers/Admin/EventCategories.php (1)
38-40: LGTM! Consistent DI enforcement pattern.The constructor refactoring aligns with the broader PR pattern of requiring non-nullable dependencies. This is consistent with similar changes in Events.php, Pages.php, and other admin controllers (see relevant code snippets), ensuring uniform DI practices across the codebase.
Also applies to: 47-49, 54-56
src/Cms/Controllers/Admin/Tags.php (1)
36-37: LGTM! Dependencies properly enforced via type system.This refactoring is consistent with the PR-wide pattern of removing nullable defaults and runtime null checks in favor of strict type enforcement. The change ensures the DI container must provide concrete implementations, improving reliability.
Also applies to: 44-45, 50-51
src/Cms/Controllers/Admin/Posts.php (1)
47-52: LGTM! Constructor refactoring consistent across all dependencies.The Posts controller now requires all six dependencies (repositories, creator, updater, deleter) to be provided via DI, matching the pattern established in Categories.php, Tags.php, Media.php, and other admin controllers. This improves type safety and ensures the DI container handles all dependency provision.
Also applies to: 58-63, 68-73
src/Cms/Controllers/Pages.php (1)
37-52: LGTM!The constructor now enforces non-nullable dependencies via PHP's type system, delegating validation to the DI container. This aligns with the broader refactoring pattern across the codebase and eliminates redundant runtime null checks.
src/Cms/Controllers/Admin/Profile.php (1)
35-53: LGTM!The constructor signature now matches the pattern established in
Member/Profile.phpand other admin controllers, enforcing required dependencies at construction time rather than via runtime guards.src/Cms/Controllers/Blog.php (5)
36-60: LGTM!The constructor now enforces all five repository/renderer dependencies as non-nullable, consistent with the PR-wide DI pattern. This ensures the DI container is responsible for providing valid instances.
105-132: Good improvement to HTTP semantics and content rendering.Returning
NOT_FOUNDstatus for missing/unpublished posts is correct HTTP behavior. The fallback rendering for empty Editor.js content (lines 128-132) provides graceful degradation. MovingincrementViewCountto the else branch correctly avoids counting views for non-existent content.
154-157: LGTM!Route parameter renamed from generic to semantic
username, matching the route definition/author/:username.
193-196: LGTM!Route parameter renamed to
slug, matching the route definition/tag/:slug.
234-237: LGTM!Route parameter renamed to
slug, matching the route definition/category/:slug.src/Cms/Controllers/Member/Profile.php (1)
34-52: LGTM!The constructor signature mirrors
Admin/Profile.phpexactly, enforcing consistent non-nullable DI across both member and admin profile controllers.src/Cms/Controllers/Member/Registration.php (1)
39-60: LGTM!The constructor now requires all four dependencies (
IRegistrationService,IEmailVerifier,ResendVerificationThrottle,IIpResolver) as non-nullable, consistent with the PR-wide pattern. The DI container configuration must provide these services.
Note
Introduces config-driven exception passthrough and tightens dependency injection across controllers while refining public/admin flows.
exceptions.passthroughtoneuron.yaml(examples/resources); Registry now loadsPassthroughExceptionsfrom configBootstrapand add tests verifying config-driven loadingTypeErrorinstead ofInvalidArgumentExceptionusername/slug), and content fallback when Editor.js output is emptyError/Success) to viewremembertofalsewhen absentWritten by Cursor Bugbot for commit 1aee669. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.