updates tests #26
updates tests #26
Conversation
📝 WalkthroughWalkthroughWidespread DI refactor: controllers and services now require explicit SettingManager and SessionManager injections (removing container fallbacks); several service constructors accept optional event emitters or custom RNGs; many unit tests added/updated to exercise constructor validation, event emission, and media/widget behaviors. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(46,134,193,0.06)
participant Test as Test (unit)
participant DTO as DTO
participant Creator as Service\Creator
participant Repo as Repository
participant Emitter as EventEmitter
note right of Emitter `#f6e58d`: optional dependency injected
end
Note over Test,DTO: Arrange DTO and mocks
Test->>Creator: create(dto)
Creator->>Repo: repository->create(entity)
Repo-->>Creator: created entity
alt emitter provided
Creator->>Emitter: emit(CategoryCreatedEvent)
Emitter-->>Test: event observed (mock expectation)
end
Note over Test: Assert repo called and event emitted (when emitter present)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/Unit/Cms/Services/EmailVerifierTest.php (1)
374-410: Good dependency injection test.The test effectively verifies that a custom random generator can be injected and is correctly wired into the token creation flow. The test structure is clear and follows good isolation practices.
Consider optionally enhancing the assertion by capturing and verifying the actual token value passed to
create()matches the custom random output. This would provide stronger confidence that the dependency is used correctly throughout the flow.💡 Optional enhancement to verify token value
$tokenRepository ->expects( $this->once() ) ->method( 'create' ) - ->with( $this->isInstanceOf( EmailVerificationToken::class ) ); + ->with( $this->callback( function( $token ) { + return $token instanceof EmailVerificationToken && + $token->getToken() === hash( 'sha256', str_repeat( 'a', 64 ) ); + } ) );
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
tests/Unit/Cms/Services/Category/CreatorTest.phptests/Unit/Cms/Services/Category/UpdaterTest.phptests/Unit/Cms/Services/EmailVerifierTest.phptests/Unit/Cms/Services/Event/UpdaterTest.phptests/Unit/Cms/Services/EventCategory/UpdaterTest.phptests/Unit/Cms/Services/Page/UpdaterTest.phptests/Unit/Cms/Services/PasswordResetterTest.phptests/Unit/Cms/Services/User/CreatorTest.phptests/Unit/Cms/Services/User/UpdaterTest.php
🧰 Additional context used
🧬 Code graph analysis (7)
tests/Unit/Cms/Services/User/CreatorTest.php (1)
src/Cms/Events/UserCreatedEvent.php (1)
UserCreatedEvent(13-23)
tests/Unit/Cms/Services/Category/CreatorTest.php (1)
src/Cms/Events/CategoryCreatedEvent.php (1)
CategoryCreatedEvent(13-23)
tests/Unit/Cms/Services/Category/UpdaterTest.php (1)
src/Cms/Events/CategoryUpdatedEvent.php (1)
CategoryUpdatedEvent(13-23)
tests/Unit/Cms/Services/Page/UpdaterTest.php (2)
tests/Unit/Cms/Services/Category/UpdaterTest.php (1)
testConstructorSetsPropertiesCorrectly(208-215)tests/Unit/Cms/Services/User/UpdaterTest.php (1)
testConstructorSetsPropertiesCorrectly(255-263)
tests/Unit/Cms/Services/PasswordResetterTest.php (3)
src/Cms/Auth/PasswordHasher.php (1)
hash(24-30)src/Cms/Models/PasswordResetToken.php (1)
PasswordResetToken(14-178)src/Cms/Services/Auth/PasswordResetter.php (1)
PasswordResetter(23-244)
tests/Unit/Cms/Services/Event/UpdaterTest.php (1)
tests/Unit/Cms/Services/EventCategory/UpdaterTest.php (1)
test_constructor_sets_properties_correctly(284-291)
tests/Unit/Cms/Services/User/UpdaterTest.php (2)
src/Cms/Models/User.php (3)
setUsername(100-104)setRole(151-155)setPasswordHash(134-138)src/Cms/Events/UserUpdatedEvent.php (1)
UserUpdatedEvent(13-23)
⏰ 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). (3)
- GitHub Check: build-test (sqlite)
- GitHub Check: build-test (mysql)
- GitHub Check: build-test (postgres)
🔇 Additional comments (19)
tests/Unit/Cms/Services/Page/UpdaterTest.php (2)
268-275: LGTM!The constructor test is straightforward and follows the same pattern as similar tests in
Category/UpdaterTest.phpandUser/UpdaterTest.php.
277-299: LGTM!The not-found exception test properly configures the mock to return
nulland verifies that the expected exception with the correct message is thrown. This aligns with the broader PR pattern of adding error handling tests across Updater services.tests/Unit/Cms/Services/PasswordResetterTest.php (2)
277-303: LGTM!This test covers an important edge case where a user is deleted after a password reset token was created. The test correctly verifies that
resetPasswordreturnsfalsewhen the token is valid but the associated user no longer exists.
305-341: LGTM!This test properly validates the custom
IRandomdependency injection. It verifies that:
- The custom random generator is used (via
expects($this->once()))- The correct parameters are passed (
64, 'hex')- A
PasswordResetTokenis created with the generated tokenThe test aligns with the
PasswordResetterconstructor signature that accepts an optionalIRandomparameter.tests/Unit/Cms/Services/Category/UpdaterTest.php (3)
208-215: LGTM!Standard constructor instantiation test following the established pattern.
217-248: LGTM!The test correctly validates that
CategoryUpdatedEventis emitted when an event emitter is injected. Thenullsecond parameter suggests there's an intermediate dependency (possibly a slug generator or similar) between the repository and event emitter in the constructor signature.
250-268: LGTM!The not-found exception test properly validates error handling when attempting to update a non-existent category.
tests/Unit/Cms/Services/Category/CreatorTest.php (2)
159-166: LGTM!Standard constructor instantiation test.
168-191: LGTM!The test properly validates that
CategoryCreatedEventis emitted when creating a category with an injected event emitter. The constructor signature with(categoryRepository, null, eventEmitter)is consistent with theUpdaterTestpattern in the same module.tests/Unit/Cms/Services/Event/UpdaterTest.php (2)
416-424: LGTM!Constructor test follows the established snake_case naming convention used throughout this test file.
426-444: Consider exception type consistency across services.This test expects
\RuntimeException, while similar tests inPage/UpdaterTestandCategory/UpdaterTestexpect\Exception. If this difference is intentional (e.g., the Event Updater implementation throwsRuntimeException), this is fine. Otherwise, consider aligning exception types across all Updater services for consistency.tests/Unit/Cms/Services/EventCategory/UpdaterTest.php (2)
284-291: LGTM!Constructor test follows the snake_case naming pattern established in this test file.
293-311: LGTM!The not-found exception test properly verifies error handling. The use of
\RuntimeExceptionis consistent with theEvent/UpdaterTestin the same domain area.tests/Unit/Cms/Services/User/CreatorTest.php (2)
238-246: LGTM!Standard constructor instantiation test following the camelCase naming convention used in this test file.
248-282: LGTM!The test comprehensively validates event emission when creating a user with an injected event emitter. The mock setup properly configures
PasswordHasherrequirements and hash generation, ensuring the fullcreate()flow executes to trigger the event.tests/Unit/Cms/Services/User/UpdaterTest.php (3)
255-263: LGTM!Standard constructor instantiation test.
265-301: LGTM!The test properly validates
UserUpdatedEventemission with an injected event emitter. The user entity is correctly configured with all required fields before the update operation.
303-322: LGTM!The not-found exception test properly validates error handling when updating a non-existent user. Uses
\Exceptionwhich is consistent with other domain services like Page and Category.tests/Unit/Cms/Services/EmailVerifierTest.php (1)
346-372: Test correctly validates the implementation behavior.The test accurately reflects the current implementation: when a token exists but the user is not found (deleted),
verifyEmail()returnsfalsewithout deleting the token. The test properly expects no call todeleteByToken(), which matches lines 143-146 of the EmailVerifier implementation.The original concern about orphaned tokens is a valid architectural consideration, but the test correctly validates the existing behavior. If cleanup of orphaned tokens is desired, that would require changes to the implementation, not the test.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
tests/Unit/Cms/Controllers/CalendarTest.php (1)
35-41: Same file path and error suppression issues as in PagesTest.This test has the same brittle file path assumption and error suppression issues identified in PagesTest.php. Consider extracting the common setup/teardown logic to a base test class to reduce duplication and ensure consistent fixes.
💡 Suggestion: Create base test class
Create a base class like
BaseControllerTestCasethat handles common setup/teardown:abstract class BaseControllerTestCase extends TestCase { protected SettingManager $_settingManager; protected string $_tempDir; protected function setUp(): void { parent::setUp(); // Create temporary directory for test artifacts $this->_tempDir = sys_get_temp_dir() . '/neuron_test_' . uniqid(); mkdir( $this->_tempDir, 0777, true ); // Setup settings $settings = new Memory(); $settings->set( 'site', 'name', 'Test Site' ); // ... rest of settings $this->_settingManager = new SettingManager( $settings ); Registry::getInstance()->set( 'Settings', $this->_settingManager ); // Create version file in temp directory file_put_contents( $this->_tempDir . '/.version.json', json_encode([ 'major' => 1, 'minor' => 0, 'patch' => 0 ]) ); } protected function tearDown(): void { // Clear registry Registry::getInstance()->set( 'Settings', null ); Registry::getInstance()->set( 'version', null ); // ... rest of cleanup // Clean up temp directory if( is_dir( $this->_tempDir ) ) { array_map( 'unlink', glob( "$this->_tempDir/*" ) ); rmdir( $this->_tempDir ); } parent::tearDown(); } }Also applies to: 52-53
tests/Unit/Cms/Controllers/HomeTest.php (1)
35-46: Same file path and error suppression issues.This test file has the same issues identified in PagesTest.php and CalendarTest.php. Refer to those comments for details and the suggestion to create a base test class.
Also applies to: 58-60
tests/Unit/Cms/Controllers/Admin/DashboardTest.php (1)
32-38: Same file path and error suppression issues.This file now uses the same setup/teardown pattern as the other test files, with the same brittle file path assumption and error suppression issues. See comments in PagesTest.php for details.
Also applies to: 50-51
🧹 Nitpick comments (13)
tests/Unit/Cms/Services/User/DeleterTest.php (2)
84-91: Test name could be more descriptive.The test name suggests it verifies properties are set correctly, but it only checks that the constructor instantiates the class without throwing an exception. While this pattern is consistent with similar tests in the codebase (e.g.,
CategoryUpdaterTest), consider either:
- Renaming to
testConstructorInstantiatesCorrectly()for accuracy- Or enhancing the test to actually verify that the repository property is correctly set
93-118: LGTM! Consider verifying event payload.The test correctly verifies that a
UserDeletedEventis emitted when the event emitter is provided. The implementation follows the same pattern as similar tests in the codebase (CategoryDeleterTest,CreatorTest,UpdaterTest).Optionally, you could enhance the test to verify the event's
userIdproperty matches the deleted user's ID:🔎 Optional enhancement to verify event payload
// Event emitter should emit UserDeletedEvent $eventEmitter ->expects( $this->once() ) ->method( 'emit' ) - ->with( $this->isInstanceOf( \Neuron\Cms\Events\UserDeletedEvent::class ) ); + ->with( $this->callback( function( $event ) { + return $event instanceof \Neuron\Cms\Events\UserDeletedEvent + && $event->userId === 1; + } ) );tests/Unit/Cms/Services/Category/DeleterTest.php (1)
84-91: Test name doesn't match behavior.The test is named
testConstructorSetsPropertiesCorrectlybut only asserts that the instance is created. It doesn't verify that any properties are correctly set. Consider renaming totestConstructorCreatesInstanceortestCanInstantiateWithRepositoryto accurately reflect what the test validates.🔎 Suggested rename
- public function testConstructorSetsPropertiesCorrectly(): void + public function testCanInstantiateWithRepository(): void { $categoryRepository = $this->createMock( ICategoryRepository::class ); $deleter = new Deleter( $categoryRepository ); $this->assertInstanceOf( Deleter::class, $deleter ); }tests/Unit/Cms/Controllers/Member/DashboardTest.php (2)
32-38: Potential race condition with shared version file.The test creates
.version.jsonin a shared location that could be accessed by parallel test runs. While this pattern is consistent with other controller tests (Admin Dashboard, Home), it may cause race conditions if multiple tests run concurrently.Consider using a unique temporary directory per test or leveraging PHPUnit's
@tempDirfunctionality to avoid file system conflicts.Based on relevant code snippets showing the same pattern in Admin/Home controller tests.
69-98: Consider reducing mocking for stronger test confidence.The test mocks both the controller and the ViewContext, which means it's verifying that mocks return what they're configured to return rather than testing the actual Dashboard behavior. This pattern is consistent with the Admin Dashboard test, but it provides limited confidence that the real
index()method works correctly.For stronger test coverage, consider either:
- Testing the real controller with a mocked ViewContext (remove controller mock)
- Adding an integration test that uses the real view pipeline
This would verify that
Dashboard::index()actually callsview()with the correct template and chains the fluent methods in the proper order.Based on relevant code snippets showing the same pattern in Admin Dashboard tests.
tests/Unit/Cms/Services/Media/CloudinaryUploaderTest.php (2)
210-398: Consider extracting the integration test skip logic.The credential check and skip logic is duplicated across six integration tests (lines 213-222, 244-253, 267-276, 289-298, 328-337, 357-366). This violates the DRY principle and makes the tests harder to maintain.
🔎 Suggested refactor to extract skip logic
Add a helper method to the test class:
/** * Skip test if real Cloudinary credentials are not configured */ private function skipIfNoRealCredentials(): void { $cloudName = $this->_settings->get( 'cloudinary', 'cloud_name' ); $isTestCredentials = ($cloudName === 'test-cloud'); $hasRealCredentials = getenv( 'CLOUDINARY_URL' ) || (!$isTestCredentials && $cloudName); if( !$hasRealCredentials ) { $this->markTestSkipped( 'Cloudinary credentials not configured. Set CLOUDINARY_URL environment variable or configure real cloudinary settings to run this integration test.' ); } }Then replace each duplicated block with a single call:
public function testUploadWithValidFile(): void { - // Skip if using test credentials (not real Cloudinary account) - $cloudName = $this->_settings->get( 'cloudinary', 'cloud_name' ); - $isTestCredentials = ($cloudName === 'test-cloud'); - $hasRealCredentials = getenv( 'CLOUDINARY_URL' ) || (!$isTestCredentials && $cloudName); - - if( !$hasRealCredentials ) - { - $this->markTestSkipped( - 'Cloudinary credentials not configured. Set CLOUDINARY_URL environment variable or configure real cloudinary settings to run this integration test.' - ); - } + $this->skipIfNoRealCredentials(); // Integration test requires actual file and credentials ...Apply the same pattern to all six integration tests.
404-879: Extract the repeated reflection-based mock injection pattern.The reflection code to inject mocked Cloudinary instances is duplicated across 11 test methods. This duplication reduces maintainability and increases the cognitive load when reading tests.
🔎 Suggested refactor to extract mock injection helper
Add a helper method to the test class:
/** * Inject a mocked Cloudinary instance into the uploader */ private function injectMockCloudinary( CloudinaryUploader $uploader, $mockCloudinary ): void { $reflection = new \ReflectionClass( $uploader ); $property = $reflection->getProperty( '_cloudinary' ); $property->setAccessible( true ); $property->setValue( $uploader, $mockCloudinary ); }Then replace each duplicated reflection block:
public function testUploadCallsCloudinaryApiCorrectly(): void { // ... create mocks ... - // Use reflection to inject the mock - $reflection = new \ReflectionClass( $uploader ); - $property = $reflection->getProperty( '_cloudinary' ); - $property->setAccessible( true ); - $property->setValue( $uploader, $mockCloudinary ); + $this->injectMockCloudinary( $uploader, $mockCloudinary ); // Test the upload $result = $uploader->upload( $testFile ); ...Apply this pattern to all 11 tests that use reflection for mock injection (lines 447-451, 513-517, 568-572, 601-605, 629-633, 692-696, 739-743, 777-781, 813-817, 841-845, 869-873).
tests/Unit/Cms/Services/Widget/CalendarWidgetTest.php (1)
345-353: Constructor test is functional but could be enhanced.The test verifies that the constructor accepts dependencies and returns the correct instance type. While this is valid, it doesn't confirm that the injected repositories are actually stored and accessible.
💡 Optional: Consider verifying dependency usage
If you want to strengthen this test, you could verify that the injected dependencies are actually used by calling a method that requires them:
public function testConstructorSetsPropertiesCorrectly(): void { $eventRepo = $this->createMock( DatabaseEventRepository::class ); $categoryRepo = $this->createMock( DatabaseEventCategoryRepository::class ); + + // Verify the repositories are actually used + $eventRepo->expects( $this->once() ) + ->method( 'getUpcoming' ) + ->willReturn( [] ); $widget = new CalendarWidget( $eventRepo, $categoryRepo ); $this->assertInstanceOf( CalendarWidget::class, $widget ); + $widget->render( [] ); // Trigger dependency usage }However, this is optional since the existing tests already verify dependency usage in various scenarios.
tests/Unit/Cms/Controllers/Admin/ProfileTest.php (2)
54-54: Avoid error suppression; handle file deletion explicitly.The
@operator suppresses all errors fromunlink(), which can hide legitimate issues like permission problems or I/O errors during cleanup.🔎 Proposed improvement
- $parentDir = dirname( getcwd() ); - @unlink( $parentDir . '/.version.json' ); + if( isset( $this->_versionFilePath ) && file_exists( $this->_versionFilePath ) ) + { + unlink( $this->_versionFilePath ); + }
69-78: Consider adding assertions to verify dependency injection.While the test successfully verifies instantiation, it could be strengthened by confirming that the dependencies are properly stored. If the Profile controller exposes getters or has public properties for its dependencies, consider asserting those values.
Example enhancement (if applicable):
$controller = new Profile( null, $mockRepository, $mockHasher, $mockUpdater ); $this->assertInstanceOf( Profile::class, $controller ); // If getters exist: // $this->assertSame( $mockRepository, $controller->getRepository() ); // $this->assertSame( $mockHasher, $controller->getHasher() ); // $this->assertSame( $mockUpdater, $controller->getUpdater() );Note: The comment acknowledging integration testing requirements is helpful documentation.
tests/Unit/Cms/Controllers/Member/ProfileTest.php (1)
69-78: Consider expanding test assertions within unit test constraints.While the integration test note is appreciated, the current test only verifies that the constructor doesn't throw an exception. Consider adding assertions to verify that the mocked dependencies can be accessed through the controller instance (e.g., via getters or by verifying method calls), provided this doesn't require the complex infrastructure mentioned in the comment.
However, if the Profile controller lacks public accessors and requires full integration context, the current minimal test is acceptable.
tests/Unit/Cms/Controllers/PagesTest.php (1)
61-63: Avoid error suppression operator.Using
@unlinksuppresses all errors, which can hide legitimate issues during test execution. Instead, check if the file exists before attempting to delete it.🔎 Proposed fix
- $parentDir = dirname( getcwd() ); - @unlink( $parentDir . '/.version.json' ); + $parentDir = dirname( getcwd() ); + $versionFile = $parentDir . '/.version.json'; + if( file_exists( $versionFile ) ) + { + unlink( $versionFile ); + }tests/Unit/Cms/Controllers/HomeTest.php (1)
168-196: Minor test duplication.This test largely duplicates the assertions from
testIndexWithRegistrationEnabled(lines 81-111). Consider whether this separate test adds sufficient value, or if the verification in the earlier test is adequate.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
tests/Unit/Cms/Controllers/Admin/DashboardTest.phptests/Unit/Cms/Controllers/Admin/ProfileTest.phptests/Unit/Cms/Controllers/CalendarTest.phptests/Unit/Cms/Controllers/HomeTest.phptests/Unit/Cms/Controllers/Member/DashboardTest.phptests/Unit/Cms/Controllers/Member/ProfileTest.phptests/Unit/Cms/Controllers/PagesTest.phptests/Unit/Cms/Services/Category/DeleterTest.phptests/Unit/Cms/Services/Media/CloudinaryUploaderTest.phptests/Unit/Cms/Services/User/DeleterTest.phptests/Unit/Cms/Services/Widget/CalendarWidgetTest.phptests/Unit/Cms/Services/Widget/WidgetRendererTest.php
🧰 Additional context used
🧬 Code graph analysis (9)
tests/Unit/Cms/Controllers/Member/ProfileTest.php (1)
src/Cms/Auth/PasswordHasher.php (1)
PasswordHasher(13-204)
tests/Unit/Cms/Services/Widget/CalendarWidgetTest.php (1)
src/Cms/Services/Widget/CalendarWidget.php (1)
CalendarWidget(15-147)
tests/Unit/Cms/Services/Widget/WidgetRendererTest.php (2)
src/Cms/Services/Widget/WidgetRenderer.php (1)
WidgetRenderer(19-146)tests/Unit/Cms/Services/Widget/WidgetTest.php (1)
render(16-19)
tests/Unit/Cms/Services/User/DeleterTest.php (5)
tests/Unit/Cms/Services/Category/DeleterTest.php (2)
testConstructorSetsPropertiesCorrectly(84-91)testConstructorWithEventEmitter(93-118)tests/Unit/Cms/Services/Category/UpdaterTest.php (2)
testConstructorSetsPropertiesCorrectly(208-215)testConstructorWithEventEmitter(217-248)tests/Unit/Cms/Services/User/CreatorTest.php (2)
testConstructorSetsPropertiesCorrectly(238-246)testConstructorWithEventEmitter(248-282)tests/Unit/Cms/Services/User/UpdaterTest.php (2)
testConstructorSetsPropertiesCorrectly(255-263)testConstructorWithEventEmitter(265-301)src/Cms/Events/UserDeletedEvent.php (1)
UserDeletedEvent(12-22)
tests/Unit/Cms/Controllers/Member/DashboardTest.php (3)
tests/Unit/Cms/Controllers/Admin/DashboardTest.php (6)
DashboardTest(14-99)setUp(18-39)tearDown(41-54)testConstructor(56-60)testConstructorWithApplication(62-67)testIndexRendersView(73-98)tests/Unit/Cms/Controllers/HomeTest.php (2)
setUp(18-47)tearDown(49-63)src/Cms/Container/Container.php (1)
getInstance(251-254)
tests/Unit/Cms/Controllers/PagesTest.php (1)
src/Cms/Services/Content/EditorJsRenderer.php (1)
EditorJsRenderer(12-370)
tests/Unit/Cms/Services/Media/CloudinaryUploaderTest.php (1)
src/Cms/Services/Media/CloudinaryUploader.php (5)
CloudinaryUploader(17-371)upload(68-89)uploadFromUrl(99-118)delete(127-140)listResources(149-184)
tests/Unit/Cms/Controllers/Admin/DashboardTest.php (3)
tests/Unit/Cms/Controllers/Member/DashboardTest.php (6)
DashboardTest(14-99)setUp(18-39)tearDown(41-54)testConstructor(56-60)testConstructorWithApplication(62-67)testIndexRendersView(73-98)tests/Unit/Cms/Controllers/HomeTest.php (2)
setUp(18-47)tearDown(49-63)tests/Unit/Cms/Controllers/PagesTest.php (2)
setUp(21-50)tearDown(52-66)
tests/Unit/Cms/Controllers/HomeTest.php (2)
src/Cms/Controllers/Home.php (1)
Home(20-65)src/Cms/Container/Container.php (1)
getInstance(251-254)
⏰ 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). (3)
- GitHub Check: build-test (postgres)
- GitHub Check: build-test (sqlite)
- GitHub Check: build-test (mysql)
🔇 Additional comments (30)
tests/Unit/Cms/Services/Category/DeleterTest.php (1)
93-118: Well-structured test for event emission.The test correctly verifies that when an event emitter is provided, a
CategoryDeletedEventis emitted upon deletion. The mock expectations properly validate the integration.Consider also asserting the return value of
delete()for completeness:- $deleter->delete( 1 ); + $result = $deleter->delete( 1 ); + $this->assertTrue( $result );tests/Unit/Cms/Controllers/Member/DashboardTest.php (4)
1-16: LGTM! Clean test structure.The imports, namespace, and class structure follow PHPUnit conventions and are consistent with other controller tests in the project.
41-54: LGTM! Proper test cleanup.The teardown correctly clears Registry entries and removes the temporary version file. The error suppression on
unlinkis appropriate here as a defensive measure.
56-60: LGTM! Basic constructor test.Simple smoke test verifying default construction works correctly.
62-67: LGTM! Dependency injection test.Correctly tests the constructor with an injected Application dependency.
tests/Unit/Cms/Services/Media/CloudinaryUploaderTest.php (2)
17-75: LGTM! Well-structured test setup.The setUp method properly configures test and integration scenarios, supporting both mocked tests with defaults and real Cloudinary integration tests via environment variables.
77-203: Excellent security test coverage!The validation tests comprehensively cover SSRF protections, including all major private/reserved IP ranges and multiple attack vectors. The flexible assertion on Line 137 for localhost DNS resolution is a good practice given system variations.
tests/Unit/Cms/Services/Widget/CalendarWidgetTest.php (7)
117-142: Excellent coverage of widget metadata methods!These tests properly verify the widget's public API for name, description, and attributes. The assertions confirm both the structure and expected values align with the implementation.
144-154: Good edge case coverage for empty event list.This test correctly verifies that the widget handles the no-events scenario gracefully by displaying a user-friendly message while maintaining proper HTML structure.
156-221: Strong category filtering test coverage!These tests thoroughly validate:
- Error handling when a category slug doesn't exist (returns appropriate HTML comment)
- Proper filtering by category ID via the repository
- Limit enforcement when combined with category filtering
The mock setup correctly simulates the category lookup and event retrieval flow.
223-248: Comprehensive HTML structure validation.This test ensures the complete rendering pipeline produces the expected HTML elements, CSS classes, URL patterns, and formatted dates. The assertions cover all critical components of the rendered output.
250-287: Excellent conditional rendering tests!These tests verify that:
- All-day events omit time information (no " at ", "AM", or "PM")
- Events without locations don't render the location span
This confirms the widget correctly adapts its output based on event properties.
289-309: Critical XSS prevention test - well done!This security-focused test verifies that malicious HTML/JavaScript in both the title and location fields is properly escaped. The assertions confirm that
<script>and<img>tags are converted to safe HTML entities (<script>,<img), preventing XSS attacks.
311-343: Thorough multi-event rendering validation.This test verifies that multiple events are all rendered correctly and uses
substr_countto confirm the exact number of list items matches the event count. Good attention to detail!tests/Unit/Cms/Services/Widget/WidgetRendererTest.php (1)
180-207: Excellent dependency validation test coverage!These three tests comprehensively validate the calendar widget's dependency requirements:
- No repositories: Confirms the error message when both repositories are missing
- Partial dependencies: Verifies that having only the event repository (without category repository) is insufficient
- Interface vs. concrete types: Tests the stricter requirement for concrete
DatabaseEventRepositoryandDatabaseEventCategoryRepositoryimplementations rather than just the interfacesThe expected error messages in the assertions correctly match the implementation logic shown in the relevant code snippets. This ensures the widget fails gracefully with clear, actionable error messages when dependencies are missing or incorrect.
tests/Unit/Cms/Controllers/Member/ProfileTest.php (3)
1-21: LGTM!The imports and class structure are well-organized and follow PHPUnit conventions.
43-57: LGTM!The tearDown method properly cleans up the Registry entries and removes the
.version.jsonfile. Using@unlinkwith error suppression is appropriate for test cleanup.
75-75: No changes needed. The Profile controller's constructor signature explicitly declares the first parameter as?Application $app = null, which permits null values. The test is correctly passing null to this parameter.tests/Unit/Cms/Controllers/PagesTest.php (6)
68-76: LGTM!Constructor test correctly verifies dependency injection.
78-123: LGTM!Comprehensive test that verifies the published page rendering flow, including view count increment and data passed to the renderer.
125-140: LGTM!Correctly verifies NotFound exception behavior for nonexistent pages.
142-160: LGTM!Correctly verifies NotFound exception behavior for unpublished pages.
162-200: LGTM!Correctly tests fallback logic when meta title is empty.
202-242: LGTM!Correctly tests fallback logic when meta description is empty.
tests/Unit/Cms/Controllers/CalendarTest.php (2)
103-146: LGTM!Excellent test with proper date range validation using callbacks. The explicit date assertions ensure correct calendar month boundaries.
189-204: No action required. The Calendar and Pages controllers intentionally throw different exception types: Calendar throwsRuntimeException(with 404 status code) for missing events/categories, while Pages throwsNotFoundfor missing/unpublished pages. The tests correctly verify their respective controller behavior.tests/Unit/Cms/Controllers/HomeTest.php (2)
65-79: LGTM!Constructor tests correctly verify instantiation with and without the optional registration service dependency.
81-139: LGTM!Comprehensive tests for index rendering with registration enabled and disabled states.
tests/Unit/Cms/Controllers/Admin/DashboardTest.php (2)
69-98: LGTM!Good test of the index rendering with proper ViewContext fluent interface mocking. The
@runInSeparateProcessannotation correctly isolates this test from global state.
56-67: LGTM!Constructor tests correctly verify instantiation with and without the Application dependency.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/Unit/Cms/Controllers/Member/ProfileTest.php (1)
35-40: This fragile path pattern was already flagged in a previous review.tests/Unit/Cms/Controllers/PagesTest.php (1)
45-49: Brittle file path assumption for version file.This uses
dirname(getcwd())which assumes tests run from a specific directory. This concern was already raised in a previous review.
🧹 Nitpick comments (22)
src/Cms/Controllers/Member/Registration.php (1)
47-55: Consider removing nullable types for required parameters.The constructor parameters are declared as nullable (
?Type) but immediately validated as required. If these dependencies are always mandatory, removing the?would make the contract clearer and allow PHP to enforce it at the type level rather than at runtime.🔎 Optional refactor to use non-nullable parameters
public function __construct( ?Application $app = null, - ?IRegistrationService $registrationService = null, - ?IEmailVerifier $emailVerifier = null, + IRegistrationService $registrationService, + IEmailVerifier $emailVerifier, ?SettingManager $settings = null, ?SessionManager $sessionManager = null, - ?ResendVerificationThrottle $resendThrottle = null, - ?IIpResolver $ipResolver = null + ResendVerificationThrottle $resendThrottle, + IIpResolver $ipResolver )Then remove the corresponding null checks in the constructor body (lines 59-81).
Note: Keep nullable parameters for
$app,$settings, and$sessionManagerif the parent class handles them or if they support container resolution.tests/Unit/Cms/Controllers/EventCategoriesControllerTest.php (3)
111-118: Redundant reflection-based SessionManager injection.Since
SessionManageris now injected via the constructor (line 86, 95), setting it again via reflection on the parent class's_sessionManagerproperty appears redundant. The constructor should already populate this property viaparent::__construct(). Consider removing this reflection code.🔎 Proposed fix
- // Mock session manager - $reflection = new \ReflectionClass( get_parent_class( EventCategories::class ) ); - $sessionProperty = $reflection->getProperty( '_sessionManager' ); - $sessionProperty->setAccessible( true ); - - $sessionManager = $this->createMock( SessionManager::class ); - $sessionManager->method( 'getFlash' )->willReturn( null ); - $sessionProperty->setValue( $controller, $sessionManager ); -
12-12: Unused import.The
Deleterclass is imported but not used anywhere in this test file. Consider removing this unused import.
165-172: Same redundant reflection-based SessionManager injection.Same issue as the
testIndexReturnsAllCategoriestest—this reflection-based injection is redundant since SessionManager is already passed via constructor.tests/Unit/Cms/Controllers/Member/DashboardTest.php (1)
32-38: Fragile version file path pattern.The
dirname(getcwd())approach for creating.version.jsonassumes tests always run from the project root. This pattern appears across multiple test files in the suite. Consider using__DIR__with a relative path to the project root, or adoptingvfsStreamfor filesystem mocking to improve test reliability across different execution contexts.tests/Unit/Cms/Controllers/UsersControllerTest.php (1)
141-152: Consider addinggetFlashmock for consistency.Unlike
testIndexReturnsAllUsers, this test doesn't configuregetFlashon theSessionManagermock. While the mocked view chain should prevent actual flash message access, adding the mock would maintain consistency across tests and prevent potential issues if test behavior changes.tests/Unit/Cms/Controllers/Member/ProfileTest.php (1)
69-80: Consider adding negative tests for missing dependencies.The AI summary mentions a
testConstructorThrowsExceptionWithoutSettingManagertest, but onlytestConstructorWithDependenciesexists. Given that other controller tests in this PR (e.g.,PagesTest,EventCategoriesControllerTest) include tests verifying exceptions when required dependencies are missing, consider adding similar negative tests here for consistency.tests/Unit/Cms/Controllers/PostsControllerTest.php (2)
95-109: Redundant SessionManager mock injection.The test creates a
mockSessionManager, passes it to the Posts constructor (lines 95-108), but then immediately overrides it via reflection (lines 124-131) with a different mock. This pattern creates confusion about which mock is actually in use and makes the constructor injection pointless.Consider either:
- Remove the reflection-based override and configure the constructor-injected mock with the necessary behavior (e.g.,
getFlash()return value), or- Document why both approaches are needed if there's a specific technical reason.
🔎 Proposed cleanup
Configure the constructor-injected mock instead of overriding:
$mockSettingManager = Registry::getInstance()->get( 'Settings' ); $mockSessionManager = $this->createMock( SessionManager::class ); +$mockSessionManager->method( 'getFlash' )->willReturn( null ); $controller = $this->getMockBuilder( Posts::class ) ->setConstructorArgs([ null, $postRepository, $categoryRepository, $tagRepository, $creator, $updater, $deleter, $mockSettingManager, $mockSessionManager ]) ->onlyMethods( ['view'] ) ->getMock(); // ... view builder setup ... $controller->method( 'view' )->willReturn( $viewBuilder ); -// Mock session manager via reflection -$reflection = new \ReflectionClass( get_parent_class( Posts::class ) ); -$sessionProperty = $reflection->getProperty( '_sessionManager' ); -$sessionProperty->setAccessible( true ); - -$sessionManager = $this->createMock( SessionManager::class ); -$sessionManager->method( 'getFlash' )->willReturn( null ); -$sessionProperty->setValue( $controller, $sessionManager ); - $request = $this->createMock( Request::class );Also applies to: 124-131
168-182: Same redundancy pattern in additional tests.The same redundant pattern (constructor injection + reflection override) appears in:
testIndexFiltersPostsByAuthorForNonAdmin(lines 168-182)testCreateReturnsFormForAuthenticatedUser(lines 231-245)testEditThrowsExceptionWhenUserUnauthorized(lines 288-301)Apply the same cleanup approach across all affected tests for consistency.
Also applies to: 231-245, 288-301
tests/Unit/Cms/Controllers/MediaUploadTest.php (2)
68-73: Redundant mock injection with reflection override.The test creates mocks for all dependencies (CloudinaryUploader, MediaValidator, SettingManager, SessionManager) and passes them to the Media constructor (lines 68-73), but then immediately overrides the validator via reflection (lines 110-119). This makes the constructor-injected
mockMediaValidatorunused.Configure the constructor-injected mocks with the necessary behavior instead of overriding them via reflection.
🔎 Proposed cleanup
$mockSettingManager = Registry::getInstance()->get( 'Settings' ); $mockSessionManager = $this->createMock( \Neuron\Cms\Auth\SessionManager::class ); $mockCloudinaryUploader = $this->createMock( CloudinaryUploader::class ); $mockMediaValidator = $this->createMock( MediaValidator::class ); +$mockMediaValidator->method( 'validate' )->willReturn( false ); +$mockMediaValidator->method( 'getFirstError' )->willReturn( 'Invalid file type' ); // Create Media controller $media = new Media( null, $mockCloudinaryUploader, $mockMediaValidator, $mockSettingManager, $mockSessionManager ); -// Create a mock validator that fails -$validatorMock = $this->createMock( MediaValidator::class ); -$validatorMock->method( 'validate' )->willReturn( false ); -$validatorMock->method( 'getFirstError' )->willReturn( 'Invalid file type' ); - -// Inject mock validator via reflection -$reflection = new \ReflectionClass( $media ); -$validatorProperty = $reflection->getProperty( '_validator' ); -$validatorProperty->setAccessible( true ); -$validatorProperty->setValue( $media, $validatorMock ); - $request = $this->createMock( Request::class );Also applies to: 102-120
140-145: Same redundancy pattern across all test methods.The redundant pattern (constructor injection + reflection override) appears in all test methods:
testUploadFeaturedImageReturnsErrorWhenNoFileUploaded(lines 140-145)testUploadFeaturedImageReturnsErrorWhenValidationFails(lines 174-180 + reflection override)testUploadImageSuccessfulUpload(lines 219-225 + overrides for validator and uploader)testUploadFeaturedImageSuccessfulUpload(lines 282-288 + overrides)testUploadImageHandlesUploadException(lines 345-351 + overrides)testUploadFeaturedImageHandlesUploadException(lines 398-404 + overrides)Apply the same cleanup across all tests: configure constructor-injected mocks instead of using reflection.
Also applies to: 174-180, 219-225, 282-288, 345-351, 398-404
tests/Unit/Cms/Controllers/CategoriesControllerTest.php (2)
84-96: Redundant SessionManager mock injection.Similar to the Posts controller tests, this test creates a
mockSessionManager, passes it to the Categories constructor (lines 84-95), but then overrides it via reflection (lines 111-117). This creates the same confusion and redundancy issue.Configure the constructor-injected mock with necessary behavior instead of using reflection.
Also applies to: 111-117
137-149: Same redundancy in additional tests.The same pattern appears in:
testCreateReturnsForm(lines 137-149, 163-169)testEditThrowsExceptionWhenCategoryNotFound(lines 191-202)Apply consistent cleanup across all test methods.
Also applies to: 163-169, 191-202
tests/Unit/Cms/Controllers/EventsControllerTest.php (2)
89-102: Redundant SessionManager mock injection.This test follows the same redundant pattern as the other controller tests: creating a
mockSessionManager, passing it to the Events constructor (lines 89-101), then overriding it via reflection (lines 117-124).For consistency with the recommended fixes in the other test files, configure the constructor-injected mock instead of using reflection.
Also applies to: 117-124
157-170: Same redundancy across all test methods.The pattern repeats in:
testIndexFiltersEventsForNonAdmin(lines 157-170, 185-192)testCreateReturnsFormForAuthenticatedUser(lines 216-229, 244-250)Consider applying the same cleanup uniformly across all controller test files in this PR.
Also applies to: 185-192, 216-229, 244-250
tests/Unit/Cms/Controllers/PagesControllerTest.php (2)
115-122: Redundant session manager injection via reflection.A
SessionManagermock is already passed to the constructor on line 99, but here a new mock is created and injected via reflection, overwriting the constructor-injected value. This redundancy is unnecessary and could mask issues with the constructor wiring.Consider removing this reflection-based injection since the dependency is now properly injected through the constructor.
🔎 Proposed fix
- // Mock session manager via reflection - $reflection = new \ReflectionClass( get_parent_class( Pages::class ) ); - $sessionProperty = $reflection->getProperty( '_sessionManager' ); - $sessionProperty->setAccessible( true ); - - $sessionManager = $this->createMock( SessionManager::class ); - $sessionManager->method( 'getFlash' )->willReturn( null ); - $sessionProperty->setValue( $controller, $sessionManager ); + // Configure the session manager mock that was passed to constructor + $mockSessionManager->method( 'getFlash' )->willReturn( null );Note: You'll also need to configure
$mockSessionManageron line 90 with thegetFlashexpectation before passing it to the constructor.
183-189: Same redundant pattern as intestIndexReturnsAllPagesForAdmin.The reflection-based session manager injection duplicates what's already passed via the constructor. Consider removing this and configuring the mock before passing it to the constructor.
src/Cms/Controllers/Content.php (1)
242-256: Dead code: unreachable lazy-load fallback.The comment states this is "for backward compatibility," but the constructor now throws
InvalidArgumentExceptionwhen$sessionManageris null (lines 90-93). This means$this->_sessionManagerwill always be set, making the conditional on lines 245-248 unreachable.Consider removing this dead code to avoid confusion:
🔎 Proposed fix
protected function getSessionManager(): SessionManager { - // Lazy-load if not injected (for backward compatibility) - if( !$this->_sessionManager ) - { - $this->_sessionManager = new SessionManager(); - } - if( !$this->_sessionManager->isStarted() ) { $this->_sessionManager->start(); } return $this->_sessionManager; }tests/Unit/Cms/Controllers/Admin/TagsTest.php (1)
63-69: LGTM with a suggestion for additional coverage.Good test for verifying the parent's
SettingManagervalidation. For completeness, consider adding tests for missingITagRepositoryandSlugGeneratorsimilar toPostsTest::testConstructorThrowsExceptionWithoutPostRepository, though this can be deferred.tests/Unit/Cms/Controllers/CalendarTest.php (1)
37-41: Same brittle file path assumption as other test files.This uses
dirname(getcwd())for the version file, which can break if tests run from different directories or in parallel. Consider extracting this setup logic to a shared test trait or base class that usessys_get_temp_dir()with unique filenames per test.🔎 Suggested approach
Create a shared test trait or base class:
trait VersionFileTestTrait { private string $_tempVersionFile; protected function setUpVersionFile(): void { $this->_tempVersionFile = sys_get_temp_dir() . '/neuron_test_' . uniqid() . '/.version.json'; mkdir(dirname($this->_tempVersionFile), 0777, true); file_put_contents($this->_tempVersionFile, json_encode([ 'major' => 1, 'minor' => 0, 'patch' => 0 ])); } protected function tearDownVersionFile(): void { @unlink($this->_tempVersionFile); @rmdir(dirname($this->_tempVersionFile)); } }tests/Unit/Cms/Controllers/Admin/DashboardTest.php (1)
16-18: Consider using typed properties consistently.
$_settingManageris typed asSettingManager, but$mockSettingManagerand$mockSessionManageruse the untyped$syntax. Since$mockSettingManageris assigned fromRegistry::getInstance()->get('Settings')which returns the sameSettingManager, consider typing it consistently.🔎 Suggested improvement
private SettingManager $_settingManager; - private $mockSettingManager; - private $mockSessionManager; + private SettingManager $mockSettingManager; + private \Neuron\Cms\Auth\SessionManager $mockSessionManager;src/Cms/Controllers/Blog.php (1)
143-143: Remove unnecessary null-safe operator.The
$_rendererproperty is now non-nullable (line 30) and the constructor throws if it's null (lines 69-71). The null-safe operator?->on line 143 is unnecessary and could mislead readers into thinking the property might be null.🔎 Suggested fix
- $renderedContent = $this->_renderer?->render( $content ) ?? (is_array($content) ? json_encode($content) : $content); + $renderedContent = $this->_renderer->render( $content );If the fallback is intentional for when
render()returnsnull, use:- $renderedContent = $this->_renderer?->render( $content ) ?? (is_array($content) ? json_encode($content) : $content); + $renderedContent = $this->_renderer->render( $content ) ?? (is_array($content) ? json_encode($content) : $content);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (49)
src/Cms/Container/Container.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/Content.phpsrc/Cms/Controllers/Home.phpsrc/Cms/Controllers/Member/Dashboard.phpsrc/Cms/Controllers/Member/Profile.phpsrc/Cms/Controllers/Member/Registration.phpsrc/Cms/Controllers/Pages.phptests/Unit/Cms/BlogControllerTest.phptests/Unit/Cms/ContentControllerTest.phptests/Unit/Cms/Controllers/Admin/CategoriesTest.phptests/Unit/Cms/Controllers/Admin/DashboardTest.phptests/Unit/Cms/Controllers/Admin/EventCategoriesTest.phptests/Unit/Cms/Controllers/Admin/EventsTest.phptests/Unit/Cms/Controllers/Admin/PagesTest.phptests/Unit/Cms/Controllers/Admin/PostsTest.phptests/Unit/Cms/Controllers/Admin/ProfileTest.phptests/Unit/Cms/Controllers/Admin/TagsTest.phptests/Unit/Cms/Controllers/Admin/UsersTest.phptests/Unit/Cms/Controllers/Auth/LoginTest.phptests/Unit/Cms/Controllers/Auth/PasswordResetTest.phptests/Unit/Cms/Controllers/CalendarTest.phptests/Unit/Cms/Controllers/CategoriesControllerTest.phptests/Unit/Cms/Controllers/EventCategoriesControllerTest.phptests/Unit/Cms/Controllers/EventsControllerTest.phptests/Unit/Cms/Controllers/HomeTest.phptests/Unit/Cms/Controllers/MediaIndexTest.phptests/Unit/Cms/Controllers/MediaUploadTest.phptests/Unit/Cms/Controllers/Member/DashboardTest.phptests/Unit/Cms/Controllers/Member/ProfileTest.phptests/Unit/Cms/Controllers/Member/RegistrationTest.phptests/Unit/Cms/Controllers/PagesControllerTest.phptests/Unit/Cms/Controllers/PagesTest.phptests/Unit/Cms/Controllers/PostsControllerTest.phptests/Unit/Cms/Controllers/TagsControllerTest.phptests/Unit/Cms/Controllers/UsersControllerTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/Unit/Cms/Controllers/Admin/ProfileTest.php
- tests/Unit/Cms/Controllers/HomeTest.php
🧰 Additional context used
🧬 Code graph analysis (27)
tests/Unit/Cms/Controllers/PagesControllerTest.php (2)
src/Cms/Container/Container.php (1)
getInstance(268-271)src/Cms/Auth/SessionManager.php (1)
SessionManager(13-216)
src/Cms/Controllers/Admin/Dashboard.php (2)
src/Cms/Controllers/Content.php (2)
Content(62-401)__construct(77-119)src/Cms/Controllers/Member/Dashboard.php (1)
__construct(32-39)
tests/Unit/Cms/Controllers/EventCategoriesControllerTest.php (1)
src/Cms/Auth/SessionManager.php (1)
SessionManager(13-216)
src/Cms/Controllers/Member/Dashboard.php (2)
src/Cms/Controllers/Content.php (2)
Content(62-401)__construct(77-119)src/Cms/Controllers/Admin/Dashboard.php (1)
__construct(30-37)
src/Cms/Controllers/Calendar.php (7)
src/Cms/Controllers/Admin/EventCategories.php (1)
__construct(43-71)src/Cms/Controllers/Admin/Events.php (1)
__construct(47-82)src/Cms/Controllers/Admin/Pages.php (1)
__construct(46-74)src/Cms/Controllers/Admin/Users.php (1)
__construct(44-79)src/Cms/Controllers/Blog.php (1)
__construct(43-78)src/Cms/Controllers/Home.php (1)
__construct(33-49)src/Cms/Controllers/Pages.php (1)
__construct(41-64)
src/Cms/Container/Container.php (4)
src/Cms/Services/Content/EditorJsRenderer.php (1)
EditorJsRenderer(12-370)src/Cms/Services/Media/CloudinaryUploader.php (1)
CloudinaryUploader(17-371)src/Cms/Services/Media/MediaValidator.php (1)
MediaValidator(15-193)src/Cms/Auth/ResendVerificationThrottle.php (1)
ResendVerificationThrottle(19-195)
tests/Unit/Cms/Controllers/PostsControllerTest.php (2)
src/Cms/Container/Container.php (1)
getInstance(268-271)src/Cms/Auth/SessionManager.php (1)
SessionManager(13-216)
tests/Unit/Cms/ContentControllerTest.php (2)
src/Cms/Auth/SessionManager.php (1)
SessionManager(13-216)src/Cms/Controllers/Content.php (1)
Content(62-401)
tests/Unit/Cms/Controllers/Member/RegistrationTest.php (1)
src/Cms/Controllers/Member/Registration.php (1)
Registration(29-268)
src/Cms/Controllers/Admin/Media.php (3)
src/Cms/Controllers/Content.php (1)
Content(62-401)src/Cms/Controllers/Admin/Dashboard.php (1)
__construct(30-37)src/Cms/Controllers/Member/Dashboard.php (1)
__construct(32-39)
tests/Unit/Cms/Controllers/Admin/EventsTest.php (9)
src/Cms/Container/Container.php (1)
getInstance(268-271)src/Cms/Auth/SessionManager.php (1)
SessionManager(13-216)tests/Unit/Cms/Controllers/Admin/CategoriesTest.php (1)
testConstructorThrowsExceptionWithoutSettingManager(74-80)tests/Unit/Cms/Controllers/Admin/EventCategoriesTest.php (1)
testConstructorThrowsExceptionWithoutSettingManager(66-72)tests/Unit/Cms/Controllers/Admin/PagesTest.php (1)
testConstructorThrowsExceptionWithoutSettingManager(66-72)tests/Unit/Cms/Controllers/Admin/PostsTest.php (1)
testConstructorThrowsExceptionWithoutSettingManager(76-82)tests/Unit/Cms/Controllers/Admin/ProfileTest.php (1)
testConstructorThrowsExceptionWithoutSettingManager(89-95)tests/Unit/Cms/Controllers/Admin/TagsTest.php (1)
testConstructorThrowsExceptionWithoutSettingManager(63-69)tests/Unit/Cms/Controllers/Admin/UsersTest.php (1)
testConstructorThrowsExceptionWithoutSettingManager(69-75)
tests/Unit/Cms/Controllers/Admin/CategoriesTest.php (9)
src/Cms/Container/Container.php (1)
getInstance(268-271)src/Cms/Auth/SessionManager.php (1)
SessionManager(13-216)tests/Unit/Cms/Controllers/Admin/EventCategoriesTest.php (1)
testConstructorThrowsExceptionWithoutSettingManager(66-72)tests/Unit/Cms/Controllers/Admin/EventsTest.php (1)
testConstructorThrowsExceptionWithoutSettingManager(69-75)tests/Unit/Cms/Controllers/Admin/PagesTest.php (1)
testConstructorThrowsExceptionWithoutSettingManager(66-72)tests/Unit/Cms/Controllers/Admin/PostsTest.php (1)
testConstructorThrowsExceptionWithoutSettingManager(76-82)tests/Unit/Cms/Controllers/Admin/ProfileTest.php (1)
testConstructorThrowsExceptionWithoutSettingManager(89-95)tests/Unit/Cms/Controllers/Admin/TagsTest.php (1)
testConstructorThrowsExceptionWithoutSettingManager(63-69)tests/Unit/Cms/Controllers/Admin/UsersTest.php (1)
testConstructorThrowsExceptionWithoutSettingManager(69-75)
tests/Unit/Cms/Controllers/Admin/UsersTest.php (9)
src/Cms/Container/Container.php (1)
getInstance(268-271)src/Cms/Auth/SessionManager.php (1)
SessionManager(13-216)tests/Unit/Cms/Controllers/Admin/CategoriesTest.php (1)
testConstructorThrowsExceptionWithoutSettingManager(74-80)tests/Unit/Cms/Controllers/Admin/EventCategoriesTest.php (1)
testConstructorThrowsExceptionWithoutSettingManager(66-72)tests/Unit/Cms/Controllers/Admin/EventsTest.php (1)
testConstructorThrowsExceptionWithoutSettingManager(69-75)tests/Unit/Cms/Controllers/Admin/PagesTest.php (1)
testConstructorThrowsExceptionWithoutSettingManager(66-72)tests/Unit/Cms/Controllers/Admin/PostsTest.php (1)
testConstructorThrowsExceptionWithoutSettingManager(76-82)tests/Unit/Cms/Controllers/Admin/ProfileTest.php (1)
testConstructorThrowsExceptionWithoutSettingManager(89-95)tests/Unit/Cms/Controllers/Admin/TagsTest.php (1)
testConstructorThrowsExceptionWithoutSettingManager(63-69)
tests/Unit/Cms/Controllers/Admin/DashboardTest.php (2)
tests/Unit/Cms/BlogControllerTest.php (1)
setUp(32-102)tests/Unit/Cms/Controllers/HomeTest.php (1)
setUp(18-47)
tests/Unit/Cms/Controllers/UsersControllerTest.php (2)
src/Cms/Container/Container.php (1)
getInstance(268-271)src/Cms/Auth/SessionManager.php (1)
SessionManager(13-216)
tests/Unit/Cms/Controllers/TagsControllerTest.php (3)
src/Cms/Container/Container.php (1)
getInstance(268-271)src/Cms/Auth/SessionManager.php (1)
SessionManager(13-216)src/Cms/Services/SlugGenerator.php (1)
SlugGenerator(20-113)
src/Cms/Controllers/Admin/Posts.php (4)
src/Cms/Controllers/Admin/Dashboard.php (1)
__construct(30-37)src/Cms/Controllers/Admin/Profile.php (1)
__construct(43-71)src/Cms/Controllers/Admin/Tags.php (1)
__construct(40-61)src/Cms/Controllers/Content.php (1)
__construct(77-119)
src/Cms/Controllers/Member/Profile.php (7)
src/Cms/Controllers/Content.php (2)
Content(62-401)__construct(77-119)src/Cms/Controllers/Admin/Pages.php (1)
__construct(46-74)src/Cms/Controllers/Admin/Profile.php (1)
__construct(43-71)src/Cms/Controllers/Auth/Login.php (1)
__construct(38-52)src/Cms/Controllers/Auth/PasswordReset.php (1)
__construct(41-55)src/Cms/Controllers/Home.php (1)
__construct(33-49)src/Cms/Controllers/Member/Dashboard.php (1)
__construct(32-39)
src/Cms/Controllers/Admin/Categories.php (5)
src/Cms/Controllers/Content.php (2)
Content(62-401)__construct(77-119)src/Cms/Controllers/Admin/Events.php (1)
__construct(47-82)src/Cms/Controllers/Admin/Pages.php (1)
__construct(46-74)src/Cms/Controllers/Admin/Profile.php (1)
__construct(43-71)src/Cms/Controllers/Admin/Tags.php (1)
__construct(40-61)
src/Cms/Controllers/Content.php (1)
src/Cms/Auth/SessionManager.php (2)
SessionManager(13-216)get(134-138)
src/Cms/Controllers/Home.php (4)
src/Cms/Controllers/Admin/Dashboard.php (1)
__construct(30-37)src/Cms/Controllers/Content.php (1)
__construct(77-119)src/Cms/Controllers/Member/Dashboard.php (1)
__construct(32-39)src/Cms/Services/Member/RegistrationService.php (1)
isRegistrationEnabled(61-64)
tests/Unit/Cms/Controllers/Auth/PasswordResetTest.php (2)
src/Cms/Controllers/Auth/PasswordReset.php (1)
PasswordReset(28-210)src/Cms/Auth/SessionManager.php (1)
SessionManager(13-216)
tests/Unit/Cms/Controllers/CategoriesControllerTest.php (2)
src/Cms/Container/Container.php (1)
getInstance(268-271)src/Cms/Auth/SessionManager.php (1)
SessionManager(13-216)
src/Cms/Controllers/Blog.php (5)
src/Cms/Auth/SessionManager.php (1)
SessionManager(13-216)src/Cms/Controllers/Admin/Posts.php (1)
__construct(54-103)src/Cms/Controllers/Auth/Login.php (1)
__construct(38-52)src/Cms/Controllers/Content.php (1)
__construct(77-119)src/Cms/Controllers/Home.php (1)
__construct(33-49)
src/Cms/Controllers/Admin/Tags.php (5)
src/Cms/Controllers/Content.php (1)
Content(62-401)src/Cms/Controllers/Admin/Dashboard.php (1)
__construct(30-37)src/Cms/Controllers/Admin/Media.php (1)
__construct(42-63)src/Cms/Controllers/Auth/Login.php (1)
__construct(38-52)src/Cms/Controllers/Member/Dashboard.php (1)
__construct(32-39)
tests/Unit/Cms/Controllers/Member/DashboardTest.php (2)
tests/Unit/Cms/Controllers/Admin/DashboardTest.php (6)
DashboardTest(14-105)setUp(20-44)tearDown(46-59)testConstructor(61-65)testConstructorWithApplication(67-72)testIndexRendersView(78-104)tests/Unit/Cms/Controllers/HomeTest.php (2)
setUp(18-47)tearDown(49-63)
tests/Unit/Cms/Controllers/Auth/LoginTest.php (2)
src/Cms/Controllers/Auth/Login.php (1)
Login(25-184)src/Cms/Auth/SessionManager.php (1)
SessionManager(13-216)
⏰ 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). (3)
- GitHub Check: build-test (postgres)
- GitHub Check: build-test (mysql)
- GitHub Check: build-test (sqlite)
🔇 Additional comments (74)
src/Cms/Controllers/Member/Registration.php (1)
59-81: LGTM! Explicit dependency injection enforcement improves testability.The fail-fast validation for all required dependencies is a solid improvement over container-based resolution. The exception messages are clear and consistent, making debugging straightforward.
tests/Unit/Cms/Controllers/Member/RegistrationTest.php (2)
127-141: LGTM! Comprehensive test coverage for explicit DI validation.The renamed test accurately reflects the new behavior, and the exception message validation ensures clear error reporting.
143-189: Excellent test coverage for all required dependencies.The three new tests comprehensively validate that each required dependency throws the expected exception when null. The tests are well-structured and follow a consistent pattern.
src/Cms/Controllers/Auth/PasswordReset.php (2)
5-5: LGTM: Use statements added correctly.The new imports for
SessionManagerandSettingManagersupport the updated constructor signature.Also applies to: 11-11
41-55: Explicit dependency validation enforced correctly.The constructor now validates that
IPasswordResetteris provided, throwingInvalidArgumentExceptionif null. This aligns with the PR's goal of removing container-based fallbacks.Note: All parameters are nullable in the signature, but
$passwordResetteris effectively required at runtime. This pattern may support DI container compatibility but creates a slight API inconsistency where the type hint suggests optional but the implementation enforces required.tests/Unit/Cms/Controllers/Auth/PasswordResetTest.php (3)
93-93: LGTM: Controller instantiation updated correctly.The
setUp()method properly instantiates the controller with all four dependencies, matching the new constructor signature.
103-110: LGTM: Constructor positive test added.This test validates that the constructor works correctly when all dependencies are provided. Good coverage of the happy path.
112-121: LGTM: Exception validation test added correctly.This test properly validates that
InvalidArgumentExceptionis thrown whenIPasswordResetteris not provided. The expected exception message matches the production code exactly.src/Cms/Container/Container.php (1)
239-249: LGTM on the new service bindings.The new autowired bindings for
EditorJsRenderer,CloudinaryUploader,MediaValidator,ResendVerificationThrottle, andIIpResolverare correctly structured. The autowire feature will handle constructor dependency resolution for services that depend onSettingManageror other already-registered services.src/Cms/Controllers/Admin/Profile.php (1)
43-71: LGTM on the explicit dependency injection pattern.The constructor correctly enforces required dependencies (
IUserRepository,PasswordHasher,IUserUpdater) with clear exception messages. TheSettingManagerandSessionManagerare passed to the parent constructor which handles their validation.tests/Unit/Cms/Controllers/Member/DashboardTest.php (1)
56-107: LGTM on the test methods.The constructor tests properly verify instantiation with and without an Application mock. The
testIndexRendersViewmethod correctly uses@runInSeparateProcessto avoid session conflicts and properly mocks the fluent ViewContext chain.tests/Unit/Cms/Controllers/UsersControllerTest.php (1)
90-103: LGTM on the updated constructor arguments.The test correctly injects
SettingManagerandSessionManageras the new constructor dependencies. ThegetFlashmock returningnullensures the controller's view rendering works without flash messages.tests/Unit/Cms/Controllers/Admin/PagesTest.php (1)
49-72: LGTM on the updated constructor tests.The
testConstructorWithAllDependenciescorrectly passes the newSettingManagerandSessionManagerdependencies. ThetestConstructorThrowsExceptionWithoutSettingManagerproperly verifies that anInvalidArgumentExceptionis thrown with the expected message when the dependency is missing. This pattern aligns well with the PR's DI migration approach.tests/Unit/Cms/ContentControllerTest.php (2)
17-17: LGTM: Clean mock setup for SessionManager.The addition of the
mockSessionManagerproperty and its initialization insetUp()properly supports the updated constructor signature for theContentcontroller.Also applies to: 36-37
83-83: LGTM: Constructor calls consistently updated.All
Contentcontroller instantiations have been correctly updated to pass the required three arguments (null, SettingManager, SessionManager), matching the new constructor signature.Also applies to: 103-103, 131-131, 159-159, 186-186, 209-209
tests/Unit/Cms/Controllers/MediaIndexTest.php (2)
66-80: LGTM! Clean migration to constructor-based DI.The test properly constructs all required dependencies (SettingManager, SessionManager, CloudinaryUploader, MediaValidator) and passes them via
setConstructorArgs. The removal of reflection-based injection in favor of explicit constructor injection improves test clarity and aligns with modern DI best practices.
135-145: Good test coverage for cursor pagination.The test properly validates that the cursor parameter is passed through to
listResourceswith the expected options. Using$this->callback()to verify multiple conditions in the options array is a clean approach.tests/Unit/Cms/Controllers/TagsControllerTest.php (1)
79-90: LGTM! Consistent DI pattern applied.The test correctly creates mocks for all required dependencies (SlugGenerator, SettingManager, SessionManager) and passes them through the constructor. This aligns with the PR-wide shift to explicit dependency injection.
tests/Unit/Cms/Controllers/Auth/LoginTest.php (2)
62-69: LGTM! Good test coverage for successful construction.The test validates that the Login controller can be constructed with all required dependencies.
71-80: LGTM! Good negative test for missing dependency.The test properly validates that constructing Login without an IAuthenticationService throws InvalidArgumentException with the correct message. This enforces explicit dependency injection at the constructor level.
src/Cms/Controllers/Calendar.php (1)
37-58: LGTM! Consistent DI enforcement pattern applied.The constructor properly enforces explicit dependency injection for IEventRepository and IEventCategoryRepository, throwing InvalidArgumentException with clear messages when dependencies are missing. The signature and validation pattern align with other controllers in the codebase (e.g., Admin/Events, Admin/Pages, Blog).
tests/Unit/Cms/Controllers/Admin/EventsTest.php (2)
51-67: LGTM! Comprehensive dependency injection test.The test correctly constructs the Events controller with all required dependencies (Application, IEventRepository, IEventCategoryRepository, IEventCreator, IEventUpdater, SettingManager, SessionManager). This validates that the constructor accepts all expected dependencies.
69-75: LGTM! Good negative test for missing dependency.The test validates that constructing Events without SettingManager throws InvalidArgumentException. This enforces explicit DI at the constructor level, consistent with other admin controller tests (Tags, Pages, Posts, Users, etc.).
src/Cms/Controllers/Member/Dashboard.php (2)
5-9: LGTM!The new imports for
SessionManagerandSettingManagerare properly added to support the explicit dependency injection pattern.
32-39: LGTM!The constructor correctly accepts the new
SettingManagerandSessionManagerdependencies and forwards them to the parentContentconstructor. This aligns with the consistent DI pattern used across other controllers in the codebase (e.g.,Admin\Dashboard,Auth\Login).Minor note: The docblock
@return voidannotation on line 29 is unconventional for constructors but doesn't affect functionality.tests/Unit/Cms/Controllers/PagesControllerTest.php (3)
89-100: LGTM!The test correctly retrieves the mock
SettingManagerfrom the Registry and creates a mockSessionManager, passing both to thePagescontroller constructor. This aligns with the new explicit DI pattern.
156-167: LGTM!Test properly updated to pass the new dependencies to the
Pagescontroller constructor.
208-220: LGTM!This test correctly uses constructor injection without redundant reflection-based overwriting, serving as a good reference for refactoring the other tests.
src/Cms/Controllers/Content.php (2)
69-70: LGTM!Good change from nullable to non-nullable types for the properties, enforcing the explicit DI contract.
85-96: LGTM!The explicit null checks with descriptive exception messages provide clear error feedback when dependencies are missing. This enforces a fail-fast approach aligned with the pure DI pattern.
src/Cms/Controllers/Admin/Dashboard.php (2)
5-8: LGTM!Proper imports added for the new dependencies.
30-37: LGTM!The constructor correctly accepts and forwards
SettingManagerandSessionManagerto the parentContentconstructor, consistent with the DI pattern used across the codebase.tests/Unit/Cms/Controllers/Admin/PostsTest.php (3)
56-74: LGTM!The test correctly verifies that the
Postscontroller can be constructed when all dependencies are provided, including the newSettingManagerandSessionManagerdependencies.
76-82: LGTM!Good negative test verifying that constructing
Postswithout aSettingManagerthrows the expectedInvalidArgumentException. This validates the fail-fast behavior from the parentContentconstructor.
84-103: LGTM!Good test coverage for verifying that
Poststhrows whenIPostRepositoryis missing, even whenSettingManagerandSessionManagerare properly provided. This ensures the controller's own dependency validation works correctly.src/Cms/Controllers/Admin/Tags.php (2)
5-11: LGTM!Proper imports added for the new
SessionManagerandSettingManagerdependencies.
40-61: LGTM!The constructor properly:
- Accepts the new
SettingManagerandSessionManagerdependencies- Forwards them to the parent
Contentconstructor (which validates them)- Validates its own domain-specific dependencies (
ITagRepository,SlugGenerator) with clear exception messagesThis pattern is consistent with other controllers like
Auth/LoginandAdmin/Media.tests/Unit/Cms/Controllers/Admin/TagsTest.php (1)
47-61: LGTM!The test correctly validates that
Tagscan be constructed with all required dependencies, including the newSettingManagerandSessionManager.tests/Unit/Cms/Controllers/Admin/EventCategoriesTest.php (2)
49-64: LGTM!The test correctly verifies that
EventCategoriescan be instantiated when all dependencies are explicitly provided, aligning with the new strict DI pattern.
66-72: LGTM!The test properly validates that the constructor throws
InvalidArgumentExceptionwhenSettingManageris not injected, consistent with the pattern used in other controller tests (TagsTest, PagesTest, PostsTest, etc. as shown in the relevant code snippets).src/Cms/Controllers/Pages.php (1)
41-64: LGTM!The constructor properly enforces explicit dependency injection for
IPageRepositoryandEditorJsRendererwhile forwardingSettingManagerandSessionManagerto the parent. The fail-fast approach with descriptive exception messages aligns with the broader DI pattern established across the codebase.src/Cms/Controllers/Admin/Events.php (1)
47-82: LGTM!The constructor enforces explicit DI for all four required dependencies with clear, specific error messages. This aligns with the pattern established in other Admin controllers (Profile, Pages, Tags) as shown in the relevant code snippets.
src/Cms/Controllers/Home.php (1)
33-49: LGTM!The constructor properly enforces explicit injection of
IRegistrationServiceand forwardsSettingManagerandSessionManagerto the parentContentconstructor. The property type change to non-nullable on line 24 correctly reflects the fail-fast DI pattern.src/Cms/Controllers/Auth/Login.php (1)
38-52: LGTM!The constructor correctly enforces explicit injection of
IAuthenticationServiceand properly forwardsSettingManagerandSessionManagerto the parent. This aligns with the established DI pattern across the codebase.src/Cms/Controllers/Admin/Categories.php (1)
42-70: LGTM!The constructor properly enforces explicit DI for all three required dependencies (
ICategoryRepository,ICategoryCreator,ICategoryUpdater) with clear error messages. The implementation is consistent with the pattern used in other Admin controllers.tests/Unit/Cms/Controllers/Admin/CategoriesTest.php (3)
57-72: LGTM!The test correctly verifies successful construction with all dependencies explicitly provided, matching the updated
Categoriesconstructor signature.
74-80: LGTM!The test properly validates that missing
SettingManagerthrowsInvalidArgumentException, consistent with the pattern used in other admin controller tests (TagsTest, PagesTest, PostsTest, UsersTest, EventsTest, ProfileTest) as shown in the relevant code snippets.
82-98: LGTM!Good test coverage for the
ICategoryRepositorynull check. The test correctly provides the base dependencies (SettingManagerandSessionManager) to ensure the parent constructor succeeds, then verifies that the repository-specific validation throws the expected exception.src/Cms/Controllers/Admin/Pages.php (1)
46-74: LGTM! Constructor enforces explicit dependency injection.The constructor properly validates all required dependencies and throws
InvalidArgumentExceptionwhen any are missing. This aligns with the project-wide shift from container-based resolution to explicit DI, as seen in the parentContentcontroller.tests/Unit/Cms/Controllers/PagesTest.php (2)
68-128: Good test coverage for constructor and show method.Tests properly verify constructor dependency wiring, published page rendering, and that
incrementViewCountis called. The callback assertions forrenderHtmleffectively validate the data structure passed to the view.
130-169: Appropriate exception testing for edge cases.Tests correctly verify that
NotFoundexceptions are thrown for both nonexistent and unpublished pages, matching the expected controller behavior.src/Cms/Controllers/Admin/EventCategories.php (1)
43-71: LGTM! Consistent explicit DI pattern.The constructor follows the same pattern as other admin controllers, properly validating and assigning all required dependencies.
tests/Unit/Cms/Controllers/Admin/UsersTest.php (2)
51-67: LGTM! Constructor test properly validates all dependencies.The test correctly provides all required dependencies including the newly added
SettingManagerandSessionManager.
69-94: Good negative tests for missing dependencies.Tests properly verify that
InvalidArgumentExceptionis thrown with appropriate messages whenSettingManagerorIUserRepositoryare missing. This pattern is consistent with other controller tests in the PR.src/Cms/Controllers/Admin/Posts.php (1)
54-103: LGTM! Comprehensive dependency validation.The constructor properly validates all six domain dependencies in addition to the settings and session manager. The pattern is consistent with other admin controllers, though Posts has the most dependencies to validate.
tests/Unit/Cms/BlogControllerTest.php (1)
261-278: Clean migration from reflection to constructor-based DI.The helper method now properly constructs the Blog controller with all dependencies injected directly. The comment explaining why
Model::setPdo()must be restored after Blog construction is helpful for maintainability.tests/Unit/Cms/Controllers/CalendarTest.php (4)
58-68: LGTM! Constructor test validates dependency wiring.The test properly verifies that the Calendar controller can be constructed with all required dependencies.
70-154: Comprehensive calendar rendering tests.Good coverage of both current month and specific month/year scenarios. The callback assertions effectively validate that the correct data structure is passed to
renderHtml.
200-241: Appropriate exception testing for events.Tests correctly verify that
RuntimeExceptionis thrown for both nonexistent and unpublished events.
304-341: Good fallback behavior test.Verifies that when an event's description is null, the event title is used as the description fallback.
src/Cms/Controllers/Admin/Media.php (3)
5-10: LGTM!The new imports for
SessionManagerandSettingManagerare correctly added to support the explicit DI pattern.
29-30: LGTM!Converting properties from nullable to non-nullable correctly reflects the strict DI requirement enforced in the constructor.
42-63: LGTM!The constructor properly:
- Forwards
$settingsand$sessionManagerto the parent (which validates them)- Validates that
CloudinaryUploaderandMediaValidatorare injected- Assigns dependencies to non-nullable properties
This is consistent with the DI pattern used across other controllers like
Content.phpandAdmin/Dashboard.php.src/Cms/Controllers/Admin/Users.php (2)
5-12: LGTM!Import statements correctly added for
SessionManagerandSettingManager.
44-79: LGTM!The constructor correctly:
- Forwards settings and session manager to the parent
- Validates all four service dependencies with clear error messages
- Assigns to non-nullable private properties
This is consistent with the DI pattern used in other admin controllers like
Admin/Pages.phpandAdmin/Posts.php.src/Cms/Controllers/Member/Profile.php (2)
5-10: LGTM!Import statements for
SessionManagerandSettingManagercorrectly added.
39-67: LGTM!The constructor implementation is consistent with
Admin/Profile.php(as shown in relevant snippets at lines 42-70). Both controllers:
- Accept the same dependencies in the same order
- Forward settings/session to parent
- Validate and assign repository, hasher, and userUpdater identically
This maintains a clean, predictable pattern across the codebase.
tests/Unit/Cms/Controllers/Admin/DashboardTest.php (4)
20-44: LGTM!The setup correctly:
- Creates in-memory settings with required site configuration
- Registers settings in the Registry (consistent with other tests like
HomeTest.phpandBlogControllerTest.php)- Creates the version file for Content controller initialization
- Creates mock SessionManager for injection
This follows the established test patterns shown in the relevant snippets.
46-59: LGTM!Proper cleanup of Registry entries and version file. Using
@unlinkto suppress errors if the file doesn't exist is appropriate for teardown.
61-72: LGTM!Constructor tests correctly pass the new required dependencies (
SettingManagerandSessionManager) to theDashboardconstructor.
74-104: LGTM!The view rendering test properly:
- Uses
@runInSeparateProcessto avoid state pollution- Mocks the
view()method to isolate the test from actual view rendering- Creates a fluent mock for
ViewContextthat returns itself for chained calls- Verifies the expected output from
index()This is a good pattern for testing controller rendering without full integration.
src/Cms/Controllers/Blog.php (3)
5-15: LGTM!Import statements for
SessionManagerandSettingManagercorrectly added.
26-30: LGTM!Properties correctly changed to non-nullable, reflecting the strict DI requirements enforced in the constructor.
43-78: LGTM!The constructor properly validates and assigns all dependencies. The pattern is consistent with
Admin/Posts.php(shown in relevant snippets at lines 53-102).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (12)
tests/Unit/Cms/Controllers/Admin/DashboardTest.php (1)
16-19: Consider removing redundant property.Both
$_settingManager(line 16) and$mockSettingManager(line 18) hold references to the same SettingManager instance (see line 41 in setUp). The tests could use$this->_settingManagerdirectly instead of$this->mockSettingManager, reducing redundancy.🔎 Suggested simplification
Remove line 18 and update test methods to use
$this->_settingManagerinstead of$this->mockSettingManager:private SettingManager $_settingManager; private string $_versionFilePath; - private $mockSettingManager; private $mockSessionManager;And in the test methods (lines 65, 72, 84):
- $controller = new Dashboard( null, $this->mockSettingManager, $this->mockSessionManager ); + $controller = new Dashboard( null, $this->_settingManager, $this->mockSessionManager );tests/Unit/Cms/Controllers/HomeTest.php (6)
47-63: Consider a more robust Registry cleanup approach.The tearDown method manually clears specific Registry keys. This approach is brittle—if new Registry entries are added in future tests or controller changes, they won't be automatically cleaned up, potentially causing test pollution.
Consider either:
- Storing the original Registry state in setUp and restoring it in tearDown
- Using a test-specific Registry instance if the framework supports it
- Documenting which keys the test uses so maintainers know to update tearDown
Also, the
isset()check on line 57 is unnecessary since$this->_versionFilePathis always initialized in setUp.🔎 Proposed simplification for version file cleanup
// Clean up temp version file - if( isset( $this->_versionFilePath ) && file_exists( $this->_versionFilePath ) ) + if( file_exists( $this->_versionFilePath ) ) { unlink( $this->_versionFilePath ); }
85-149: Reduce code duplication by extracting a helper method.The three index test methods share significant setup code: creating the same mocks, building a partial mock of the Home controller, and setting up renderHtml expectations. This duplication makes maintenance harder and increases the risk of inconsistencies.
Consider extracting a helper method like
createHomeControllerMock(?bool $registrationEnabled)to centralize the common setup logic.🔎 Example helper method to reduce duplication
private function createHomeControllerMock( ?bool $registrationEnabled ): Home { $mockRegistrationService = $this->createMock( IRegistrationService::class ); if( $registrationEnabled !== null ) { $mockRegistrationService->method( 'isRegistrationEnabled' ) ->willReturn( $registrationEnabled ); } $mockSessionManager = $this->createMock( \Neuron\Cms\Auth\SessionManager::class ); return $this->getMockBuilder( Home::class ) ->setConstructorArgs( [ null, $mockRegistrationService, $this->_settingManager, $mockSessionManager ] ) ->onlyMethods( [ 'renderHtml' ] ) ->getMock(); }Then each test can simply call
$controller = $this->createHomeControllerMock( true )or$controller = $this->createHomeControllerMock( false ).
99-112: Consider verifying the HTTP status code parameter.All three index tests use
$this->anything()for the first parameter torenderHtml, which means they don't verify the HTTP status code. According to the Home controller code (line 43 in src/Cms/Controllers/Home.php), the status should beHttpResponseStatus::OK.Verifying the correct status code would make these tests more thorough.
🔎 Example change to verify HTTP status
$controller->expects( $this->once() ) ->method( 'renderHtml' ) ->with( - $this->anything(), + \Neuron\Mvc\HttpResponseStatus::OK, $this->callback( function( $data ) { // ... } ),Also applies to: 133-143, 165-179
151-151: Remove extra blank line.Line 151 contains an unnecessary blank line between test methods.
152-183: Reconsider the necessity of this test method.
testIndexPassesCorrectDataToViewappears redundant withtestIndexWithRegistrationEnabled. Both tests:
- Mock registration as enabled (line 155 vs line 88)
- Verify the same data keys exist in the view data (lines 171-175 vs lines 104-108)
- Use identical callback assertions to check data structure
The only difference is that line 182 doesn't assert the return value, whereas line 117 does—but that doesn't justify a separate test.
Consider either removing this test or differentiating it by testing a distinct scenario (e.g., verifying actual values of Title/Name/Description rather than just key existence).
85-183: Consider additional test scenarios for more comprehensive coverage.The current tests cover the happy path well, but consider adding tests for:
- Exception handling if
isRegistrationEnabled()throws an exception- Verification that Title, Name, and Description contain the expected values from settings (currently only checking key existence)
- Edge cases like empty or null setting values
These additions would make the test suite more robust, though they're not critical if the current coverage meets project standards.
src/Cms/Controllers/Content.php (1)
243-257: Remove unreachable backward-compatibility code.The fallback logic on lines 246-249 that creates a new
SessionManagerif not injected is unreachable. The constructor (lines 90-93) now throws anInvalidArgumentExceptionif$sessionManageris null, guaranteeing$this->_sessionManageris always set.🔎 Proposed fix to remove unreachable code
protected function getSessionManager(): SessionManager { - // Lazy-load if not injected (for backward compatibility) - if( !$this->_sessionManager ) - { - $this->_sessionManager = new SessionManager(); - } - if( !$this->_sessionManager->isStarted() ) { $this->_sessionManager->start(); } return $this->_sessionManager; }tests/Unit/Cms/ContentControllerTest.php (1)
18-18: Add type hint for consistency.The
$mockSessionManagerproperty lacks a type hint, while$_settingManager(line 16) is properly typed. Add a type hint for consistency and better IDE support.🔎 Proposed fix
- private $mockSessionManager; + private \Neuron\Cms\Auth\SessionManager $mockSessionManager;tests/Unit/Cms/Controllers/PagesTest.php (3)
103-106: Consider the trade-offs of partial mocking.Several tests use
onlyMethods()to partially mock the controller, replacingrenderHtml()andgetDescription()with stubs. While this isolates theshow()method logic, it bypasses the actual rendering implementation and could mask issues in those methods.Consider whether full integration tests (without partial mocking) would provide better coverage, or document why the partial approach is preferred here.
Also applies to: 192-195, 235-238
203-203: Consider stricter validation for title fallback.Using
str_contains($data['Title'], 'Test Page')is loose—it passes if 'Test Page' appears anywhere in the Title string. Consider using a more precise assertion like$data['Title'] === 'Test Page | Test Title'(or whatever the expected format is) to ensure the fallback logic constructs the title correctly.
130-147: Add negative assertion for incrementViewCount in failure cases.Tests for nonexistent and unpublished pages throw
NotFoundexceptions but don't verify thatincrementViewCount()is not called. Add$mockPageRepository->expects($this->never())->method('incrementViewCount')to ensure view counts don't increment for failed page lookups.🔎 Proposed fix
For
testShowThrowsNotFoundForNonexistentPage:$mockPageRepository = $this->createMock( IPageRepository::class ); $mockPageRepository->method( 'findBySlug' )->with( 'nonexistent' )->willReturn( null ); +$mockPageRepository->expects( $this->never() )->method( 'incrementViewCount' );For
testShowThrowsNotFoundForUnpublishedPage:$mockPageRepository = $this->createMock( IPageRepository::class ); $mockPageRepository->method( 'findBySlug' )->with( 'unpublished' )->willReturn( $mockPage ); +$mockPageRepository->expects( $this->never() )->method( 'incrementViewCount' );Also applies to: 149-169
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/Cms/Container/CmsServiceProvider.phpsrc/Cms/Container/Container.phpsrc/Cms/Controllers/Content.phpsrc/Cms/Controllers/Member/Registration.phpsrc/Cms/Services/Security/ResendVerificationThrottle.phptests/Unit/BootstrapTest.phptests/Unit/Cms/Container/CmsServiceProviderTest.phptests/Unit/Cms/ContentControllerTest.phptests/Unit/Cms/Controllers/Admin/DashboardTest.phptests/Unit/Cms/Controllers/Admin/ProfileTest.phptests/Unit/Cms/Controllers/CalendarTest.phptests/Unit/Cms/Controllers/HomeTest.phptests/Unit/Cms/Controllers/Member/DashboardTest.phptests/Unit/Cms/Controllers/Member/ProfileTest.phptests/Unit/Cms/Controllers/Member/RegistrationTest.phptests/Unit/Cms/Controllers/PagesTest.phptests/Unit/Cms/Services/Security/ResendVerificationThrottleTest.php
✅ Files skipped from review due to trivial changes (1)
- tests/Unit/Cms/Services/Security/ResendVerificationThrottleTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/Unit/Cms/Controllers/Member/ProfileTest.php
- src/Cms/Container/Container.php
- tests/Unit/Cms/Controllers/Member/DashboardTest.php
🧰 Additional context used
🧬 Code graph analysis (8)
tests/Unit/Cms/Controllers/HomeTest.php (1)
src/Cms/Controllers/Home.php (1)
Home(22-75)
src/Cms/Controllers/Member/Registration.php (1)
src/Cms/Services/Security/ResendVerificationThrottle.php (1)
ResendVerificationThrottle(19-195)
tests/Unit/Cms/ContentControllerTest.php (2)
src/Cms/Auth/SessionManager.php (2)
set(125-129)SessionManager(13-216)src/Cms/Controllers/Content.php (1)
Content(62-402)
tests/Unit/Cms/Controllers/Member/RegistrationTest.php (2)
src/Cms/Services/Security/ResendVerificationThrottle.php (1)
ResendVerificationThrottle(19-195)src/Cms/Controllers/Member/Registration.php (1)
Registration(29-268)
src/Cms/Container/CmsServiceProvider.php (1)
src/Cms/Services/Security/ResendVerificationThrottle.php (1)
ResendVerificationThrottle(19-195)
src/Cms/Controllers/Content.php (1)
src/Cms/Auth/SessionManager.php (2)
SessionManager(13-216)get(134-138)
tests/Unit/Cms/Container/CmsServiceProviderTest.php (1)
src/Cms/Services/Security/ResendVerificationThrottle.php (1)
ResendVerificationThrottle(19-195)
tests/Unit/Cms/Controllers/CalendarTest.php (1)
src/Cms/Container/Container.php (1)
getInstance(268-271)
⏰ 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). (4)
- GitHub Check: Cursor Bugbot
- GitHub Check: build-test (postgres)
- GitHub Check: build-test (mysql)
- GitHub Check: build-test (sqlite)
🔇 Additional comments (35)
tests/Unit/BootstrapTest.php (1)
17-27: LGTM! Whitespace improvements enhance readability.The blank lines added in the
setUpmethod improve visual separation between logical blocks (property declarations, parent setup call, virtual filesystem setup, and environment handling), making the test initialization easier to scan.tests/Unit/Cms/Controllers/CalendarTest.php (6)
22-58: Excellent test isolation and cleanup practices.The setUp and tearDown methods demonstrate strong test isolation:
- Unique temporary version file prevents inter-test conflicts
- In-memory settings ensure no external dependencies
- Thorough Registry cleanup prevents state leakage
- Safe file deletion with existence check
60-70: Good constructor validation.This test confirms the controller accepts its dependencies correctly. Using
nullfor the Application parameter is appropriate for isolated unit testing.
72-156: Comprehensive coverage of calendar rendering scenarios.These tests effectively validate:
- Default current-month rendering with proper data structure
- Specific month/year parameter handling with accurate date range calculations (lines 119-125)
- Appropriate use of callbacks for complex DateTime assertions
The match expression for parameter mapping (lines 146-150) is clean and maintainable.
158-243: Thorough testing of event display and error handling.Excellent coverage of:
- Published event rendering with view count increment verification (lines 167-169)
- Proper exception handling for both nonexistent and unpublished events
- Appropriate use of PHPUnit's expectException API
245-304: Complete category listing validation.These tests properly verify:
- Category resolution by slug and event filtering by category ID with published status (line 252)
- Required view data structure including category, events, and metadata
- Exception handling for nonexistent categories
306-343: Valuable edge case coverage for missing descriptions.This test validates the fallback behavior when an event's description is null, ensuring the view receives the event title as the Description field. Good defensive programming validation.
tests/Unit/Cms/Controllers/Admin/ProfileTest.php (3)
24-42: LGTM! Properly addresses the past review comment.The setUp() method now correctly uses
sys_get_temp_dir()with a unique identifier for the version file, storing the path in$_versionFilePathfor cleanup. This resolves the previous concerns about unpredictable paths and race conditions.
44-61: LGTM! Thorough cleanup implementation.The tearDown() method properly cleans up all Registry entries and safely removes the temporary version file with appropriate existence checks.
63-98: LGTM! Tests follow established patterns.Both test methods are consistent with similar controller tests in the codebase (TagsTest, PagesTest, CategoriesTest). The docblock at lines 64-71 provides valuable context about the controller's testing limitations.
tests/Unit/Cms/Controllers/Admin/DashboardTest.php (4)
25-39: LGTM! Proper temp file setup.The setUp() method correctly uses
sys_get_temp_dir()with a unique identifier for the version file and configures Memory-based settings appropriately.
45-61: LGTM! Thorough cleanup implementation.The tearDown() method properly cleans up all Registry entries and safely removes the temporary version file.
63-74: LGTM! Constructor tests validate dependency injection.Both constructor tests appropriately verify that the Dashboard controller can be instantiated with the required dependencies.
76-106: LGTM! View rendering test with appropriate isolation.The test correctly uses
@runInSeparateProcessfor isolation and properly mocks the fluent ViewContext interface to validate the rendering flow.src/Cms/Controllers/Member/Registration.php (2)
7-7: LGTM: Import updated to reflect namespace reorganization.The import path correctly reflects the namespace change of
ResendVerificationThrottlefromNeuron\Cms\AuthtoNeuron\Cms\Services\Security.
59-82: LGTM: Explicit dependency validation enforces strict DI.The fail-fast validation for all four required dependencies (
IRegistrationService,IEmailVerifier,ResendVerificationThrottle,IIpResolver) is consistent with the PR's goal of removing container fallbacks and enforcing explicit injection.src/Cms/Services/Security/ResendVerificationThrottle.php (1)
3-3: LGTM: Clean namespace reorganization.The namespace move from
Neuron\Cms\AuthtoNeuron\Cms\Services\Securitybetter reflects the class's purpose as a security service. The docblock is correctly updated to match.Also applies to: 17-17
src/Cms/Container/CmsServiceProvider.php (3)
29-29: LGTM: Import updated for namespace change.The import correctly reflects the namespace reorganization of
ResendVerificationThrottle.
36-37: LGTM: IP resolver interface imports added.The imports support the new IP resolver binding introduced in this PR.
115-120: LGTM: Auth service bindings configured correctly.The singleton binding for
ResendVerificationThrottleand interface binding forIIpResolver→DefaultIpResolverare properly configured within the auth services registration.tests/Unit/Cms/Container/CmsServiceProviderTest.php (1)
6-6: LGTM: Test import synchronized with namespace change.The import path correctly updated to match the production code namespace reorganization.
src/Cms/Controllers/Content.php (3)
69-70: LGTM: Properties correctly declared as non-nullable.The properties are now non-nullable, which aligns with the strict DI enforcement in the constructor.
85-96: LGTM: Explicit dependency validation enforces strict DI.The fail-fast validation for required dependencies (
SettingManagerandSessionManager) is consistent with the PR's goal of removing service locator fallbacks.
98-102: LGTM: Direct property access is safe.Since the constructor now guarantees non-null dependencies, direct access to
$this->_settingsis safe and correct.Also applies to: 109-110
tests/Unit/Cms/ContentControllerTest.php (4)
24-31: LGTM! Good test isolation approach.Creating a real temporary version file with a unique name ensures test isolation and avoids conflicts. The cleanup in
tearDown(lines 61-64) properly removes the file.
44-46: Verify that no SessionManager behavior needs to be mocked.The mock SessionManager is created without any configured expectations or return values. Ensure that the Content controller methods being tested don't invoke SessionManager methods that require stubbed behavior. Based on the test implementations, this appears intentional (testing dependency wiring rather than behavior), but worth confirming.
61-64: LGTM! Proper cleanup.The tearDown correctly guards against missing property or file before unlinking, properly cleaning up the temporary file created in setUp.
74-74: LGTM! Consistent constructor updates.All Content instantiations are consistently updated to pass the three required arguments (null Application, SettingManager, SessionManager), aligning with the new explicit dependency injection pattern.
Also applies to: 94-94, 122-122, 150-150, 177-177, 200-200
tests/Unit/Cms/Controllers/Member/RegistrationTest.php (5)
127-141: LGTM! Comprehensive constructor validation test.The test properly verifies that the Registration controller throws
InvalidArgumentExceptionwith the message "IRegistrationService must be injected" when the required dependency is null. This aligns with the PR's goal of enforcing explicit dependency injection.
143-157: LGTM! Consistent validation test for IEmailVerifier.The test follows the same pattern as the previous constructor test, ensuring that IEmailVerifier is properly validated as a required dependency.
159-173: LGTM! Consistent validation test for ResendVerificationThrottle.The test ensures that ResendVerificationThrottle is properly validated as a required dependency, maintaining consistency with the other constructor validation tests.
175-189: LGTM! Complete constructor validation coverage.This test completes the comprehensive coverage of all four required dependencies for the Registration controller. Each dependency (IRegistrationService, IEmailVerifier, ResendVerificationThrottle, and IIpResolver) now has a dedicated test verifying that the constructor properly enforces explicit injection.
8-8: Namespace migration is complete and consistent. All references toResendVerificationThrottlehave been updated to the newNeuron\Cms\Services\Securitynamespace across the codebase, with no stale imports remaining in the oldNeuron\Cms\Authnamespace. The class definition exists at the new location and is properly imported in all relevant files.tests/Unit/Cms/Controllers/PagesTest.php (2)
75-75: No action needed - null is a valid first parameter for the Pages constructor.The
Pagesconstructor explicitly declares the first parameter as nullable (?Application $app = null) with a default value ofnull. Passingnullis intentional and valid by design. The strict dependency injection requirements are for the$pageRepositoryand$rendererparameters, which throwInvalidArgumentExceptionif null.
22-48: Remove unused version file setup code.While the switch to
sys_get_temp_dir()with a unique ID is a good improvement, the version file creation and configuration is unused—the Pages controller does not reference theversion_filesetting. Remove lines 26-33 (version file creation) and line 41 (the setting assignment) to simplify the test. TheSettingManagerinstance is still necessary since it's passed to the parent constructor, but keep only the actually-used settings:site.name,site.title,site.description, andsite.url.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/Unit/Cms/Controllers/HomeTest.php (1)
148-179: Consider removing this duplicate test.This test is nearly identical to
testIndexWithRegistrationEnabled(lines 84-116)—both mockisRegistrationEnabled()to returntrueand verify the same data keys with the same assertions. The only difference is this test omits the return value assertion, making it strictly less comprehensive.Either remove this test or differentiate it by testing a distinct scenario (e.g., verifying specific values like
Titlematching the configured setting).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/Unit/Cms/Controllers/HomeTest.php
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Unit/Cms/Controllers/HomeTest.php (1)
src/Cms/Controllers/Home.php (1)
Home(22-75)
⏰ 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). (4)
- GitHub Check: Cursor Bugbot
- GitHub Check: build-test (postgres)
- GitHub Check: build-test (sqlite)
- GitHub Check: build-test (mysql)
🔇 Additional comments (6)
tests/Unit/Cms/Controllers/HomeTest.php (6)
19-45: Well-structured test fixture setup.Good use of temporary directory with unique suffix for test isolation, and proper in-memory settings configuration.
47-63: Good cleanup practice.Proper cleanup of Registry state and temp files prevents test pollution.
65-73: Previous feedback addressed.The test now correctly uses
$this->_settingManagerinstance property instead of fetching from Registry.
75-82: LGTM!The test correctly validates that the constructor enforces required dependencies.
84-116: LGTM!Well-structured test using partial mock to verify
renderHtmlis called with expected data structure.
118-146: LGTM!Good complementary test covering the registration-disabled scenario.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.
Note
Refactor/DI
SettingManagerandSessionManager(and other deps); remove service-locator fallbacks and throw on missing depsContentuses DI-only and reads version file viasettings.paths.version_fileContainer
EditorJsRenderer,CloudinaryUploader,MediaValidator,ResendVerificationThrottle; bindIIpResolvertoDefaultIpResolverSecurity
ResendVerificationThrottletoNeuron\Cms\Services\SecurityTests
Written by Cursor Bugbot for commit 7b769ee. This will update automatically on new commits. Configure here.