feat: architecture improvements - #13
Conversation
- Remove direct DateService::getNextDate() and DateService::getNextDates() calls - Remove BackendUserHelper::checkAndSetModuleDate() calls - Clean up unused imports (DateService, BackendUserHelper) - Domain models should only contain data, not business logic dependencies This improves separation of concerns and makes the domain model more testable by removing tight coupling to service layers.
WalkthroughThis update refactors several core services and utilities from static to dependency-injected, instance-based design. The Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Pull Request Test Coverage Report for Build 16667925858Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 16667691540Details
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
Classes/Utilities/BackendUserHelper.php (2)
28-28: Consider using more specific type instead of mixedThe
mixedtype for the$valueparameter is too broad. Based on the usage context (comparing within_array), consider usingstring|intor a more specific type that matches the actual use cases.- public function checkAndSetModuleDate(string $moduleName, mixed $value): bool + public function checkAndSetModuleDate(string $moduleName, string|int $value): bool
31-43: Consider dependency injection for better testabilityWhile the validation improves robustness, direct access to
$GLOBALS['BE_USER']makes testing more difficult. Consider injecting the backend user context as a dependency to improve testability and reduce coupling to global state.Classes/ViewHelpers/NextDatesViewHelper.php (2)
40-43: Remove redundant type checking.Since the
newsargument is registered withNews::classtype andrequired=true, the type check is redundant. TYPO3 will ensure type safety.- $news = $this->arguments['news']; - if (!$news instanceof News) { - return []; - } + $news = $this->arguments['news'];
45-46: Consider dependency injection for better testability.While
GeneralUtility::makeInstanceworks, injectingNewsServicevia constructor would improve testability and follow modern TYPO3 patterns.+ public function __construct( + private readonly NewsService $newsService + ) {} + public function render(): array { $news = $this->arguments['news']; - $newsService = GeneralUtility::makeInstance(NewsService::class); - return $newsService->getNextDates($news); + return $this->newsService->getNextDates($news); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
Classes/Backend/ToolbarItems/NewsItem.php(2 hunks)Classes/Controller/DateController.php(1 hunks)Classes/Domain/Model/News.php(0 hunks)Classes/Service/DateService.php(6 hunks)Classes/Service/NewsService.php(1 hunks)Classes/Utilities/BackendUserHelper.php(1 hunks)Classes/Utilities/UserFunc.php(1 hunks)Classes/ViewHelpers/IsNewViewHelper.php(1 hunks)Classes/ViewHelpers/IsTopAndNewViewHelper.php(1 hunks)Classes/ViewHelpers/NextDateViewHelper.php(1 hunks)Classes/ViewHelpers/NextDatesViewHelper.php(1 hunks)Configuration/Services.yaml(1 hunks)Resources/Private/Partials/Detail.html(2 hunks)Resources/Private/Partials/Item.html(1 hunks)Resources/Private/Templates/Backend/ToolbarItems/NewsItemDropDown.html(2 hunks)Tests/Unit/Backend/ToolbarItems/NewsItemTest.php(2 hunks)Tests/Unit/Controller/DateControllerTest.php(5 hunks)Tests/Unit/Service/DateServiceTest.php(12 hunks)Tests/Unit/Utilities/BackendUserHelperTest.php(8 hunks)composer.json(1 hunks)
💤 Files with no reviewable changes (1)
- Classes/Domain/Model/News.php
🧰 Additional context used
🧬 Code Graph Analysis (3)
Classes/ViewHelpers/IsNewViewHelper.php (4)
Classes/Domain/Model/News.php (3)
News(31-123)News(14-129)isNew(104-107)Classes/Service/NewsService.php (2)
NewsService(29-58)isNew(54-57)Classes/ViewHelpers/IsTopAndNewViewHelper.php (2)
initializeArguments(33-36)render(38-47)Classes/ViewHelpers/NextDatesViewHelper.php (2)
initializeArguments(33-36)render(38-47)
Classes/ViewHelpers/IsTopAndNewViewHelper.php (3)
Classes/Domain/Model/News.php (4)
News(31-123)News(14-129)isTopAndNew(96-102)isNew(104-107)Classes/Service/NewsService.php (2)
NewsService(29-58)isTopAndNew(46-52)Classes/ViewHelpers/IsNewViewHelper.php (2)
initializeArguments(33-36)render(38-47)
Classes/Utilities/BackendUserHelper.php (2)
Tests/Unit/Utilities/BackendUserHelperTest.php (2)
getModuleData(173-176)pushModuleData(178-181)Classes/Domain/Model/News.php (1)
isNew(104-107)
🔇 Additional comments (40)
Configuration/Services.yaml (2)
19-19: LGTM: Formatting improvementThe blank line addition improves readability by separating service definitions.
1-50: No explicit service definitions needed for NewsService and DateServiceAutowiring and autoconfiguration under your
_defaultsresource grab all classes inClasses/, includingService/NewsService.phpandService/DateService.php. Both are registered automatically with their constructor-injected dependencies:
- NewsService (depends on DateService and BackendUserHelper)
- DateService (depends on ExtensionConfiguration)
You don’t need to add explicit YAML entries for these. The current setup already covers them—no further action required.
Likely an incorrect or invalid review comment.
Classes/Utilities/BackendUserHelper.php (1)
28-33: Good defensive programming with backend user validationThe validation of
$GLOBALS['BE_USER']existence and type is a good defensive programming practice that prevents potential runtime errors.Classes/Utilities/UserFunc.php (1)
41-42: LGTM: Consistent with service refactoringThe change from static to instance method call aligns with the architectural improvements. Using
GeneralUtility::makeInstance()is appropriate here since UserFunc classes are typically instantiated by TYPO3's TCA system where constructor injection isn't readily available.Resources/Private/Templates/Backend/ToolbarItems/NewsItemDropDown.html (2)
2-3: LGTM: Proper namespace declaration for custom ViewHelpersThe XML namespace declaration for the custom ViewHelpers is correctly added and follows TYPO3 conventions.
21-21: Excellent separation of concernsReplacing the direct property access
record.topAndNewwith the ViewHelper callxin:isTopAndNew(news: record)properly separates presentation logic from domain models. This allows the business logic to be handled by the service layer while keeping templates focused on presentation.Classes/Controller/DateController.php (2)
42-43: LGTM! Proper dependency injection implementation.The addition of
DateServiceas a readonly dependency follows best practices for dependency injection and improves testability.
51-51: LGTM! Consistent refactoring from static to instance method.The change from static call to instance method call on the injected
DateServiceis consistent with the architectural improvements and enhances maintainability.Tests/Unit/Controller/DateControllerTest.php (3)
32-32: LGTM! Proper test setup for new dependency.The addition of
DateServicemock and its proper initialization in the test setup correctly reflects the production code changes.Also applies to: 39-39, 51-53
62-66: LGTM! Constructor updated with all required dependencies.The constructor call now properly includes all three dependencies in the correct order, matching the production code signature.
120-120: LGTM! Comprehensive parameter validation.The test correctly verifies that the constructor now accepts three parameters and validates the new
DateServiceparameter's name and type.Also applies to: 134-138
Resources/Private/Partials/Item.html (2)
2-4: LGTM! Proper namespace declaration for ViewHelpers.The addition of the
xinnamespace for custom ViewHelpers follows Fluid template conventions and enables clean access to extension-specific ViewHelpers.
5-5: LGTM! Clean separation of concerns with ViewHelper.The use of
xin:nextDateViewHelper instead of direct property accessrecord.nextDateproperly separates presentation logic from domain models and aligns with the architectural improvements.Tests/Unit/Utilities/BackendUserHelperTest.php (3)
32-32: LGTM! Proper instance-based test setup.The addition of the
BackendUserHelperproperty and its instantiation insetUp()correctly reflects the change from static to instance methods.Also applies to: 39-39
53-53: LGTM! Consistent instance method calls.All test method calls have been properly updated from static calls to instance method calls on the
$this->backendUserHelperobject.Also applies to: 65-65, 69-69, 79-79, 89-90, 102-104, 118-118
128-128: LGTM! Correct verification of non-static method.The test correctly verifies that
checkAndSetModuleDateis no longer a static method, reflecting the architectural change to instance-based usage.Tests/Unit/Backend/ToolbarItems/NewsItemTest.php (3)
31-31: LGTM! Proper mock setup for new dependency.The addition of
NewsServicemock and its proper initialization correctly supports the updated constructor signature in the production code.Also applies to: 37-37, 45-48
49-49: LGTM! Constructor updated with both dependencies.The constructor call now properly includes both
NewsRepositoryandNewsServicedependencies, matching the production code changes.
75-75: LGTM! Comprehensive parameter validation.The test correctly verifies that the constructor now accepts two parameters and validates both the
newsRepositoryandnewsServiceparameters with their proper names and types.Also applies to: 78-78, 84-86
Resources/Private/Partials/Detail.html (2)
2-3: LGTM! Proper namespace declaration for custom ViewHelpers.The addition of the
xinnamespace for custom ViewHelpers follows TYPO3 conventions and enables clean usage of the new service-based ViewHelpers throughout the template.
47-53: Excellent refactoring to use service-based ViewHelper.The change from direct property access (
{record.nextDates}) to thexin:nextDatesViewHelper properly encapsulates the date logic in the service layer, improving separation of concerns and maintainability.Classes/ViewHelpers/NextDateViewHelper.php (2)
31-48: Solid ViewHelper implementation with proper service delegation.The ViewHelper correctly delegates to the
NewsServiceand includes appropriate type checking. The use ofGeneralUtility::makeInstance()is standard practice for ViewHelpers in TYPO3, though it creates a service locator dependency.
38-43: Good defensive programming with type validation.The explicit
instanceofcheck ensures type safety even though the argument is registered with theNewsclass type. This prevents potential runtime errors if the argument is somehow invalid.Classes/ViewHelpers/IsTopAndNewViewHelper.php (1)
31-48: Consistent ViewHelper implementation following established patterns.The ViewHelper follows the same architectural pattern as other ViewHelpers in the codebase, properly delegating business logic to the
NewsService. The type checking and service instantiation are handled correctly.Classes/Backend/ToolbarItems/NewsItem.php (2)
35-38: Excellent refactoring to dependency injection.The constructor now properly injects the
NewsServicedependency, moving away from static method calls to a cleaner, more testable architecture. This follows SOLID principles and improves maintainability.
58-58: Clean service-based filtering logic.The refactoring from
$item->isNew()to$this->newsService->isNew($item)properly delegates the business logic to the service layer. The functional approach witharray_filterand arrow function is concise and readable.Classes/ViewHelpers/IsNewViewHelper.php (2)
31-48: Well-implemented ViewHelper maintaining architectural consistency.The ViewHelper properly follows the established pattern of delegating to
NewsServicewhile maintaining type safety. The implementation is clean and aligns with the broader refactoring to service-based architecture.
38-47: Appropriate service delegation and return type.The method correctly delegates the
isNewcheck to theNewsServiceand maintains the boolean return type contract. The defensive type checking ensures robustness.Classes/ViewHelpers/NextDatesViewHelper.php (1)
33-36: LGTM!The argument registration follows TYPO3 ViewHelper conventions correctly.
Tests/Unit/Service/DateServiceTest.php (3)
37-62: LGTM!The setUp method correctly creates a
DateServiceinstance with proper dependency injection of the mockedExtensionConfiguration. The mock configuration is appropriate for testing.
75-75: LGTM!All method calls correctly updated from static to instance method calls using the injected
DateService.Also applies to: 97-97, 109-109, 118-118, 142-142, 154-154, 167-167
88-88: LGTM!Reflection tests correctly updated to assert methods are no longer static, reflecting the architectural change.
Also applies to: 131-131
Classes/Service/NewsService.php (4)
31-34: LGTM!Clean dependency injection with readonly properties following modern PHP patterns.
36-44: LGTM!Clean delegation to
DateServicewith proper return type declarations. The service layer abstraction is well implemented.
46-52: LGTM!Logical flow with early return optimization and proper delegation to
BackendUserHelper. The module key'internal_news/top'appears consistent with the application's naming conventions.
54-57: LGTM!Clean delegation to
BackendUserHelperwith appropriate module key for tracking read status.Classes/Service/DateService.php (4)
35-37: LGTM!Proper dependency injection of
ExtensionConfigurationwith readonly property, enabling better testability and removing static dependencies.
42-42: LGTM!All internal method calls correctly updated from static to instance method calls, maintaining consistency with the architectural refactoring.
Also applies to: 56-56, 69-69, 84-84, 94-94, 115-115
125-125: LGTM!Configuration access properly updated to use the injected
ExtensionConfigurationdependency instead of static access, improving testability.
38-38: LGTM!Public method signatures preserved while correctly removing static modifiers, ensuring clean migration to instance-based usage.
Also applies to: 51-51, 64-64, 78-78
2 participants
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
Chores