feat: custom env inventory paths - #19
Conversation
- Document stateful vs stateless service patterns with lazy loading - Clarify test rules enforcement in architecture guidelines - Update banner text in README - Add inventory.yml to .gitignore and reorganize entries
Add --env and --inventory CLI options to all commands for specifying custom configuration file paths. Changes: - BaseCommand: Add --env and --inventory options in configure() - BaseCommand: Initialize services with custom paths in initialize() - BaseCommand: Display env and inventory status in execute() - EnvService: Implement lazy loading with setCustomPath() and loadEnvFile() - InventoryService: Implement lazy loading with setCustomPath() and loadInventoryFile() - HelloCommand: Call parent::execute() to display status - Tests: Update all service and command tests for lazy loading behavior Breaking change: Services now require explicit initialization via load methods before use. This enables custom path support and proper error handling.
- Redesign banner with decorative borders and updated tagline - Add quiet mode check to suppress banner in automated environments - Update tests to reflect new banner content
|
Caution Review failedThe pull request is closed. WalkthroughThis PR makes metadata/rule applicability changes, updates branding/banner text and display behavior, adds command options and status flow in BaseCommand, introduces explicit load/custom-path APIs for EnvService and InventoryService, adjusts HelloCommand to call parent execute, updates .gitignore, and revises related tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as SymfonyApp
participant Cmd as BaseCommand
participant Env as EnvService
participant Inv as InventoryService
participant Sub as HelloCommand
User->>CLI: Run command [--env=path] [--inventory=path]
CLI->>CLI: doRun()
alt output not quiet
CLI->>CLI: displayBanner()
else quiet
note over CLI: Banner suppressed
end
CLI->>Cmd: initialize(input, output)
Cmd->>Env: setCustomPath(envPath)
Cmd->>Inv: setCustomPath(inventoryPath)
Cmd->>Env: loadEnvFile()
Cmd->>Inv: loadInventoryFile()
CLI->>Cmd: execute(input, output)
Cmd->>Cmd: write Env/Inventory status lines (if not quiet)
Cmd-->>CLI: return 0
%% subclass flow
CLI->>Sub: execute()
Sub->>Cmd: parent::execute()
Sub->>Sub: subclass-specific output
Sub-->>CLI: return 0
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 (2)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Services/InventoryService.php (1)
27-30: Remove stale documentation referencing deletedhas()method.The example references
has(), which was removed in this refactor. Update the example to demonstrate the newget()with default parameter pattern instead, or remove this section entirely.Apply this diff to update the example:
-// Check if path exists -if ($inventory->has('servers.production')) { - // Path exists -} +// Check if path exists using default value +if ($inventory->get('servers.production') !== null) { + // Path exists +}
🧹 Nitpick comments (8)
app/Contracts/BaseCommand.php (4)
41-54: Option names are fine; consider future-proofing against collisions.If this package is used inside a full Symfony app,
--envhas historically existed. Not a blocker here, but consider documenting/namespace‑prefixing options (e.g.,--env-path,--inventory-path) to avoid ambiguity in mixed environments.
69-73: Normalize empty string to null for custom env path.An empty
--env ""becomes''(not null), leading to an invalid path. Normalize tonullbefore passing to the service.- /** @var ?string $customEnvPath */ - $customEnvPath = $input->getOption('env'); + /** @var string|null $opt */ + $opt = $input->getOption('env'); + $customEnvPath = is_string($opt) && trim($opt) !== '' ? $opt : null; $this->env->setCustomPath($customEnvPath);
77-81: Normalize empty string to null for custom inventory path.Same rationale as for
--env.- /** @var ?string $customInventoryPath */ - $customInventoryPath = $input->getOption('inventory'); + /** @var string|null $optInv */ + $optInv = $input->getOption('inventory'); + $customInventoryPath = is_string($optInv) && trim($optInv) !== '' ? $optInv : null; $this->inventory->setCustomPath($customInventoryPath);
83-91: Silence PHPMD unused parameter warnings in execute().Parameters are required by the Command signature but unused here; either use them or suppress. Add a suppression to keep CI quiet.
- /** - * Display env and inventory statuses. - */ + /** + * Display env and inventory statuses. + * + * @SuppressWarnings(PHPMD.UnusedFormalParameter) + */ protected function execute(InputInterface $input, OutputInterface $output): intAlso applies to: 96-104
app/Services/EnvService.php (2)
105-112: Use Symfony Path helper for portability.Avoid manual
getcwd()+ string concat; preferPath::join()and normalize for cross‑platform paths. [As per coding guidelines]+use Symfony\Component\Filesystem\Path; @@ - private function getEnvPath(): string - { - return $this->envPath ?? rtrim((string) getcwd(), '/') . '/.env'; - } + private function getEnvPath(): string + { + $base = getcwd() ?: '.'; + return $this->envPath ?? Path::join($base, '.env'); + }
54-58: Import RuntimeException instead of FQCN.Follow the “use statements over FQCN” guideline and keep exception types consistent. [As per coding guidelines]
use Symfony\Component\Dotenv\Dotenv; use Symfony\Component\Filesystem\Filesystem; +use RuntimeException; @@ - throw new \RuntimeException("Missing required environment {$label}: {$list}"); + throw new RuntimeException("Missing required environment {$label}: {$list}"); @@ - } catch (\Throwable $e) { - throw new \RuntimeException("Error reading .env file from {$path}: " . $e->getMessage()); + } catch (\Throwable $e) { + throw new RuntimeException("Error reading .env file from {$path}: " . $e->getMessage()); }Also applies to: 131-133, 7-9
tests/Unit/Contracts/BaseCommandTest.php (2)
71-114: Consolidate duplicate tests.The test on lines 71-93 and the test on lines 95-114 cover nearly identical scenarios:
- Both test env file present/absent
- Both verify env status messages (one with string contains, one with regex)
- Both verify inventory status
- Both use the same dataset structure
Consolidate into a single test with the most meaningful assertions.
Apply this diff to consolidate:
- it('executes with proper status output', function (bool $hasEnvFile, string $expectedEnvMessage) { + it('displays correct env and inventory status messages', function (bool $hasEnvFile, string $expectedEnvMessage) { // ARRANGE $container = new Container(); $env = mockEnvService($hasEnvFile); $inventory = mockInventoryService(true); $command = new TestableBaseCommand($container, $env, $inventory); $tester = new CommandTester($command); // ACT $exitCode = $tester->execute([]); $output = $tester->getDisplay(); // ASSERT expect($exitCode)->toBe(Command::SUCCESS) ->and($output)->toContain('Environment:') ->and($output)->toContain('Inventory:') ->and($output)->toContain($expectedEnvMessage) ->and($output)->toContain('Reading inventory from') ->and($output)->toContain('Test command executed successfully'); })->with([ 'env file exists' => [true, 'Reading variables from'], 'no env file' => [false, 'No .env file found'], ]); - - it('displays correct env status messages for different scenarios', function (bool $hasEnvFile, string $envPattern) { - // ARRANGE - $container = new Container(); - $env = mockEnvService($hasEnvFile); - $inventory = mockInventoryService(true); - $command = new TestableBaseCommand($container, $env, $inventory); - $tester = new CommandTester($command); - - // ACT - $exitCode = $tester->execute([]); - $output = $tester->getDisplay(); - - // ASSERT - expect($exitCode)->toBe(Command::SUCCESS) - ->and($output)->toMatch($envPattern) - ->and($output)->toContain('Reading inventory from'); - })->with([ - 'env file exists' => [true, '/Reading variables from/'], - 'no env file' => [false, '/No \\.env file found/'], - ]);
206-208: Remove generic length assertion.The
strlen($output))->toBeGreaterThan(40)assertion is a generic, weak check that doesn't verify meaningful behavior. ThetoContain('╭───────')assertion already confirms the separator is present.Apply this diff to remove the weak assertion:
// ASSERT expect($output)->toContain('╭───────') - ->and(strlen($output))->toBeGreaterThan(40); + ->and($output)->not->toBe('');Alternatively, if you want to verify the full separator pattern, check for multiple color segments:
// ASSERT expect($output)->toContain('╭───────') - ->and(strlen($output))->toBeGreaterThan(40); + ->and($output)->toContain('─────────');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.cursor/rules/00-main.mdc(1 hunks).cursor/rules/01-architecture.mdc(2 hunks).cursor/rules/02-tests.mdc(1 hunks).cursor/rules/03-commands.mdc(1 hunks).gitignore(1 hunks)README.md(1 hunks)app/Console/HelloCommand.php(1 hunks)app/Contracts/BaseCommand.php(2 hunks)app/Services/EnvService.php(3 hunks)app/Services/InventoryService.php(3 hunks)app/SymfonyApp.php(2 hunks)tests/Integration/SymfonyAppTest.php(2 hunks)tests/Unit/Contracts/BaseCommandTest.php(2 hunks)tests/Unit/Services/EnvServiceTest.php(5 hunks)tests/Unit/Services/InventoryServiceTest.php(7 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.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/Services/InventoryServiceTest.phptests/Integration/SymfonyAppTest.phpapp/SymfonyApp.phpapp/Console/HelloCommand.phptests/Unit/Services/EnvServiceTest.phpapp/Services/EnvService.phpapp/Contracts/BaseCommand.phptests/Unit/Contracts/BaseCommandTest.phpapp/Services/InventoryService.php
tests/**
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
Do not write or run tests unless specifically instructed
Files:
tests/Unit/Services/InventoryServiceTest.phptests/Integration/SymfonyAppTest.phptests/Unit/Services/EnvServiceTest.phptests/Unit/Contracts/BaseCommandTest.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.In tests, instantiate the Container directly and build services via
$container->build()for isolation
Files:
tests/Unit/Services/InventoryServiceTest.phptests/Integration/SymfonyAppTest.phptests/Unit/Services/EnvServiceTest.phptests/Unit/Contracts/BaseCommandTest.php
{app,tests}/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
{app,tests}/**/*.php: Adhere to PSR-12 coding standard in all PHP files
Declare strict types (declare(strict_types=1);) in all PHP files
Prefer PHP 8.x language features (unions, match, attributes, readonly) where appropriate
Always use importusestatements instead of fully qualified class names in code
All methods must have explicit return types and include proper generics where applicable (e.g.,Collection<int, User>)
Use$container->build(Foo::class)for object creation instead ofnew Foo()except for value objects/DTOs/pure data
Add DocBlock comments with minimalist descriptions, parameters, and return types for classes and functions
Use comments as visual separators with the specified section/subheader/paragraph formatting and spacing
Always use the full section header comment format (with the correct number of dashes) and avoid simplified headers
Files:
tests/Unit/Services/InventoryServiceTest.phptests/Integration/SymfonyAppTest.phpapp/SymfonyApp.phpapp/Console/HelloCommand.phptests/Unit/Services/EnvServiceTest.phpapp/Services/EnvService.phpapp/Contracts/BaseCommand.phptests/Unit/Contracts/BaseCommandTest.phpapp/Services/InventoryService.php
app/**/*.php
📄 CodeRabbit inference engine (.cursor/rules/01-architecture.mdc)
app/**/*.php: Use dependency injection instead of manually resolving or instantiating classes in production code
Prefer Symfony utility classes (e.g., Filesystem, Process) over native PHP functions for easier mocking
Access the container via constructor injection in production code
Files:
app/SymfonyApp.phpapp/Console/HelloCommand.phpapp/Services/EnvService.phpapp/Contracts/BaseCommand.phpapp/Services/InventoryService.php
app/Console/*.php
📄 CodeRabbit inference engine (.cursor/rules/03-commands.mdc)
app/Console/*.php: All console output must respect --quiet/-q and --silent: commands should be silent when requested but must still show critical errors
Prefer SymfonyStyle high-level methods (success, info, warning, error, note, caution, table, progress) for output; they auto-honor quiet/silent and verbosity
For simple text output, use project wrapper methods writeln(), text(), and hr() from BaseCommand instead of raw SymfonyStyle methods
Do not call $this->io->writeln() or $this->io->text() directly in commands (forbidden as they bypass quiet mode)
If truly necessary, only use raw $this->io->writeln() for complex styling and guard it with a quiet check (e.g., if (!$this->isQuiet) ...)
Honor Symfony verbosity levels (-v/-vv/-vvv): gate additional details using $this->io->isVerbose()/isVeryVerbose()/isDebug()
Avoid expensive computations for debug output unless the requested verbosity justifies it (e.g., only collect when isDebug())
Structure command output progressively: essential info at normal, context at -v, detailed steps at -vv, and traces at -vvv
Files:
app/Console/HelloCommand.php
app/Contracts/BaseCommand.php
📄 CodeRabbit inference engine (.cursor/rules/03-commands.mdc)
Set and cache quiet detection once in BaseCommand::initialize(); wrapper methods (writeln, text, hr) must consult $this->isQuiet
Files:
app/Contracts/BaseCommand.php
🧠 Learnings (9)
📚 Learning: 2025-09-24T07:12:42.205Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/00-main.mdc:0-0
Timestamp: 2025-09-24T07:12:42.205Z
Learning: Applies to tests/** : Do not write or run tests unless specifically instructed
Applied to files:
.cursor/rules/00-main.mdc
📚 Learning: 2025-09-24T07:12:42.205Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/00-main.mdc:0-0
Timestamp: 2025-09-24T07:12:42.205Z
Learning: AI Agent Protocol: ULTRATHINK → STEP BY STEP → ACT
Applied to files:
.cursor/rules/00-main.mdc
📚 Learning: 2025-09-28T15:36:51.669Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-28T15:36:51.669Z
Learning: Applies to {app,tests}/**/*.php : Adhere to PSR-12 coding standard in all PHP files
Applied to files:
.cursor/rules/01-architecture.mdc.cursor/rules/02-tests.mdc
📚 Learning: 2025-09-28T15:36:51.669Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-28T15:36:51.669Z
Learning: Services provide atomic, reusable business functionality with no console I/O; accept and return plain PHP types
Applied to files:
.cursor/rules/01-architecture.mdc
📚 Learning: 2025-09-28T15:36:51.669Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-28T15:36:51.669Z
Learning: Services must be stateless, dependency-injected, and handle core business logic, external APIs, and file operations
Applied to files:
.cursor/rules/01-architecture.mdc
📚 Learning: 2025-09-28T15:36:51.669Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-28T15:36:51.669Z
Learning: Only Commands perform console I/O; use SymfonyStyle for all user-facing output; Services return exceptions/structured data
Applied to files:
.cursor/rules/01-architecture.mdc
📚 Learning: 2025-09-28T15:37:26.619Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/03-commands.mdc:0-0
Timestamp: 2025-09-28T15:37:26.619Z
Learning: Commands handle all console I/O; services return plain data and must not perform console operations; validation errors bubble up to commands
Applied to files:
.cursor/rules/01-architecture.mdc
📚 Learning: 2025-09-28T15:36:51.669Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-28T15:36:51.669Z
Learning: Applies to {app,tests}/**/*.php : Declare strict types (`declare(strict_types=1);`) in all PHP files
Applied to files:
.cursor/rules/02-tests.mdc
📚 Learning: 2025-09-28T15:36:51.669Z
Learnt from: CR
PR: bigpixelrocket/deployer-php#0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-28T15:36:51.669Z
Learning: Applies to {app,tests}/**/*.php : Always use the full section header comment format (with the correct number of dashes) and avoid simplified headers
Applied to files:
.cursor/rules/02-tests.mdc
🧬 Code graph analysis (7)
tests/Unit/Services/InventoryServiceTest.php (2)
tests/TestHelpers.php (1)
mockFilesystem(32-126)app/Services/InventoryService.php (6)
InventoryService(35-266)loadInventoryFile(99-113)get(69-75)delete(80-86)set(56-62)getInventoryFileStatus(118-121)
app/Console/HelloCommand.php (2)
app/Contracts/BaseCommand.php (1)
execute(86-104)tests/Unit/Contracts/BaseCommandTest.php (3)
execute(39-44)execute(146-158)execute(193-197)
tests/Unit/Services/EnvServiceTest.php (2)
app/Services/EnvService.php (3)
EnvService(13-135)loadEnvFile(74-91)getEnvFileStatus(96-99)tests/TestHelpers.php (1)
mockFilesystem(32-126)
app/Services/EnvService.php (2)
app/Services/InventoryService.php (1)
setCustomPath(91-94)tests/TestHelpers.php (2)
exists(72-91)readFile(93-105)
app/Contracts/BaseCommand.php (4)
tests/Unit/Contracts/BaseCommandTest.php (6)
configure(33-37)configure(140-144)configure(187-191)execute(39-44)execute(146-158)execute(193-197)app/Services/EnvService.php (3)
setCustomPath(66-69)loadEnvFile(74-91)getEnvFileStatus(96-99)app/Services/InventoryService.php (3)
setCustomPath(91-94)loadInventoryFile(99-113)getInventoryFileStatus(118-121)app/Console/HelloCommand.php (1)
execute(19-28)
tests/Unit/Contracts/BaseCommandTest.php (3)
app/Contracts/BaseCommand.php (6)
configure(37-54)execute(86-104)BaseCommand(16-158)writeln(115-125)text(132-142)hr(147-157)tests/TestHelpers.php (2)
mockEnvService(133-137)mockInventoryService(144-148)app/Console/HelloCommand.php (1)
execute(19-28)
app/Services/InventoryService.php (2)
app/Services/EnvService.php (2)
get(37-61)setCustomPath(66-69)tests/TestHelpers.php (3)
exists(72-91)readFile(93-105)dumpFile(117-124)
🪛 PHPMD (2.15.0)
app/Contracts/BaseCommand.php
86-86: Avoid unused parameters such as '$input'. (undefined)
(UnusedFormalParameter)
86-86: Avoid unused parameters such as '$output'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (33)
.gitignore (1)
10-12: LGTM! Inventory file correctly ignored.The addition of
inventory.ymlto.gitignorecorrectly prevents the new default inventory file from being committed to version control, aligning with the InventoryService refactor.app/Services/InventoryService.php (7)
37-47: LGTM! Clean state initialization.The refactored properties and constructor correctly separate concerns by removing auto-initialization and requiring explicit
loadInventoryFile()calls.
56-62: LGTM! Set operation correctly enforces initialization.The method correctly updates the in-memory inventory and persists changes. The
writeInventory()guard ensures the service is properly initialized.
69-75: LGTM! Default parameter improves API ergonomics.The addition of the
$defaultparameter follows established patterns and provides a cleaner alternative to the removedhas()method.
80-86: LGTM! Delete operation follows consistent pattern.The method correctly removes the path and persists changes, matching the pattern established in
set().
118-121: LGTM! Correct nullable return type.The nullable return type correctly reflects that status is unset until
loadInventoryFile()is called.
232-246: LGTM! Robust error handling and type safety.The method correctly handles file read/parse errors and ensures type safety by defaulting to an empty array if YAML parsing doesn't yield an array.
251-265: LGTM! Critical guard prevents uninitialized writes.The initialization guard on line 253 correctly prevents writing uninitialized inventory state. The error handling and YAML formatting options are appropriate.
.cursor/rules/02-tests.mdc (1)
2-2: LGTM! Rule scope broadened appropriately.Changing to
alwaysApply: trueensures test rules apply unconditionally across the codebase, consistent with the other rule file updates in this PR..cursor/rules/00-main.mdc (1)
35-35: LGTM! Clearer policy wording.The updated wording more emphatically communicates the test policy while maintaining the same intent.
README.md (1)
2-13: LGTM! Enhanced branding and documentation.The ASCII art header, updated tagline, and additional badges improve the README's visual appeal and provide useful project information at a glance.
app/Console/HelloCommand.php (1)
21-22: LGTM! Parent execute call correctly integrated.The addition of
parent::execute($input, $output)properly displays environment and inventory status before the command's main logic, consistent with the BaseCommand pattern shown in related test files..cursor/rules/03-commands.mdc (1)
2-2: LGTM! Rule applicability updated to unconditional.The change from path-specific globs to
alwaysApply: trueensures the console rules apply universally, consistent with the broader rule consolidation across.cursor/rules/*.mdcfiles in this PR..cursor/rules/01-architecture.mdc (3)
2-2: LGTM! Rule applicability updated to unconditional.Consistent with the rule consolidation pattern across the
.cursor/rules/directory.
85-85: LGTM! Service dependency injection guidance clarified.The updated wording maintains the DI requirement while removing the blanket stateless constraint, paving the way for the new Service State section below.
89-96: LGTM! Service State guidance added.The new section provides clear guidance on stateless vs. stateful services, lazy loading patterns, and explicit initialization requirements. This aligns with the broader PR changes to
EnvServiceandInventoryService(per AI summary) that introduce explicitload()methods and path customization.tests/Integration/SymfonyAppTest.php (2)
40-40: LGTM! Test description updated to match simplified banner.The removal of "complete" reflects the streamlined banner content verified below.
65-71: LGTM! Banner expectations updated to match new branding.The test correctly verifies:
- ASCII art structure (lines 66-68)
- Dynamic version line (line 68)
- New branding: "The Server & Site Deployment Tool for PHP" (line 69)
This aligns with the updated banner implementation in
app/SymfonyApp.php(lines 75-82).app/SymfonyApp.php (2)
54-56: LGTM! Banner now respects quiet mode.The conditional display properly honors the
--quiet/-qflag, preventing banner output in automated/CI environments while maintaining visibility in interactive use.
73-83: LGTM! Banner branding updated with improved framing.The changes enhance the banner presentation:
- New decorative frame lines (75, 81) create cleaner boundaries
- Updated branding: "The Server & Site Deployment Tool for PHP" (80)
- Proper spacing with empty lines (74, 79, 82)
- ASCII art and color scheme preserved
The test expectations in
tests/Integration/SymfonyAppTest.php(lines 65-71) correctly verify this new content.app/Contracts/BaseCommand.php (1)
115-125: Quiet-mode gating looks correct.Helpers correctly consult
$this->isQuietset in initialize(); aligns with the project guideline for BaseCommand wrappers.Also applies to: 132-142, 147-157
tests/Unit/Services/InventoryServiceTest.php (6)
24-55: Solid dataset-driven set() coverage.Good breadth (new/existing file, deep paths, type conflicts). Clear AAA structure and fast unit scope.
60-118: Get() scenarios are comprehensive.Positive/negative and “file missing” paths are well covered and readable.
124-144: Defaulting behavior test is on point.Covers scalar, array, null defaults and “existing path ignores default”. Nice.
150-185: Delete() behavior validated precisely.Assertions verify both removal and non-target preservation; clear scenarios.
191-199: Error paths covered well.Initialization write failure, set() write failure, read failure, and “write before init” are all asserted with messages. Great resilience coverage.
Also applies to: 201-211, 212-220, 222-230
236-269: Status reporting test is pragmatic.Pattern-based assertions avoid brittle absolute paths; good trade-off.
app/Services/EnvService.php (1)
122-125: No action needed: symfony/filesystem version requirement satisfied
Composer.json declares"symfony/filesystem": "^7.3", which covers ≥7.1 and supportsreadFile().tests/Unit/Services/EnvServiceTest.php (2)
22-50: LGTM! Clear exception handling with meaningful assertions.The test correctly branches on the exception path and validates both error messages and status patterns. The dataset coverage is comprehensive.
52-85: LGTM! Explicit loadEnvFile() call aligns with new API.The addition of
loadEnvFile()on line 61 correctly reflects the updated EnvService behavior where the constructor no longer auto-loads. The precedence tests clearly verify that file values win over environment values.tests/Unit/Contracts/BaseCommandTest.php (3)
33-37: LGTM! Parent configure() call ensures option registration.Calling
parent::configure()before setting test-specific properties correctly ensures that the env and inventory options are registered by BaseCommand.
52-69: LGTM! Comprehensive verification of command configuration.The test properly verifies command name, option existence, and option descriptions using chained assertions. This validates the configure() flow introduced in BaseCommand.
133-178: LGTM! Thorough testing of quiet mode for wrapper methods.The use of an anonymous class and dataset-driven testing effectively verifies that all three wrapper methods (writeln, text, hr) respect quiet mode. The normal vs. quiet comparison is clear and meaningful.
| public function loadInventoryFile(): void | ||
| { | ||
| $inventory = $this->readInventory(); | ||
| $segments = $this->parsePath($path); | ||
| $this->inventory = []; | ||
|
|
||
| $path = $this->getInventoryPath(); | ||
|
|
||
| // Initialize empty inventory file if it doesn't exist | ||
| if (!$this->filesystem->exists($path)) { | ||
| $this->inventoryFileStatus = "Creating inventory file at {$path}"; | ||
| $this->writeInventory(); | ||
| } | ||
|
|
||
| $this->unsetByPath($inventory, $segments); | ||
| $this->writeInventory($inventory); | ||
| $this->readInventory(); | ||
| $this->inventoryFileStatus = "Reading inventory from {$path}"; | ||
| } |
There was a problem hiding this comment.
Status message inconsistency in creation flow.
When the inventory file doesn't exist, the method sets the status to "Creating..." on line 107, but then immediately overwrites it with "Reading..." on line 112. The final status will always report "Reading" even for newly created files.
Consider this fix to preserve the creation status:
public function loadInventoryFile(): void
{
$this->inventory = [];
$path = $this->getInventoryPath();
// Initialize empty inventory file if it doesn't exist
if (!$this->filesystem->exists($path)) {
$this->inventoryFileStatus = "Creating inventory file at {$path}";
$this->writeInventory();
+ return;
}
$this->readInventory();
$this->inventoryFileStatus = "Reading inventory from {$path}";
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function loadInventoryFile(): void | |
| { | |
| $inventory = $this->readInventory(); | |
| $segments = $this->parsePath($path); | |
| $this->inventory = []; | |
| $path = $this->getInventoryPath(); | |
| // Initialize empty inventory file if it doesn't exist | |
| if (!$this->filesystem->exists($path)) { | |
| $this->inventoryFileStatus = "Creating inventory file at {$path}"; | |
| $this->writeInventory(); | |
| } | |
| $this->unsetByPath($inventory, $segments); | |
| $this->writeInventory($inventory); | |
| $this->readInventory(); | |
| $this->inventoryFileStatus = "Reading inventory from {$path}"; | |
| } | |
| public function loadInventoryFile(): void | |
| { | |
| $this->inventory = []; | |
| $path = $this->getInventoryPath(); | |
| // Initialize empty inventory file if it doesn't exist | |
| if (!$this->filesystem->exists($path)) { | |
| $this->inventoryFileStatus = "Creating inventory file at {$path}"; | |
| $this->writeInventory(); | |
| return; | |
| } | |
| $this->readInventory(); | |
| $this->inventoryFileStatus = "Reading inventory from {$path}"; | |
| } |
🤖 Prompt for AI Agents
In app/Services/InventoryService.php around lines 99 to 113, the code sets
inventoryFileStatus to "Creating..." when the file is missing but then
unconditionally overwrites it with "Reading...", so creation status is lost;
change the control flow so the "Reading..." status is only set when the file
already existed (e.g., wrap readInventory() and the subsequent
inventoryFileStatus = "Reading..." in an else branch of the exists check, or
record a boolean indicating creation and only set the "Reading..." status when
not created) so the creation message is preserved for newly created files.
| private function getInventoryPath(): string | ||
| { | ||
| $path = $this->inventoryPath; | ||
| return $this->inventoryPath ?? rtrim((string) getcwd(), '/') . '/inventory.yml'; | ||
| } |
There was a problem hiding this comment.
Handle getcwd() failure edge case.
getcwd() can return false if the current directory was deleted or is unreadable. The current cast to string would result in a path of /inventory.yml (root directory), which is likely not the intended behavior.
Consider adding explicit error handling:
private function getInventoryPath(): string
{
- return $this->inventoryPath ?? rtrim((string) getcwd(), '/') . '/inventory.yml';
+ if ($this->inventoryPath !== null) {
+ return $this->inventoryPath;
+ }
+
+ $cwd = getcwd();
+ if ($cwd === false) {
+ throw new \RuntimeException('Cannot determine current working directory');
+ }
+
+ return rtrim($cwd, '/') . '/inventory.yml';
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private function getInventoryPath(): string | |
| { | |
| $path = $this->inventoryPath; | |
| return $this->inventoryPath ?? rtrim((string) getcwd(), '/') . '/inventory.yml'; | |
| } | |
| private function getInventoryPath(): string | |
| { | |
| if ($this->inventoryPath !== null) { | |
| return $this->inventoryPath; | |
| } | |
| $cwd = getcwd(); | |
| if ($cwd === false) { | |
| throw new \RuntimeException('Cannot determine current working directory'); | |
| } | |
| return rtrim($cwd, '/') . '/inventory.yml'; | |
| } |
🤖 Prompt for AI Agents
In app/Services/InventoryService.php around lines 222 to 225, the current code
casts getcwd() to string which hides the case where getcwd() returns false;
update the method to explicitly handle getcwd() returning false by checking the
return value first and either throwing a clear RuntimeException (e.g. "Unable to
determine current working directory") or falling back to a deterministic
alternative such as dirname(__DIR__) or a configured base path, then build the
inventory path from that validated directory; ensure the method never relies on
casting false to a string so you don't accidentally return "/inventory.yml".
1 participant
Summary by CodeRabbit