Moved several methods out of the installer command class.#2308
Merged
AlexSkrypnyk merged 5 commits intomainfrom Feb 19, 2026
Merged
Moved several methods out of the installer command class.#2308AlexSkrypnyk merged 5 commits intomainfrom
AlexSkrypnyk merged 5 commits intomainfrom
Conversation
WalkthroughInstallCommand was refactored to delegate responsibilities to new components: OptionsResolver for option/requirement resolution, FileManager for filesystem/demo tasks, and InstallerPresenter for UI/footers; AgentHelp supplies static help text. Build/result constants moved into InstallerPresenter. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant InstallCommand
participant OptionsResolver
participant InstallerPresenter
participant FileManager
participant Downloader
participant PromptManager
CLI->>InstallCommand: run install command (options)
InstallCommand->>OptionsResolver: checkRequirements()
OptionsResolver-->>InstallCommand: ok / throw
InstallCommand->>OptionsResolver: resolve(options)
OptionsResolver-->>InstallCommand: Config + Artifact
InstallCommand->>InstallerPresenter: header(Artifact, version)
InstallerPresenter-->>InstallCommand: rendered header
InstallCommand->>FileManager: prepareDestination()
FileManager-->>InstallCommand: destination messages
InstallCommand->>FileManager: copyFiles()
FileManager-->>InstallCommand: copy result
InstallCommand->>FileManager: prepareDemo(Downloader)
FileManager->>Downloader: download demo DB (if needed)
Downloader-->>FileManager: download result
FileManager-->>InstallCommand: demo messages
InstallCommand->>PromptManager: run post-build hooks (via presenter)
PromptManager-->>InstallerPresenter: handler output
InstallCommand->>InstallerPresenter: footerBuild*(result)
InstallerPresenter-->>CLI: rendered footer and next steps
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.vortex/installer/src/Prompts/InstallerPresenter.php:
- Line 29: The nullable property $promptManager can cause fatal errors because
footerBuildSucceeded(), footerBuildSkipped(), and footerBuildFailed() call
methods on it without guards; fix by making PromptManager required: add a
PromptManager parameter to the InstallerPresenter constructor, assign it to the
$promptManager property and change its type to non-nullable (remove ?), and
remove setPromptManager() usage, or alternatively add null checks in each of
footerBuildSucceeded(), footerBuildSkipped(), and footerBuildFailed() to bail
out or handle the missing manager before calling methods on $promptManager.
In @.vortex/installer/src/Utils/FileManager.php:
- Around line 35-42: Replace the passthru() call with an exec() (or proc_open)
variant that captures both stdout/stderr and the exit code when running the git
init command (the block using passthru(sprintf('git --work-tree="%s"
--git-dir="%s/.git" init > /dev/null', $dst, $dst)) and the subsequent
File::exists($dst . '/.git') check); after running the command, verify the exit
code first and if non-zero throw the RuntimeException including the captured
output/exit code for diagnostics, otherwise proceed to the File::exists check
and keep the existing exception path but augment its message with captured git
output when available.
- Around line 98-138: The prepareDemo method currently returns strings on early
exits and an array on success; change it to always return an array to make the
API consistent: update the function signature to return array (remove string
union) and replace each early string return (the 'Not a demo mode.' check, the
IS_DEMO_DB_DOWNLOAD_SKIP message, the missing VORTEX_DOWNLOAD_DB_URL message,
and the existing DB file found message) with a one-element array containing the
same message (or push into $messages and return $messages), and ensure the final
return remains an array of messages; locate prepareDemo in FileManager (method
name prepareDemo, references to $this->config, Env::get, $downloader->download)
to apply these changes.
In @.vortex/installer/src/Utils/OptionsResolver.php:
- Around line 58-61: When reading a config file in OptionsResolver.php you must
handle file_get_contents returning false: check the result of
file_get_contents($config_candidate) and if it === false either set $config_json
to a safe default (e.g. '{}') or throw a clear exception before calling
Config::fromString; ensure you reference the existing variables
$config_candidate and $config_json and update the branch that currently assigns
$config_json to (string) file_get_contents(...) so it validates the read and
handles the failure case explicitly.
| /** | ||
| * The prompt manager. | ||
| */ | ||
| protected ?PromptManager $promptManager = NULL; |
There was a problem hiding this comment.
Potential null pointer exception when promptManager is not set.
The $promptManager property is declared as nullable (Line 29), but footerBuildSucceeded(), footerBuildSkipped(), and footerBuildFailed() call methods on it without null checks. If setPromptManager() is not called before these methods, a fatal error will occur.
Proposed fix - add null checks or make non-nullable
Option 1: Add null checks in each method
public function footerBuildSucceeded(): void {
$output = '';
$output .= 'Get site info: ahoy info' . PHP_EOL;
$output .= 'Login: ahoy login' . PHP_EOL;
$output .= PHP_EOL;
- $handler_output = $this->promptManager->runPostBuild(self::BUILD_RESULT_SUCCESS);
- if (!empty($handler_output)) {
- $output .= $handler_output;
+ if ($this->promptManager !== NULL) {
+ $handler_output = $this->promptManager->runPostBuild(self::BUILD_RESULT_SUCCESS);
+ if (!empty($handler_output)) {
+ $output .= $handler_output;
+ }
}
Tui::box($output, 'Site is ready');
}Option 2: Require PromptManager in constructor (preferred)
public function __construct(
protected Config $config,
+ protected PromptManager $promptManager,
) {}
- public function setPromptManager(PromptManager $prompt_manager): void {
- $this->promptManager = $prompt_manager;
- }Also applies to: 161-231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.vortex/installer/src/Prompts/InstallerPresenter.php at line 29, The
nullable property $promptManager can cause fatal errors because
footerBuildSucceeded(), footerBuildSkipped(), and footerBuildFailed() call
methods on it without guards; fix by making PromptManager required: add a
PromptManager parameter to the InstallerPresenter constructor, assign it to the
$promptManager property and change its type to non-nullable (remove ?), and
remove setPromptManager() usage, or alternatively add null checks in each of
footerBuildSucceeded(), footerBuildSkipped(), and footerBuildFailed() to bail
out or handle the missing manager before calling methods on $promptManager.
Comment on lines
+35
to
+42
| if (!is_readable($dst . '/.git')) { | ||
| $messages[] = sprintf('Initialising a new Git repository in directory "%s".', $dst); | ||
| passthru(sprintf('git --work-tree="%s" --git-dir="%s/.git" init > /dev/null', $dst, $dst)); | ||
|
|
||
| if (!File::exists($dst . '/.git')) { | ||
| throw new \RuntimeException(sprintf('Unable to initialise Git repository in directory "%s".', $dst)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider capturing git init output and exit code.
Using passthru() with output redirection to /dev/null discards both success and error output. While the subsequent File::exists check catches initialization failures, actual git error messages are lost, making debugging harder. Additionally, the exit code isn't checked before the file existence verification.
Proposed fix using exec()
if (!is_readable($dst . '/.git')) {
$messages[] = sprintf('Initialising a new Git repository in directory "%s".', $dst);
- passthru(sprintf('git --work-tree="%s" --git-dir="%s/.git" init > /dev/null', $dst, $dst));
+ $output = [];
+ $return_code = 0;
+ exec(sprintf('git --work-tree="%s" --git-dir="%s/.git" init 2>&1', $dst, $dst), $output, $return_code);
- if (!File::exists($dst . '/.git')) {
+ if ($return_code !== 0 || !File::exists($dst . '/.git')) {
throw new \RuntimeException(sprintf('Unable to initialise Git repository in directory "%s".', $dst));
}
}📝 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.
Suggested change
| if (!is_readable($dst . '/.git')) { | |
| $messages[] = sprintf('Initialising a new Git repository in directory "%s".', $dst); | |
| passthru(sprintf('git --work-tree="%s" --git-dir="%s/.git" init > /dev/null', $dst, $dst)); | |
| if (!File::exists($dst . '/.git')) { | |
| throw new \RuntimeException(sprintf('Unable to initialise Git repository in directory "%s".', $dst)); | |
| } | |
| } | |
| if (!is_readable($dst . '/.git')) { | |
| $messages[] = sprintf('Initialising a new Git repository in directory "%s".', $dst); | |
| $output = []; | |
| $return_code = 0; | |
| exec(sprintf('git --work-tree="%s" --git-dir="%s/.git" init 2>&1', $dst, $dst), $output, $return_code); | |
| if ($return_code !== 0 || !File::exists($dst . '/.git')) { | |
| throw new \RuntimeException(sprintf('Unable to initialise Git repository in directory "%s".', $dst)); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.vortex/installer/src/Utils/FileManager.php around lines 35 - 42, Replace
the passthru() call with an exec() (or proc_open) variant that captures both
stdout/stderr and the exit code when running the git init command (the block
using passthru(sprintf('git --work-tree="%s" --git-dir="%s/.git" init >
/dev/null', $dst, $dst)) and the subsequent File::exists($dst . '/.git') check);
after running the command, verify the exit code first and if non-zero throw the
RuntimeException including the captured output/exit code for diagnostics,
otherwise proceed to the File::exists check and keep the existing exception path
but augment its message with captured git output when available.
Comment on lines
+98
to
+138
| * @return array|string | ||
| * Array of messages or a single message. | ||
| */ | ||
| public function prepareDemo(Downloader $downloader): array|string { | ||
| if (empty($this->config->get(Config::IS_DEMO))) { | ||
| return 'Not a demo mode.'; | ||
| } | ||
|
|
||
| if (!empty($this->config->get(Config::IS_DEMO_DB_DOWNLOAD_SKIP))) { | ||
| return sprintf('%s is set. Skipping demo database download.', Config::IS_DEMO_DB_DOWNLOAD_SKIP); | ||
| } | ||
|
|
||
| // Reload variables from destination's .env. | ||
| Env::putFromDotenv($this->config->getDst() . '/.env'); | ||
|
|
||
| $url = Env::get('VORTEX_DOWNLOAD_DB_URL'); | ||
| if (empty($url)) { | ||
| return 'No database download URL provided. Skipping demo database download.'; | ||
| } | ||
|
|
||
| $data_dir = $this->config->getDst() . DIRECTORY_SEPARATOR . Env::get('VORTEX_DB_DIR', './.data'); | ||
| $db_file = Env::get('VORTEX_DB_FILE', 'db.sql'); | ||
|
|
||
| if (file_exists($data_dir . DIRECTORY_SEPARATOR . $db_file)) { | ||
| return 'Database dump file already exists. Skipping demo database download.'; | ||
| } | ||
|
|
||
| $messages = []; | ||
| if (!file_exists($data_dir)) { | ||
| $data_dir = File::mkdir($data_dir); | ||
| $messages[] = sprintf('Created data directory "%s".', $data_dir); | ||
| } | ||
|
|
||
| $destination = $data_dir . DIRECTORY_SEPARATOR . $db_file; | ||
| $downloader->download($url, $destination); | ||
|
|
||
| $messages[] = sprintf('No database dump file was found in "%s" directory.', $data_dir); | ||
| $messages[] = sprintf('Downloaded demo database from %s.', $url); | ||
|
|
||
| return $messages; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent return type reduces predictability.
The method returns string for early-exit cases (Lines 103, 107, 115, 122) but array for the success path (Line 137). This forces callers to handle both types, adding unnecessary complexity.
Proposed fix - always return array
- public function prepareDemo(Downloader $downloader): array|string {
+ public function prepareDemo(Downloader $downloader): array {
if (empty($this->config->get(Config::IS_DEMO))) {
- return 'Not a demo mode.';
+ return ['Not a demo mode.'];
}
if (!empty($this->config->get(Config::IS_DEMO_DB_DOWNLOAD_SKIP))) {
- return sprintf('%s is set. Skipping demo database download.', Config::IS_DEMO_DB_DOWNLOAD_SKIP);
+ return [sprintf('%s is set. Skipping demo database download.', Config::IS_DEMO_DB_DOWNLOAD_SKIP)];
}
// ... similar changes for other early returns🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.vortex/installer/src/Utils/FileManager.php around lines 98 - 138, The
prepareDemo method currently returns strings on early exits and an array on
success; change it to always return an array to make the API consistent: update
the function signature to return array (remove string union) and replace each
early string return (the 'Not a demo mode.' check, the IS_DEMO_DB_DOWNLOAD_SKIP
message, the missing VORTEX_DOWNLOAD_DB_URL message, and the existing DB file
found message) with a one-element array containing the same message (or push
into $messages and return $messages), and ensure the final return remains an
array of messages; locate prepareDemo in FileManager (method name prepareDemo,
references to $this->config, Env::get, $downloader->download) to apply these
changes.
Comment on lines
+58
to
+61
| if (isset($options['config']) && is_scalar($options['config'])) { | ||
| $config_candidate = strval($options['config']); | ||
| $config_json = is_file($config_candidate) ? (string) file_get_contents($config_candidate) : $config_candidate; | ||
| } |
There was a problem hiding this comment.
Handle file_get_contents failure.
If the file exists but cannot be read, file_get_contents returns false, which when cast to string becomes an empty string. This would pass an empty string (not '{}') to Config::fromString, potentially causing unexpected behavior or JSON parsing errors downstream.
Proposed fix
if (isset($options['config']) && is_scalar($options['config'])) {
$config_candidate = strval($options['config']);
- $config_json = is_file($config_candidate) ? (string) file_get_contents($config_candidate) : $config_candidate;
+ if (is_file($config_candidate)) {
+ $contents = file_get_contents($config_candidate);
+ if ($contents === FALSE) {
+ throw new \RuntimeException(sprintf('Unable to read config file: %s', $config_candidate));
+ }
+ $config_json = $contents;
+ }
+ else {
+ $config_json = $config_candidate;
+ }
}📝 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.
Suggested change
| if (isset($options['config']) && is_scalar($options['config'])) { | |
| $config_candidate = strval($options['config']); | |
| $config_json = is_file($config_candidate) ? (string) file_get_contents($config_candidate) : $config_candidate; | |
| } | |
| if (isset($options['config']) && is_scalar($options['config'])) { | |
| $config_candidate = strval($options['config']); | |
| if (is_file($config_candidate)) { | |
| $contents = file_get_contents($config_candidate); | |
| if ($contents === FALSE) { | |
| throw new \RuntimeException(sprintf('Unable to read config file: %s', $config_candidate)); | |
| } | |
| $config_json = $contents; | |
| } | |
| else { | |
| $config_json = $config_candidate; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.vortex/installer/src/Utils/OptionsResolver.php around lines 58 - 61, When
reading a config file in OptionsResolver.php you must handle file_get_contents
returning false: check the result of file_get_contents($config_candidate) and if
it === false either set $config_json to a safe default (e.g. '{}') or throw a
clear exception before calling Config::fromString; ensure you reference the
existing variables $config_candidate and $config_json and update the branch that
currently assigns $config_json to (string) file_get_contents(...) so it
validates the read and handles the failure case explicitly.
Member
Author
|
This comment has been minimized.
This comment has been minimized.
Member
Author
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2308 +/- ##
==========================================
- Coverage 78.20% 78.09% -0.11%
==========================================
Files 121 117 -4
Lines 6400 6245 -155
Branches 44 0 -44
==========================================
- Hits 5005 4877 -128
+ Misses 1395 1368 -27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.vortex/installer/tests/Unit/Utils/OptionsResolverTest.php:
- Around line 209-216: The test testResolveSetsIsVortexProject currently asserts
that Config::IS_VORTEX_PROJECT is not null, which can pass when the value is
TRUE; update the assertion to explicitly assert false so the SUT is confirmed
not to be a Vortex project: locate the testResolveSetsIsVortexProject method and
replace the assertNotNull($config->get(Config::IS_VORTEX_PROJECT)) with an
assertion that the value is false (e.g., use assertFalse on
$config->get(Config::IS_VORTEX_PROJECT)) to match the test comment and intent
when calling OptionsResolver::resolve.
Comment on lines
+209
to
+216
| public function testResolveSetsIsVortexProject(): void { | ||
| $options = self::defaultOptions(); | ||
|
|
||
| [$config] = OptionsResolver::resolve($options); | ||
|
|
||
| // The SUT directory should not be a Vortex project. | ||
| $this->assertNotNull($config->get(Config::IS_VORTEX_PROJECT)); | ||
| } |
There was a problem hiding this comment.
Strengthen the Vortex project assertion.
Line 215 only asserts non-null, which will pass even if the value is TRUE. The comment says the SUT should not be a Vortex project, so this should assert false explicitly.
✅ Proposed fix
- // The SUT directory should not be a Vortex project.
- $this->assertNotNull($config->get(Config::IS_VORTEX_PROJECT));
+ // The SUT directory should not be a Vortex project.
+ $this->assertFalse((bool) $config->get(Config::IS_VORTEX_PROJECT));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.vortex/installer/tests/Unit/Utils/OptionsResolverTest.php around lines 209
- 216, The test testResolveSetsIsVortexProject currently asserts that
Config::IS_VORTEX_PROJECT is not null, which can pass when the value is TRUE;
update the assertion to explicitly assert false so the SUT is confirmed not to
be a Vortex project: locate the testResolveSetsIsVortexProject method and
replace the assertNotNull($config->get(Config::IS_VORTEX_PROJECT)) with an
assertion that the value is false (e.g., use assertFalse on
$config->get(Config::IS_VORTEX_PROJECT)) to match the test comment and intent
when calling OptionsResolver::resolve.
Member
Author
|
|
Member
Author
|
Member
Author
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Tests