refactor: process service replacement - #38
Conversation
Replace the ProcessFactory class with ProcessService to provide a more standardized service layer implementation with dependency injection support and better testability. The new service encapsulates process execution logic while maintaining the same interface compatibility.
Introduce SiteDTO for site configuration data and SiteRepository for inventory management. This establishes foundational components for site-related functionality, following the same pattern as server management.
Update BaseCommand with improved console output and input methods. Adjust test fixtures and helpers to maintain compatibility and enhance test isolation for command testing.
Remove the deprecated ProcessFactory class and corresponding test file following successful replacement with ProcessService.
Apply small fixes and improvements to the VersionService implementation for better reliability and consistency with service layer patterns.
|
Caution Review failedThe pull request is closed. WalkthroughAdds a readonly SiteDTO and an inventory-backed SiteRepository; introduces ProcessService and replaces ProcessFactory usage; BaseCommand now injects ProcessService and SiteRepository and loads site inventory during initialization; tests and test helpers updated to match new DI and services. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as Console Command
participant Base as BaseCommand
participant Inv as InventoryService
participant Sites as SiteRepository
participant Servers as ServerRepository
User->>CLI: invoke command
CLI->>Base: __construct(...)
activate Base
Base->>Sites: loadInventory(Inv)
Sites->>Inv: get("sites")
Inv-->>Sites: array|null
Sites-->>Base: initialized
Base->>Servers: loadInventory(Inv)
Servers-->>Base: initialized
deactivate Base
Note right of CLI: Repositories populated from inventory before command runs
sequenceDiagram
autonumber
participant Ver as VersionService
participant Proc as ProcessService
participant OS as System Process
Ver->>Proc: run(["git","describe","--tags"], cwd, timeout)
activate Proc
Proc->>OS: start and run command
OS-->>Proc: exit code + output
Proc-->>Ver: completed Process
Ver->>Ver: check isSuccessful(), parse output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
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 (2)
app/Services/ProcessService.php (1)
24-36: Consider simplifying the timeout handling.The null-coalescing operator on line 35 is redundant since the parameter already has a default value of
3.0. The?? 3.0will only apply whennullis explicitly passed, which is unlikely given the default.Consider this simpler approach:
- public function run(array $command, string $cwd, ?float $timeout = 3.0): Process + public function run(array $command, string $cwd, float|null $timeout = 3.0): Process { if ($command === []) { throw new \InvalidArgumentException('Process command cannot be empty'); } if (!$this->fs->isDirectory($cwd)) { throw new \InvalidArgumentException("Invalid working directory: {$cwd}"); } $process = new Process($command, $cwd); - $process->setTimeout($timeout ?? 3.0); + $process->setTimeout($timeout); $process->run(); return $process; }app/Contracts/BaseCommand.php (1)
109-114: Consider lazy loading for site inventory.The site inventory is now loaded eagerly for all commands, even those that may not need it. Since
SiteRepositoryalready follows the stateful service pattern with explicitloadInventory(), consider deferring this initialization to commands that actually use site data.However, this matches the pattern used for
ServerRepository(line 112), so consistency may be more valuable than the minor performance gain from lazy loading.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/Contracts/BaseCommand.php(3 hunks)app/DTOs/SiteDTO.php(1 hunks)app/Repositories/SiteRepository.php(1 hunks)app/Services/ProcessService.php(2 hunks)app/Services/VersionService.php(4 hunks)tests/Fixtures/TestConsoleCommand.php(2 hunks)tests/TestHelpers.php(6 hunks)tests/Unit/Contracts/BaseCommandTest.php(5 hunks)tests/Unit/DTOs/SiteDTOTest.php(1 hunks)tests/Unit/Repositories/SiteRepositoryTest.php(1 hunks)tests/Unit/Services/ProcessServiceTest.php(3 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.php
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
**/*.php: Eliminate single-use private methods by inlining them directly
Cache expensive computed values by initializing them in the constructor instead of recomputing
Prefer direct property access over method calls when appropriate to avoid call overhead
Organize code into comment-separated sections; prefer alphabetical ordering when it does not conflict with logical grouping
**/*.php: Adhere to PSR-12 coding style in all PHP files
Declare strict_types=1 at the top of every PHP file
Prefer PHP 8.x features (union types, match, attributes, readonly) where appropriate
Always import classes with use statements; avoid fully qualified class names in code bodies
All methods must declare explicit return types; use proper generics in types/docblocks (e.g., Collection<int, User>)
Use dependency injection instead of manually resolving or instantiating classes
Prefer Symfony component classes (e.g., Filesystem, Process) over native PHP functions for testability
All object creation must use $container->build(ClassName::class) instead of new, except for value objects/DTOs/pure data structures
In production code, access the Container via constructor injection, not via static/global access
Add minimal DocBlock comments with descriptions, parameters, and return types for classes and functions
Use comments as visual separators for sections/subsections with a single newline between header, subheader, and paragraph; avoid obvious or stale comments
Run rector on changed PHP files before completing a task
Run pint on changed PHP files to fix code style before completing a task
Files:
app/DTOs/SiteDTO.phptests/Unit/Repositories/SiteRepositoryTest.phptests/Unit/Services/ProcessServiceTest.phpapp/Repositories/SiteRepository.phptests/Fixtures/TestConsoleCommand.phptests/Unit/Contracts/BaseCommandTest.phpapp/Contracts/BaseCommand.phpapp/Services/VersionService.phpapp/Services/ProcessService.phptests/Unit/DTOs/SiteDTOTest.phptests/TestHelpers.php
{tests/**,test/**,**/*@(Test|Spec).php}
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
Do not run or edit tests unless explicitly instructed
Files:
tests/Unit/Repositories/SiteRepositoryTest.phptests/Unit/Services/ProcessServiceTest.phptests/Fixtures/TestConsoleCommand.phptests/Unit/Contracts/BaseCommandTest.phptests/Unit/DTOs/SiteDTOTest.phptests/TestHelpers.php
tests/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
tests/**/*.php: In tests, direct Container instantiation and bind() for mocks is allowed and encouraged for isolation
Do not run PHPStan on test files; tests are excluded from static analysis
tests/**/*.php: Unit tests must instantiate services manually (no DI container)
Command/integration tests must use mockCommandContainer() for building commands and overriding services
Only use container auto-wiring in tests to verify DI configuration or multi-service integration (edge cases)
Keep test files under 1.8x the size of the source they test (without sacrificing readability)
Test core business logic; avoid testing the framework itself
Prefer dataset-driven testing using ->with([...]) for multiple scenarios
Consolidate related assertions (e.g., expect($x)->toBe(...)->and($y)->toBe(...))
Mock only external dependencies; keep unit tests isolated from filesystem/HTTP/processes
Avoid performance tests unless performance is the primary concern
Use the AAA pattern in tests (Arrange, Act, Assert; optional Cleanup)
In exception tests, use a combined // ACT & ASSERT step when the act triggers the assertion
Organize tests with describe() blocks, beforeEach() setup, and shared helpers/traits for DRY
Forbidden assertions in tests: type-only or generic checks (e.g., toBeInstanceOf, toBeArray, not->toBeNull, expect(true)->toBeTrue) and sleep(...); prefer time mocking
Preferred assertions: assert observable behavior and interactions (e.g., domain values, validator outcomes, mock expectations)
Unit tests: mock all external dependencies, test single units in isolation, and complete in milliseconds
Integration tests: use real file operations and external processes; cover CLI commands and full workflows
Do not require PHPStan compliance in tests; avoid excessive phpdoc solely to satisfy types in tests
Files:
tests/Unit/Repositories/SiteRepositoryTest.phptests/Unit/Services/ProcessServiceTest.phptests/Fixtures/TestConsoleCommand.phptests/Unit/Contracts/BaseCommandTest.phptests/Unit/DTOs/SiteDTOTest.phptests/TestHelpers.php
**/*Command.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
**/*Command.php: Commands handle user interaction (input/output) and orchestrate services
Commands must not contain business logic; delegate business logic to Services
Commands must not duplicate orchestration logic; extract shared orchestration to Services
Commands should not invoke other commands (no proxy commands)
Use SymfonyStyle consistently for all user-facing console output
Files:
tests/Fixtures/TestConsoleCommand.phpapp/Contracts/BaseCommand.php
app/{Contracts/BaseCommand.php,Traits/ConsoleOutputTrait.php,Traits/ConsoleInputTrait.php}
📄 CodeRabbit inference engine (.cursor/rules/03-commands.mdc)
If output functionality is missing, add a new method to BaseCommand (and the appropriate Console*Trait) with modern styling and documentation
Files:
app/Contracts/BaseCommand.php
app/Contracts/BaseCommand.php
📄 CodeRabbit inference engine (.cursor/rules/03-commands.mdc)
BaseCommand should contain only shared initialization/configuration/orchestration logic; do not put individual I/O operations here
Files:
app/Contracts/BaseCommand.php
**/*Service.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
**/*Service.php: Services provide atomic, reusable functionality with no console I/O
Services accept and return plain PHP data types (no console I/O)
Services must be dependency-injected via constructor; declare dependencies in constructor signatures
Extract complex orchestration shared by multiple Commands into dedicated Services
Stateful services should use lazy loading when initialization is expensive or path-dependent
State must be initialized explicitly via public methods (e.g., load(), initialize()) before use
Services should document their stateful nature and initialization requirements
Files:
app/Services/VersionService.phpapp/Services/ProcessService.php
app/Services/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/03-commands.mdc)
Services must not perform any console I/O; they should return plain data only
Files:
app/Services/VersionService.phpapp/Services/ProcessService.php
tests/TestHelpers.php
📄 CodeRabbit inference engine (.cursor/rules/02-tests.mdc)
When BaseCommand gains a new service, update mockCommandContainer() in tests/TestHelpers.php by adding a parameter, constructing/binding it, and wiring it to the container
Files:
tests/TestHelpers.php
🧠 Learnings (2)
📚 Learning: 2025-10-10T11:10:25.074Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-10-10T11:10:25.074Z
Learning: Applies to tests/TestHelpers.php : When BaseCommand gains a new service, update mockCommandContainer() in tests/TestHelpers.php by adding a parameter, constructing/binding it, and wiring it to the container
Applied to files:
tests/Fixtures/TestConsoleCommand.phptests/Unit/Contracts/BaseCommandTest.phptests/TestHelpers.php
📚 Learning: 2025-10-10T11:10:25.074Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-10-10T11:10:25.074Z
Learning: Applies to tests/**/*.php : Command/integration tests must use mockCommandContainer() for building commands and overriding services
Applied to files:
tests/Unit/Contracts/BaseCommandTest.phptests/TestHelpers.php
🧬 Code graph analysis (10)
tests/Unit/Repositories/SiteRepositoryTest.php (4)
app/DTOs/SiteDTO.php (1)
SiteDTO(7-19)app/Repositories/SiteRepository.php (6)
SiteRepository(15-165)all(83-93)loadInventory(31-43)create(48-60)findByDomain(65-76)delete(98-112)tests/TestHelpers.php (1)
mockInventoryService(156-172)app/Services/InventoryService.php (1)
loadInventoryFile(97-111)
tests/Unit/Services/ProcessServiceTest.php (2)
tests/TestHelpers.php (1)
mockProcessService(186-190)app/Services/ProcessService.php (1)
run(24-39)
app/Repositories/SiteRepository.php (2)
app/DTOs/SiteDTO.php (1)
SiteDTO(7-19)app/Services/InventoryService.php (2)
InventoryService(33-264)set(54-60)
tests/Fixtures/TestConsoleCommand.php (3)
app/Repositories/SiteRepository.php (1)
SiteRepository(15-165)app/Services/ProcessService.php (1)
ProcessService(12-40)app/Contracts/BaseCommand.php (1)
__construct(38-54)
tests/Unit/Contracts/BaseCommandTest.php (4)
app/Repositories/SiteRepository.php (2)
SiteRepository(15-165)findByDomain(65-76)tests/TestHelpers.php (2)
mockCommandContainer(410-449)mockInventoryService(156-172)app/Container.php (1)
build(59-85)app/Repositories/ServerRepository.php (2)
ServerRepository(15-168)findByName(65-76)
app/Contracts/BaseCommand.php (2)
app/Repositories/SiteRepository.php (1)
SiteRepository(15-165)app/Services/ProcessService.php (1)
ProcessService(12-40)
app/Services/VersionService.php (1)
app/Services/ProcessService.php (2)
ProcessService(12-40)run(24-39)
app/Services/ProcessService.php (1)
app/Services/FilesystemService.php (1)
FilesystemService(27-106)
tests/Unit/DTOs/SiteDTOTest.php (1)
app/DTOs/SiteDTO.php (1)
SiteDTO(7-19)
tests/TestHelpers.php (4)
app/Repositories/SiteRepository.php (1)
SiteRepository(15-165)app/Services/InventoryService.php (2)
InventoryService(33-264)loadInventoryFile(97-111)app/Services/ProcessService.php (1)
ProcessService(12-40)app/Container.php (2)
Container(23-250)bind(46-50)
🔇 Additional comments (16)
app/DTOs/SiteDTO.php (1)
7-19: LGTM!The readonly DTO follows PHP 8.x best practices with constructor property promotion and proper type declarations. The implementation is clean and appropriate for an immutable value object.
app/Services/VersionService.php (4)
21-21: LGTM!The dependency injection change from
ProcessFactorytoProcessServicealigns with the broader refactor and maintains proper service isolation.
117-127: LGTM!The migration to
ProcessService::run()is clean and maintains the same error handling logic. The method correctly checksisSuccessful()on the returned process.
135-145: LGTM!Consistent migration to the new
ProcessService::run()API with proper error handling.
153-166: LGTM!The refactor correctly uses
ProcessService::run()for both git commands and combines their outputs when both succeed. The error handling is appropriate.tests/Unit/DTOs/SiteDTOTest.php (1)
7-23: LGTM!The test properly validates SiteDTO construction with named parameters and verifies all properties using chained Pest assertions. Good coverage for a simple value object.
tests/Unit/Services/ProcessServiceTest.php (3)
14-32: LGTM!The test correctly validates the new
run()method behavior, including the important addition of theisSuccessful()assertion. The parameterized timeout scenarios provide good coverage.
34-41: LGTM!Exception test properly updated to use the new
run()API while preserving validation logic.
43-56: LGTM!The parameterized test for invalid working directories covers multiple edge cases and correctly validates exception behavior.
tests/Unit/Repositories/SiteRepositoryTest.php (5)
15-22: LGTM!Clear test ensuring operations fail gracefully when inventory is not loaded. This validates the repository's state management requirements.
28-67: LGTM!Comprehensive CRUD lifecycle test that validates create, find, list, and delete operations with proper assertions at each step. The test also verifies edge cases like finding non-existent domains and deleting non-existent sites.
69-81: LGTM!Proper validation of duplicate domain prevention, ensuring data integrity in the repository.
87-128: LGTM!Excellent parameterized test covering various malformed data scenarios. The test validates that hydration handles missing fields, wrong types, and mixed arrays gracefully with sensible defaults.
134-145: LGTM!Good edge case coverage for initialization when the
siteskey is missing from inventory.tests/Fixtures/TestConsoleCommand.php (1)
34-45: LGTM!The constructor correctly reflects BaseCommand's updated signature with the new
ProcessServiceandSiteRepositorydependencies. Parameter ordering aligns with the parent class.app/Contracts/BaseCommand.php (1)
38-54: LGTM!The constructor is well-organized into logical sections (Framework, Base services, Servers & sites). The new
ProcessServiceandSiteRepositorydependencies are properly injected.
- Changed timeout parameter from ?float to float in ProcessService::run() - Removed null coalescing operator since parameter cannot be null - Fixed test to omit parameter instead of passing null when testing default behavior - All existing usages already rely on default value without passing null
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @lucianvacaroiu. * https://github.com/bigpixelrocket/deployer-php/pull/38#issuecomment-3393662882 The following files were modified: * `app/Contracts/BaseCommand.php` * `app/DTOs/SiteDTO.php` * `app/Repositories/SiteRepository.php` * `app/Services/ProcessService.php` * `app/Services/VersionService.php` * `tests/Fixtures/TestConsoleCommand.php` * `tests/TestHelpers.php`
Docstrings generation was requested by @lucianvacaroiu. * https://github.com/bigpixelrocket/deployer-php/pull/38#issuecomment-3393662882 The following files were modified: * `app/Contracts/BaseCommand.php` * `app/DTOs/SiteDTO.php` * `app/Repositories/SiteRepository.php` * `app/Services/ProcessService.php` * `app/Services/VersionService.php` * `tests/Fixtures/TestConsoleCommand.php` * `tests/TestHelpers.php` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Summary by CodeRabbit
New Features
Refactor
Tests