refactor: symfony app and pest migration - #17
Conversation
Adds Symfony components for CLI bootstrapping and Pest for modern testing. Enables app refactor and testing migration.
Introduces BaseCommand contract and SymfonyApp kernel for modular CLI. Updates HelloCommand and entrypoint. Removes obsolete App.php.\n\nMotivation: Standardizes command structure for extensibility.
Configures Pest, adds shared TestCase and helpers, updates SymfonyApp integration test. Removes legacy AppTest.\n\nMotivation: Simplifies test syntax and reduces boilerplate for better maintainability.
Enforces code architecture rules and tests BaseCommand. Refines EnvService and InventoryService tests for Pest and refactored logic (e.g., consolidated overlaps).\n\nMotivation: Ensures architectural integrity and full coverage post-refactor.
Move service and contract tests into dedicated subdirectories matching the app/ folder layout for better organization and discoverability. Update namespaces and require paths in BaseCommandTest.php to reflect the new location under Contracts/. The HelloCommandTest.php integration test rename to Console/ was part of aligning integration tests with the Console/ structure.
Improved environment variable handling by storing originals in beforeEach and restoring them in afterEach, preventing pollution across tests.
Standardized import placement at the file top for better PHP conventions and added minor formatting.
|
Caution Review failedThe pull request is closed. WalkthroughRemoves the static App facade and replaces bootstrap with DI via Container. Adds constructor-injected SymfonyApp and a new BaseCommand for console commands. Updates HelloCommand, EnvService messaging, InventoryService initialization/status, editorconfig, docs/rules, composer/dev test setup, extensive test fixtures/helpers, and deletes App integration tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant bin as bin/deployer
participant C as Container
participant A as SymfonyApp
participant S as Symfony Console
User->>bin: execute deployer
bin->>C: new Container()
bin->>C: build(SymfonyApp::class)
C-->>bin: SymfonyApp instance
bin->>A: run()
A->>S: register commands (container builds each command)
S-->>User: list / execute commands
sequenceDiagram
autonumber
actor User
participant S as Symfony Console
participant BC as BaseCommand
participant ENV as EnvService
participant INV as InventoryService
User->>S: run "hello"
S->>BC: initialize(input, output)
BC->>ENV: inspect/load env status
BC->>INV: getInventoryFileStatus()
BC-->>S: render header, statuses, command output (honors quiet)
S-->>User: stdout + exit code
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 (3)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/SymfonyApp.php (1)
94-101: Update architecture guidelines to use Container::build() for production object instantiation
NoApp::build()calls remain and all production code now usesContainer::build(). Adjust the documentation/policy to reflectContainer::build()as the canonical instantiation method.
🧹 Nitpick comments (13)
tests/TestCase.php (1)
9-12: Add a concise class DocBlock per repo guidelines.Tests fall under {app,tests}/** and should include minimal DocBlocks for classes. Add a brief description to align with the documented standards. As per coding guidelines.
abstract class TestCase extends BaseTestCase { - // + /** + * Base PHPUnit TestCase used by Pest tests that need fixtures/setup helpers. + */ + // }tests/Unit/ArchitectureTest.php (3)
5-5: Import Symfony classes instead of using FQCNs in expectations.Follow the rule “Always add use statements instead of fully qualified class names” for tests too. As per coding guidelines.
use Bigpixelrocket\DeployerPHP\Contracts\BaseCommand; +use Symfony\Component\Console\Attribute\AsCommand; +use Symfony\Component\Console\Command\Command as SymfonyCommand;
18-23: Use imported class; drop FQCN.Replace the FQCN with the imported alias.
arch('base command contract', function () { expect(BaseCommand::class) ->toBeAbstract() - ->toExtend(\Symfony\Component\Console\Command\Command::class) + ->toExtend(SymfonyCommand::class) ->toHaveConstructor(); });
25-29: Narrow the AsCommand assertion to only concrete command classes.Current check targets every class in the Console namespace; this will fail if non‑command or abstract classes live there. Filter by suffix and exclude abstract classes to avoid false positives.
arch('commands expose Symfony metadata', function () { - expect('Bigpixelrocket\\DeployerPHP\\Console\\') - ->classes() - ->toHaveAttribute(\Symfony\Component\Console\Attribute\AsCommand::class); + expect('Bigpixelrocket\\DeployerPHP\\Console\\') + ->classes() + ->toHaveSuffix('Command') + ->not->toBeAbstract() + ->toHaveAttribute(AsCommand::class); });app/Services/InventoryService.php (1)
42-48: Option: make inventory path injectable for easier testing and flexibility.Allow passing a project root/path; default to getcwd() to preserve behavior. Reduces reliance on CWD and simplifies integration tests.
- public function __construct( - private readonly Filesystem $filesystem, - ) { - $this->inventoryPath = rtrim((string) getcwd(), '/').'/.deployer/inventory.yml'; + public function __construct( + private readonly Filesystem $filesystem, + ?string $projectRoot = null, + ) { + $root = $projectRoot ?? (string) getcwd(); + $this->inventoryPath = rtrim($root, '/').'/.deployer/inventory.yml'; $this->inventoryDir = dirname($this->inventoryPath); $this->initializeInventoryFile(); }tests/TestHelpers.php (2)
5-5: Prefer imports over FQCNs in helpers.Use imports for EnvService, InventoryService, and Dotenv to keep consistency with rules.
use Symfony\Component\Filesystem\Filesystem; +use Symfony\Component\Dotenv\Dotenv; +use Bigpixelrocket\DeployerPHP\Services\EnvService; +use Bigpixelrocket\DeployerPHP\Services\InventoryService;- function mockEnvService(bool $hasFile = true): \Bigpixelrocket\DeployerPHP\Services\EnvService + function mockEnvService(bool $hasFile = true): EnvService { $content = $hasFile ? 'API_KEY=test_value' : ''; - return new \Bigpixelrocket\DeployerPHP\Services\EnvService(mockFilesystem($hasFile, $content), new \Symfony\Component\Dotenv\Dotenv()); + return new EnvService(mockFilesystem($hasFile, $content), new Dotenv()); }- function mockInventoryService(bool $hasFile = true): \Bigpixelrocket\DeployerPHP\Services\InventoryService + function mockInventoryService(bool $hasFile = true): InventoryService { $content = $hasFile ? 'servers:' . PHP_EOL . ' web1:' . PHP_EOL . ' host: example.com' : ''; - return new \Bigpixelrocket\DeployerPHP\Services\InventoryService(mockFilesystem($hasFile, $content)); + return new InventoryService(mockFilesystem($hasFile, $content)); }Also applies to: 99-107, 110-118
80-86: Optional: silence PHPMD unused-parameter warnings in tests.Harmless in tests, but if PHPMD runs on them, unset the params locally.
public function mkdir($dirs, int $mode = 0777): void { + unset($dirs, $mode); if ($this->throwOnMkdir) { throw new \Exception('Permission denied'); } $this->dirExists = true; }.cursor/rules/01-architecture.mdc (1)
21-29: Container-first rule looks good; clarify it supersedes App::build docs.Since this PR moves to $container->build(...), add an explicit note that this replaces any prior “App::build()” guidance to avoid drift across docs.
Use `$container->build(ClassName::class)` for all object creation instead of `new ClassName()`. +This supersedes any previous guidance referring to `App::build(...)`.app/Contracts/BaseCommand.php (2)
30-49: Initialize IO in initialize() and respect --quiet; add opt-out for preambleSet up SymfonyStyle earlier so subclasses can use $this->io in initialize()/interact(), and skip the banner in quiet mode. Also expose a toggle to opt out of the preamble per-command.
Apply:
abstract class BaseCommand extends Command { protected SymfonyStyle $io; + /** Toggle to print the environment/inventory preamble. */ + protected bool $showPreamble = true; public function __construct( protected readonly Container $container, protected readonly EnvService $env, protected readonly InventoryService $inventory, ) { parent::__construct(); } + /** + * Initialize IO early so subclasses can use $this->io in initialize()/interact(). + */ + protected function initialize(InputInterface $input, OutputInterface $output): void + { + parent::initialize($input, $output); + $this->io = new SymfonyStyle($input, $output); + } + /** * The main execution method in Symfony commands. */ protected function execute(InputInterface $input, OutputInterface $output): int { - $this->io = new SymfonyStyle($input, $output); + if ($output->isQuiet() || !$this->showPreamble) { + return Command::SUCCESS; + } $envStatus = $this->env->getEnvFileStatus(); $inventoryStatus = $this->inventory->getInventoryFileStatus(); $this->hr(); $this->writeln([Based on learnings
60-73: Consider widening helper visibility for reuse in subclassesIf child commands may want consistent separators/blocks, make writeln()/hr() protected.
- private function writeln(array $lines): void + protected function writeln(array $lines): void ... - private function hr(): void + protected function hr(): voidapp/Console/HelloCommand.php (1)
19-26: Preserve parent exit code and honor --quiet before emitting greetingAvoid hardcoding SUCCESS so future BaseCommand return codes propagate; skip output when quiet.
protected function execute(InputInterface $input, OutputInterface $output): int { - parent::execute($input, $output); + $exit = parent::execute($input, $output); + + if ($output->isQuiet()) { + return $exit; + } $user = $this->env->get(['USER', 'USERNAME'], false) ?? 'there'; $this->io->text("Hello {$user}!"); - return Command::SUCCESS; + return $exit; }tests/Unit/Contracts/BaseCommandTest.php (1)
50-77: LGTM; add a quiet-mode scenarioSolid coverage and fixtures. Consider adding a dataset that runs with OutputInterface set to quiet (CommandTester->execute with 'verbosity' => OutputInterface::VERBOSITY_QUIET) to assert the preamble and success message are suppressed once the quiet handling is added.
app/SymfonyApp.php (1)
50-57: Skip banner when running with --quietRespect Console verbosity. This prevents banner noise in scripted contexts.
public function doRun(InputInterface $input, OutputInterface $output): int { $this->io = new SymfonyStyle($input, $output); - $this->displayBanner(); + if (!$output->isQuiet()) { + $this->displayBanner(); + } return parent::doRun($input, $output); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
.cursor/commands/review-branch.md(1 hunks).cursor/commands/review-diff.md(1 hunks).cursor/rules/01-architecture.mdc(2 hunks).editorconfig(1 hunks)app/App.php(0 hunks)app/Console/HelloCommand.php(1 hunks)app/Contracts/BaseCommand.php(1 hunks)app/Services/EnvService.php(1 hunks)app/Services/InventoryService.php(2 hunks)app/SymfonyApp.php(4 hunks)bin/deployer(2 hunks)composer.json(2 hunks)tests/Fixtures/ContainerFixtures.php(3 hunks)tests/Integration/AppTest.php(0 hunks)tests/Integration/Console/HelloCommandTest.php(2 hunks)tests/Integration/SymfonyAppTest.php(1 hunks)tests/Pest.php(1 hunks)tests/TestCase.php(1 hunks)tests/TestHelpers.php(2 hunks)tests/Unit/ArchitectureTest.php(1 hunks)tests/Unit/Contracts/BaseCommandTest.php(1 hunks)tests/Unit/Services/EnvServiceTest.php(2 hunks)tests/Unit/Services/InventoryServiceTest.php(7 hunks)
💤 Files with no reviewable changes (2)
- app/App.php
- tests/Integration/AppTest.php
🧰 Additional context used
📓 Path-based instructions (6)
**/*.php
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
Organize code by grouping related functions into comment-separated sections; prefer alphabetical ordering when it doesn’t conflict with logical grouping
Files:
app/Services/EnvService.phptests/Pest.phptests/Integration/SymfonyAppTest.phptests/TestCase.phpapp/Services/InventoryService.phpapp/SymfonyApp.phptests/TestHelpers.phptests/Unit/Contracts/BaseCommandTest.phptests/Unit/Services/EnvServiceTest.phptests/Fixtures/ContainerFixtures.phptests/Integration/Console/HelloCommandTest.phpapp/Console/HelloCommand.phptests/Unit/ArchitectureTest.phptests/Unit/Services/InventoryServiceTest.phpapp/Contracts/BaseCommand.php
{app,tests}/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
{app,tests}/**/*.php: Follow PSR-12, declare strict_types, and prefer PHP 8.x features (unions, match, attributes, readonly) in PHP files
Always use import use statements instead of fully qualified class names
All methods must declare explicit return types and use proper generics in types/docblocks (e.g., Collection<int, User>)
Add concise DocBlocks for classes and functions (description, params, return types); use sectioned comments as visual separators; avoid obvious/stale comments; use the mandated section header format
Files:
app/Services/EnvService.phptests/Pest.phptests/Integration/SymfonyAppTest.phptests/TestCase.phpapp/Services/InventoryService.phpapp/SymfonyApp.phptests/TestHelpers.phptests/Unit/Contracts/BaseCommandTest.phptests/Unit/Services/EnvServiceTest.phptests/Fixtures/ContainerFixtures.phptests/Integration/Console/HelloCommandTest.phpapp/Console/HelloCommand.phptests/Unit/ArchitectureTest.phptests/Unit/Services/InventoryServiceTest.phpapp/Contracts/BaseCommand.php
app/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
app/**/*.php: Use dependency injection instead of manually resolving/instantiating classes in production code
Prefer Symfony utility classes (e.g., Filesystem, Process) over native PHP functions to aid testability
All object creation in production code must use App::build() (exceptions: value objects, DTOs, pure data structures)
Files:
app/Services/EnvService.phpapp/Services/InventoryService.phpapp/SymfonyApp.phpapp/Console/HelloCommand.phpapp/Contracts/BaseCommand.php
tests/**
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
Do not write or run tests unless specifically instructed
Files:
tests/Pest.phptests/Integration/SymfonyAppTest.phptests/TestCase.phptests/TestHelpers.phptests/Unit/Contracts/BaseCommandTest.phptests/Unit/Services/EnvServiceTest.phptests/Fixtures/ContainerFixtures.phptests/Integration/Console/HelloCommandTest.phptests/Unit/ArchitectureTest.phptests/Unit/Services/InventoryServiceTest.php
tests/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/02-tests.mdc)
tests/**/*.php: Use Pest exclusively for PHP tests with it() syntax.
Keep each test file under 1.8x the size of the source code it tests.
Test core business logic only; do not test the framework.
Use dataset-driven testing (->with([...])) for multiple scenarios.
Consolidate related assertions using expect(...)->and(...).
Mock only external dependencies; avoid mocking internal logic.
Do not write performance tests unless performance is the primary concern.
Do not consolidate tests when covering different public methods.
Do not consolidate exception-flow tests with normal-flow tests.
Do not consolidate when setup requirements differ.
Do not consolidate when testing distinct business logic.
Follow the AAA pattern (Arrange, Act, Assert) in tests.
For exception tests, use a combined // ACT & ASSERT section when the act triggers the assertion.
Organize tests using describe() blocks, beforeEach() setup, and extracted helpers/traits for DRY.
Avoid type-only and generic assertions (e.g., toBeInstanceOf, toBeArray, not->toBeNull, expect(true)).
Avoid sleep(...); use time mocking instead.
Write meaningful assertions tied to behavior and outcomes (e.g., checking config values, validator results, and mock expectations).
Unit tests must mock all external dependencies, test single units in isolation, and complete in milliseconds.
Integration tests should use real file operations and external processes; cover CLI commands and full workflows.
Ignore PHPStan issues in tests; prioritize test functionality over static analysis compliance.
Avoid excessive PHPDoc in tests added solely to appease types.Direct instantiation is acceptable in tests; using a local Container for isolation is allowed
Files:
tests/Pest.phptests/Integration/SymfonyAppTest.phptests/TestCase.phptests/TestHelpers.phptests/Unit/Contracts/BaseCommandTest.phptests/Unit/Services/EnvServiceTest.phptests/Fixtures/ContainerFixtures.phptests/Integration/Console/HelloCommandTest.phptests/Unit/ArchitectureTest.phptests/Unit/Services/InventoryServiceTest.php
{composer.json,package.json}
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
Check composer.json and package.json for installed packages before starting any task
Files:
composer.json
🧠 Learnings (15)
📓 Common learnings
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-27T13:20:25.351Z
Learning: Applies to app/**/*.php : Use dependency injection instead of manually resolving/instantiating classes in production code
📚 Learning: 2025-09-24T07:14:11.629Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-09-24T07:14:11.629Z
Learning: Applies to tests/**/*.php : Use Pest exclusively for PHP tests with it() syntax.
Applied to files:
tests/Pest.phpcomposer.json
📚 Learning: 2025-09-24T07:14:11.629Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-09-24T07:14:11.629Z
Learning: Applies to tests/**/*.php : Organize tests using describe() blocks, beforeEach() setup, and extracted helpers/traits for DRY.
Applied to files:
tests/Pest.php
📚 Learning: 2025-09-24T07:14:11.639Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-09-24T07:14:11.639Z
Learning: Applies to tests/**/*.php : Avoid excessive PHPDoc in tests added solely to appease types.
Applied to files:
tests/Pest.php
📚 Learning: 2025-09-24T07:14:11.639Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-09-24T07:14:11.639Z
Learning: Applies to tests/**/*.php : Consolidate related assertions using expect(...)->and(...).
Applied to files:
tests/Pest.php
📚 Learning: 2025-09-24T07:14:11.629Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-09-24T07:14:11.629Z
Learning: Applies to tests/**/*.php : Avoid type-only and generic assertions (e.g., toBeInstanceOf, toBeArray, not->toBeNull, expect(true)).
Applied to files:
tests/Pest.phptests/Unit/ArchitectureTest.php
📚 Learning: 2025-09-24T07:14:11.639Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-09-24T07:14:11.639Z
Learning: Applies to tests/**/*.php : Write meaningful assertions tied to behavior and outcomes (e.g., checking config values, validator results, and mock expectations).
Applied to files:
tests/Pest.php
📚 Learning: 2025-09-24T07:14:11.639Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-09-24T07:14:11.639Z
Learning: Applies to tests/**/*.php : Follow the AAA pattern (Arrange, Act, Assert) in tests.
Applied to files:
tests/Pest.php
📚 Learning: 2025-09-27T13:20:25.351Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-27T13:20:25.351Z
Learning: Only Commands perform console I/O; use SymfonyStyle consistently; Services return data/exceptions for Commands to render
Applied to files:
app/SymfonyApp.phpapp/Console/HelloCommand.phpapp/Contracts/BaseCommand.php
📚 Learning: 2025-09-24T07:14:11.639Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-09-24T07:14:11.639Z
Learning: Use composer pest and vendor/bin/pest for running tests.
Applied to files:
composer.json
📚 Learning: 2025-09-27T13:20:25.351Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-27T13:20:25.351Z
Learning: Before completing a task, run rector and pint on changed PHP files and run phpstan on changed non-test PHP files only
Applied to files:
composer.json
📚 Learning: 2025-09-27T13:20:25.351Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-27T13:20:25.351Z
Learning: Applies to tests/**/*.php : Direct instantiation is acceptable in tests; using a local Container for isolation is allowed
Applied to files:
tests/Fixtures/ContainerFixtures.php.cursor/rules/01-architecture.mdc
📚 Learning: 2025-09-27T13:20:25.351Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-27T13:20:25.351Z
Learning: Applies to app/**/*.php : Use dependency injection instead of manually resolving/instantiating classes in production code
Applied to files:
bin/deployer.cursor/rules/01-architecture.mdc
📚 Learning: 2025-09-27T13:20:25.351Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-27T13:20:25.351Z
Learning: Applies to app/**/*.php : All object creation in production code must use App::build() (exceptions: value objects, DTOs, pure data structures)
Applied to files:
.cursor/rules/01-architecture.mdc
📚 Learning: 2025-09-24T07:14:11.639Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-09-24T07:14:11.639Z
Learning: Applies to tests/**/*.php : Integration tests should use real file operations and external processes; cover CLI commands and full workflows.
Applied to files:
tests/Unit/Services/InventoryServiceTest.php
🧬 Code graph analysis (9)
app/Services/InventoryService.php (2)
tests/TestHelpers.php (4)
__construct(39-50)exists(52-69)mkdir(80-86)dumpFile(88-94)app/Services/EnvService.php (1)
__construct(20-25)
app/SymfonyApp.php (2)
app/Services/VersionService.php (3)
VersionService(18-171)__construct(20-25)getVersion(30-46)app/Container.php (2)
Container(23-227)build(42-62)
tests/TestHelpers.php (2)
app/Services/EnvService.php (2)
__construct(20-25)EnvService(13-105)app/Services/InventoryService.php (2)
__construct(42-48)InventoryService(35-313)
tests/Unit/Contracts/BaseCommandTest.php (4)
app/Contracts/BaseCommand.php (1)
BaseCommand(15-74)app/Services/EnvService.php (1)
EnvService(13-105)app/Services/InventoryService.php (1)
InventoryService(35-313)tests/TestHelpers.php (2)
mockEnvService(103-107)mockInventoryService(114-118)
tests/Integration/Console/HelloCommandTest.php (1)
tests/TestHelpers.php (1)
setEnv(11-21)
app/Console/HelloCommand.php (2)
app/Contracts/BaseCommand.php (2)
BaseCommand(15-74)execute(30-49)app/Services/EnvService.php (1)
get(36-60)
tests/Unit/ArchitectureTest.php (2)
app/Contracts/BaseCommand.php (1)
BaseCommand(15-74)app/Console/HelloCommand.php (1)
AsCommand(13-28)
tests/Unit/Services/InventoryServiceTest.php (2)
tests/TestHelpers.php (1)
mockFilesystem(28-96)app/Services/InventoryService.php (5)
InventoryService(35-313)getAll(82-85)has(90-96)set(57-64)getInventoryFileStatus(113-116)
app/Contracts/BaseCommand.php (5)
app/Container.php (1)
Container(23-227)app/Services/EnvService.php (2)
EnvService(13-105)getEnvFileStatus(65-68)app/Services/InventoryService.php (2)
InventoryService(35-313)getInventoryFileStatus(113-116)app/SymfonyApp.php (1)
__construct(22-33)app/Console/HelloCommand.php (1)
execute(19-27)
🪛 PHPMD (2.15.0)
tests/TestHelpers.php
40-40: Avoid unused parameters such as '$initialExists'. (undefined)
(UnusedFormalParameter)
41-41: Avoid unused parameters such as '$initialContent'. (undefined)
(UnusedFormalParameter)
80-80: Avoid unused parameters such as '$dirs'. (undefined)
(UnusedFormalParameter)
80-80: Avoid unused parameters such as '$mode'. (undefined)
(UnusedFormalParameter)
88-88: Avoid unused parameters such as '$filename'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (10)
.editorconfig (1)
3-18: Standardized editor defaults look solidUTF-8, LF endings, and the per-language indent overrides will keep the repo consistent across editors. Nicely done.
.cursor/commands/review-branch.md (1)
1-5: Instruction update aligns with our review workflowClear reminder to audit branches against the rules before acting; no concerns here.
app/Services/EnvService.php (1)
97-100: Status copy tweak keeps messaging consistentThe revised wording still communicates a successful .env read without altering behavior. Looks good.
.cursor/commands/review-diff.md (1)
1-5: Diff-review checklist addition is helpfulThe directive mirrors our broader rules and sets expectations for thorough diff analysis. No changes requested.
composer.json (2)
30-33: Pest architecture plugin addition makes senseBringing in
pestphp/pest-plugin-archcomplements the new Pest tooling and keeps the dependency set consistent.
40-42: Autoload-dev classmap switch looks appropriateClassmapping
tests/keeps helper classes discoverable for Pest without saddling production autoload; no issues spotted.app/Services/InventoryService.php (1)
265-287: readFile() usage is safe – composer.json requires symfony/filesystem ^7.3
composer.json’s ^7.3 constraint ensures Filesystem::readFile (added in 7.1) is available..cursor/rules/01-architecture.mdc (1)
55-67: No legacy references toApp::build/App::rundetected
Thergsearch returned no matches; the legacy purge appears complete.tests/Unit/Services/InventoryServiceTest.php (2)
235-245: LGTM on YAML edge-case handlingGood assertions for empty/invalid YAML scenarios; behavior matches InventoryService’s null/array fallbacks.
251-269: Error-path coverage is strongNice coverage for mkdir/write failures with clear message matching.
- Move status display to initialize() method for early IO setup - Add isQuiet flag and checks to suppress output in quiet mode - Introduce text() wrapper method for simple output - Update hr() and writeln() to respect quiet mode - Adjust HelloCommand to use success() for greeting - Add tests for quiet mode suppression in HelloCommand and BaseCommand - Minor test refactors for short class names
- Document quiet mode philosophy and output method hierarchy - Cover verbosity level management and content guidelines - Include command layer patterns and performance considerations - Ensure rules align with recent BaseCommand quiet mode implementation
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.cursor/rules/03-commands.mdc(1 hunks)app/Console/HelloCommand.php(1 hunks)app/Contracts/BaseCommand.php(1 hunks)tests/Integration/Console/HelloCommandTest.php(2 hunks)tests/TestHelpers.php(3 hunks)tests/Unit/ArchitectureTest.php(1 hunks)tests/Unit/Contracts/BaseCommandTest.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .cursor/rules/03-commands.mdc
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Unit/ArchitectureTest.php
🧰 Additional context used
📓 Path-based instructions (5)
**/*.php
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
Organize code by grouping related functions into comment-separated sections; prefer alphabetical ordering when it doesn’t conflict with logical grouping
Files:
tests/Unit/Contracts/BaseCommandTest.phpapp/Contracts/BaseCommand.phptests/Integration/Console/HelloCommandTest.phpapp/Console/HelloCommand.phptests/TestHelpers.php
tests/**
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
Do not write or run tests unless specifically instructed
Files:
tests/Unit/Contracts/BaseCommandTest.phptests/Integration/Console/HelloCommandTest.phptests/TestHelpers.php
tests/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/02-tests.mdc)
tests/**/*.php: Use Pest exclusively for PHP tests with it() syntax.
Keep each test file under 1.8x the size of the source code it tests.
Test core business logic only; do not test the framework.
Use dataset-driven testing (->with([...])) for multiple scenarios.
Consolidate related assertions using expect(...)->and(...).
Mock only external dependencies; avoid mocking internal logic.
Do not write performance tests unless performance is the primary concern.
Do not consolidate tests when covering different public methods.
Do not consolidate exception-flow tests with normal-flow tests.
Do not consolidate when setup requirements differ.
Do not consolidate when testing distinct business logic.
Follow the AAA pattern (Arrange, Act, Assert) in tests.
For exception tests, use a combined // ACT & ASSERT section when the act triggers the assertion.
Organize tests using describe() blocks, beforeEach() setup, and extracted helpers/traits for DRY.
Avoid type-only and generic assertions (e.g., toBeInstanceOf, toBeArray, not->toBeNull, expect(true)).
Avoid sleep(...); use time mocking instead.
Write meaningful assertions tied to behavior and outcomes (e.g., checking config values, validator results, and mock expectations).
Unit tests must mock all external dependencies, test single units in isolation, and complete in milliseconds.
Integration tests should use real file operations and external processes; cover CLI commands and full workflows.
Ignore PHPStan issues in tests; prioritize test functionality over static analysis compliance.
Avoid excessive PHPDoc in tests added solely to appease types.Direct instantiation is acceptable in tests; using a local Container for isolation is allowed
Files:
tests/Unit/Contracts/BaseCommandTest.phptests/Integration/Console/HelloCommandTest.phptests/TestHelpers.php
{app,tests}/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
{app,tests}/**/*.php: Follow PSR-12, declare strict_types, and prefer PHP 8.x features (unions, match, attributes, readonly) in PHP files
Always use import use statements instead of fully qualified class names
All methods must declare explicit return types and use proper generics in types/docblocks (e.g., Collection<int, User>)
Add concise DocBlocks for classes and functions (description, params, return types); use sectioned comments as visual separators; avoid obvious/stale comments; use the mandated section header format
Files:
tests/Unit/Contracts/BaseCommandTest.phpapp/Contracts/BaseCommand.phptests/Integration/Console/HelloCommandTest.phpapp/Console/HelloCommand.phptests/TestHelpers.php
app/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
app/**/*.php: Use dependency injection instead of manually resolving/instantiating classes in production code
Prefer Symfony utility classes (e.g., Filesystem, Process) over native PHP functions to aid testability
All object creation in production code must use App::build() (exceptions: value objects, DTOs, pure data structures)
Files:
app/Contracts/BaseCommand.phpapp/Console/HelloCommand.php
🧠 Learnings (2)
📚 Learning: 2025-09-27T13:20:25.351Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-27T13:20:25.351Z
Learning: Only Commands perform console I/O; use SymfonyStyle consistently; Services return data/exceptions for Commands to render
Applied to files:
app/Contracts/BaseCommand.phpapp/Console/HelloCommand.php
📚 Learning: 2025-09-24T07:14:11.639Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-09-24T07:14:11.639Z
Learning: Applies to tests/**/*.php : Integration tests should use real file operations and external processes; cover CLI commands and full workflows.
Applied to files:
tests/TestHelpers.php
🧬 Code graph analysis (5)
tests/Unit/Contracts/BaseCommandTest.php (4)
app/Contracts/BaseCommand.php (4)
BaseCommand(15-113)__construct(21-27)execute(56-59)text(87-97)app/Services/EnvService.php (1)
EnvService(13-105)app/Services/InventoryService.php (1)
InventoryService(35-313)tests/TestHelpers.php (3)
__construct(42-53)mockEnvService(109-113)mockInventoryService(120-124)
app/Contracts/BaseCommand.php (4)
app/Container.php (1)
Container(23-227)app/Services/EnvService.php (2)
EnvService(13-105)getEnvFileStatus(65-68)app/Services/InventoryService.php (2)
InventoryService(35-313)getInventoryFileStatus(113-116)tests/Unit/Contracts/BaseCommandTest.php (2)
__construct(24-31)execute(38-43)
tests/Integration/Console/HelloCommandTest.php (2)
tests/TestHelpers.php (1)
setEnv(14-24)app/Console/HelloCommand.php (1)
execute(19-26)
app/Console/HelloCommand.php (2)
app/Contracts/BaseCommand.php (2)
BaseCommand(15-113)execute(56-59)app/Services/EnvService.php (1)
get(36-60)
tests/TestHelpers.php (2)
app/Services/EnvService.php (2)
EnvService(13-105)__construct(20-25)app/Services/InventoryService.php (2)
InventoryService(35-313)__construct(42-48)
🪛 PHPMD (2.15.0)
app/Contracts/BaseCommand.php
56-56: Avoid unused parameters such as '$input'. (undefined)
(UnusedFormalParameter)
56-56: Avoid unused parameters such as '$output'. (undefined)
(UnusedFormalParameter)
app/Console/HelloCommand.php
19-19: Avoid unused parameters such as '$input'. (undefined)
(UnusedFormalParameter)
19-19: Avoid unused parameters such as '$output'. (undefined)
(UnusedFormalParameter)
tests/TestHelpers.php
43-43: Avoid unused parameters such as '$initialExists'. (undefined)
(UnusedFormalParameter)
44-44: Avoid unused parameters such as '$initialContent'. (undefined)
(UnusedFormalParameter)
94-94: Avoid unused parameters such as '$filename'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (2)
tests/TestHelpers.php (1)
71-100: Fix mock filesystem path handlingLine 71:
exists()currently returns true for any path once'.deployer/inventory.yml'is seeded, so tests will happily pass even if callers look in the wrong location. Lines 80-100:readFile()/dumpFile()ignore the requested$filename, which is why the newmockEnvService()ends up reading inventory content instead of.env. This regression reintroduces the false positives I flagged earlier; we still have to store and resolve content by the normalized path that was asked for. Please normalize the incoming path, key the in-memory map by that value, and rely on it consistently so the helpers behave like the real filesystem.- private array $fileSystem = []; - private bool $dirExists = true; + private array $fileSystem = []; + private bool $dirExists = false; @@ - if ($this->initialExists) { - $this->fileSystem['.deployer/inventory.yml'] = $this->initialContent; - } - $this->dirExists = !$this->throwOnMkdir; + if ($this->initialExists) { + $inventoryPath = $this->normalizePath('.deployer/inventory.yml'); + $this->fileSystem[$inventoryPath] = $this->initialContent; + $this->dirExists = true; + } @@ + private function normalizePath(string $path): string + { + $normalized = str_replace('\\', '/', $path); + $cwd = str_replace('\\', '/', (string) getcwd()); + + if ($cwd !== '' && str_starts_with($normalized, $cwd . '/')) { + $normalized = substr($normalized, strlen($cwd) + 1); + } + + $normalized = preg_replace('#^(\./)+#', '', $normalized) ?? $normalized; + return rtrim($normalized, '/'); + } + public function exists(string|iterable $files): bool { if (is_iterable($files)) { @@ - // Handle directory checks - if (str_ends_with($files, '.deployer')) { - return $this->dirExists; - } - - return isset($this->fileSystem[$files]) || isset($this->fileSystem['.deployer/inventory.yml']); + $normalized = $this->normalizePath($files); + + if ($normalized === '.deployer') { + return $this->dirExists; + } + + return isset($this->fileSystem[$normalized]); } @@ - return $this->fileSystem['.deployer/inventory.yml'] ?? $this->initialContent; + $normalized = $this->normalizePath($filename); + + if (isset($this->fileSystem[$normalized])) { + return $this->fileSystem[$normalized]; + } + + return $this->initialContent; } @@ - $this->fileSystem['.deployer/inventory.yml'] = $content; + $normalized = $this->normalizePath($filename); + $this->fileSystem[$normalized] = $content; }tests/Integration/Console/HelloCommandTest.php (1)
31-31: Preserve falsy environment values when capturing originalsLine 31:
getenv($key) ?: nullstill collapses legitimate falsy values (e.g.'0','') tonull, so those values are never restored inafterEach. Please capture the raw getenv result and only translate strictfalsetonullbefore caching.- $this->originals[$key] = getenv($key) ?: null; + $value = getenv($key); + $this->originals[$key] = $value === false ? null : $value;
- Add initialPath parameter to support .env and inventory.yml - Implement path normalization and target key resolution - Use IOException for filesystem errors - Update EnvServiceTest to use new mock parameters - Fix getenv handling in HelloCommandTest beforeEach
1 participant
Summary by CodeRabbit
New Features
Refactor
Documentation
Style/Chores
Tests