Skip to content

refactor: server install modularization - #86

Merged
loadinglucian merged 11 commits into
mainfrom
refactor/server-install-modularization
Nov 16, 2025
Merged

refactor: server install modularization#86
loadinglucian merged 11 commits into
mainfrom
refactor/server-install-modularization

Conversation

@loadinglucian

@loadinglucian loadinglucian commented Nov 15, 2025

Copy link
Copy Markdown
Owner

Summary by CodeRabbit

  • New Features

    • System-wide Bun installer, deployer user setup with SSH key export, and new package-list and PHP discovery routines.
    • New HTTP verification helper for URL checks.
  • Improvements

    • Action-oriented progress messages (arrow style) and clearer, idempotent install flows for Ubuntu/Debian.
    • Hardened demo-site and Caddy setup with prerequisite checks and PHP-FPM restart handling.
    • More detailed YAML output including distro and PHP metadata.
  • Removed

    • Prior standalone server-install and PHP-install commands/scripts.

Remove ServerInstallPhpCommand and integrate PHP installation
functionality into the main ServerInstallCommand. Add support for
PHP extensions and improve command options.
Add new HttpService for handling HTTP operations and API calls.
Provides centralized HTTP client functionality for the application.
Remove the large monolithic server-install.sh playbook in preparation
for splitting it into smaller, focused playbooks.
Add new package-list.sh playbook that handles package list updates
and repository configuration. Supports gathering PHP versions and
extensions for informed package selection.
Add three new focused installation playbooks:
- install-base.sh: Installs Caddy and Git
- install-deployer.sh: Sets up deployer user and SSH keys
- install-bun.sh: Installs Bun JavaScript runtime
Rename server-install-php.sh to install-php.sh for consistency
with other install-* playbooks. Update functionality to work
with the new modular installation approach.
Update demo-site.sh, helpers.sh, and server-info.sh to work
with the new modular installation approach and improved
package management.
Update .cursor/rules/06-playbooks.mdc to reflect the new
modular playbook structure and installation approach.
@coderabbitai

coderabbitai Bot commented Nov 15, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

Refactors server installation into modular playbooks (package-list, install-base, install-bun, install-deployer, install-php, demo-site), removes the monolithic server-install.sh and ServerInstallPhpCommand, adds an HttpService and injects it into BaseCommand, updates ServerInstallCommand to orchestrate playbooks and PHP flow, and standardizes playbook outputs and progress messaging.

Changes

Cohort / File(s) Summary
Playbook additions
playbooks/install-base.sh, playbooks/install-bun.sh, playbooks/install-deployer.sh, playbooks/package-list.sh
New modular playbooks implementing base package install & Caddy config, Bun installer, deployer user + SSH key setup, and package/repo listing with apt throttling and optional PHP detection.
Playbook updates
playbooks/install-php.sh, playbooks/demo-site.sh, playbooks/server-info.sh, playbooks/helpers.sh, .cursor/rules/06-playbooks.mdc
Converted PHP extension install to use DEPLOYER_PHP_EXTENSIONS, added detect_php_extensions(), enriched PHP metadata (versions + extensions), changed progress indicators (✓ → →), tightened distro handling (Ubuntu/Debian), improved idempotency and messaging, and updated docs/examples.
Playbook removals
playbooks/server-install.sh
Removed monolithic server-install.sh; its responsibilities split into new modular playbooks.
Command consolidation & wiring
app/Console/Server/ServerInstallCommand.php, app/Console/Server/ServerInstallPhpCommand.php
Merged PHP install flow into ServerInstallCommand (new private installPhp signature accepting package list); removed the standalone ServerInstallPhpCommand class.
Service & base command changes
app/Services/HttpService.php, app/Contracts/BaseCommand.php, app/SymfonyApp.php
Added HttpService (verifyUrl) and injected it into BaseCommand constructor; removed ServerInstallPhpCommand from SymfonyApp command registration.
Trait / display logic
app/Traits/ServersTrait.php
displayServerInfo() updated to support array-form PHP version data (version + extensions + default flag); removed trait's installPhp helper.
Playbook outputs & consistency
playbooks/* (playbooks writing YAML outputs)
Standardized and enriched YAML outputs (distro, caddy_version, php metadata, etc.), unified action-style progress messages, and added explicit error handling around writing/verification.
Tooling / reviewer helper
.cursor/commands/review-pr-comment.md
Added a reviewer instruction file for PR comment assessment.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant CLI as ServerInstallCommand
    participant Http as HttpService
    participant Playbook as Remote Playbooks
    participant Remote as Remote Server

    User->>CLI: server:install --server <id>
    CLI->>Playbook: package-list.sh (DEPLOYER_GATHER_PHP?)
    Playbook->>Remote: configure repos, apt update
    Playbook-->>CLI: php metadata (versions + extensions)

    CLI->>Playbook: install-base.sh
    Playbook->>Remote: install packages, configure Caddy
    Playbook-->>CLI: status

    CLI->>Playbook: install-bun.sh
    Playbook->>Remote: install Bun
    Playbook-->>CLI: status

    CLI->>Playbook: install-deployer.sh
    Playbook->>Remote: create deployer user, ssh key
    Playbook-->>CLI: deployer public key

    CLI->>User: prompt choose PHP version & extensions
    User-->>CLI: selection
    CLI->>Playbook: install-php.sh (selected extensions)
    Playbook->>Remote: install PHP packages, restart php-fpm
    Playbook-->>CLI: status

    CLI->>Playbook: demo-site.sh
    Playbook->>Remote: create site, configure Caddy
    Playbook-->>CLI: status

    CLI->>Http: verifyUrl(<url>) 
    Http-->>CLI: {success, status_code, body}
    CLI-->>User: verification result & next steps
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

  • Areas needing focused review:
    • app/Console/Server/ServerInstallCommand.php — new orchestration, installPhp signature, state/replay propagation.
    • playbooks/package-list.sh — apt throttling, repo detection, PHP detection caching and cache invalidation.
    • playbooks/install-deployer.sh — user creation, SSH key generation, permissions and idempotency.
    • app/Contracts/BaseCommand.php & app/Services/HttpService.php — DI change and HTTP verification behavior.
    • playbook output format changes — ensure callers/consumers parse the enriched YAML shape.

Possibly related PRs

  • bigpixelrocket/deployer-php#62 — related server-info enhancements and the command that consumes the playbook PHP metadata.
  • bigpixelrocket/deployer-php#64 — related trait refactor (ServersTrait/PlaybooksTrait) and executePlaybook behavior changes.
  • bigpixelrocket/deployer-php#75 — overlaps on server install flow, deploy key handling, and verification logic.

Poem

🐰 I hopped through scripts and tidy'd chores,
Arrows now guide actions across the floors.
Bun, Caddy, deployer keys in tow,
PHP versions bloom with extensions aglow.
YAML hums — the rabbit says, "Let's go!"

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'refactor: server install modularization' clearly summarizes the main change—breaking down server installation into modular components across multiple scripts and classes.
Docstring Coverage ✅ Passed Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch refactor/server-install-modularization

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 31d69d4 and e12f6f4.

📒 Files selected for processing (1)
  • app/Console/Server/ServerInstallCommand.php (7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php

📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)

**/*.php: Eliminate single-use methods: inline if a method is called only once
Cache computed values: initialize expensive calculations in the constructor
Avoid method call overhead: prefer direct property access when appropriate

Files:

  • app/Console/Server/ServerInstallCommand.php
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: deployer-plus/deployer-php PR: 0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-22T11:10:21.459Z
Learning: Applies to app/**/@(Service|Services)/**/*.php : Extract complex orchestration shared by multiple Commands into dedicated Services
🔇 Additional comments (6)
app/Console/Server/ServerInstallCommand.php (6)

31-39: LGTM: Configuration changes support new PHP installation flow.

The option changes correctly enable the refactored installation:

  • VALUE_NEGATABLE for php-default allows explicit true/false from CLI
  • php-extensions with VALUE_REQUIRED accepts comma-separated extension lists

82-99: LGTM: Package list preparation enables data-driven PHP installation.

The new preparatory step correctly gathers available PHP versions and extensions before prompting the user, enabling validation of user selections against actual package availability.


255-470: LGTM: Well-structured PHP installation orchestration.

The installPhp method is single-use but appropriately kept as a private method given its size (~215 lines) and complexity. Inlining would significantly harm readability of execute(). The implementation correctly:

  • Validates package list and server info structure with proper type guards
  • Handles backward compatibility for old/new PHP version formats
  • Filters extensions to only those available for the selected version
  • Implements sensible three-way default-setting logic (auto-default for first install, skip prompt if already default, otherwise prompt)
  • Properly parses CLI string input and filters empty strings (addressing past review concern)
  • Returns structured data enabling conditional command replay

206-230: LGTM: IPv6 handling corrected and verification flow properly integrated.

The verification correctly brackets any IPv6 address (including ::1) per the past review, and properly integrates with the injected HTTP service for URL verification.


236-246: LGTM: Command replay correctly reflects user choices.

The conditional inclusion of php-default ensures the replay command only shows options the user explicitly selected, avoiding clutter from auto-defaulted values.


476-530: LGTM: Verification provides helpful feedback and next steps.

The method properly leverages the injected HTTP service, differentiates between network and protocol errors, and provides clear next-step guidance including the deploy key when available.


Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (4)
app/Traits/ServersTrait.php (1)

258-301: PHP versions rendering is solid; consider tightening the phpdoc

The new loop correctly handles both the new {version, extensions} structure and the legacy scalar format, and default detection is robust. The only nit is the inline doc:

/** @var string|int|float */
$version = $versionData['version'];

Most static analysers expect the variable name:

-/** @var string|int|float */
+/** @var string|int|float $version */
 $version = $versionData['version'];

Purely optional polish; behavior is fine as-is.

playbooks/install-bun.sh (1)

1-55: Bun install flow looks good; double‑check run_cmd availability

The playbook is idempotent, fails fast on install/output errors, and matches the DEPLOYER_OUTPUT_FILE YAML pattern—nice.

One thing to confirm: this script relies on run_cmd but the helpers import is commented out with the note about automatic inlining:

# Shared helpers are automatically inlined when executing playbooks remotely
# source "$(dirname "$0")/helpers.sh"

If there are any code paths (e.g., local/manual execution) where the inlining doesn’t happen, run_cmd will be undefined. If that’s a possibility, consider either:

  • Un‑commenting the source line, or
  • Documenting that this script must only be invoked through the mechanism that inlines helpers.sh.
playbooks/demo-site.sh (1)

145-201: Caddy + PHP-FPM wiring is sound; consider double-checking php_fastcgi upstream

Using detect_php_default() plus a version-specific PHP-FPM socket and adding php_fastcgi + file_server is a good match for the Debian/Ubuntu layout, and restarting PHP-FPM after reloading Caddy is reasonable. One thing to double-check: with php_fpm_socket including a leading /, php_fastcgi unix//${php_fpm_socket} expands to unix///run/...; if you haven’t already validated this with Caddy, it may be worth confirming it behaves as expected or adjusting the socket string.

Also applies to: 222-227

.cursor/rules/06-playbooks.mdc (1)

210-225: Rules align with new playbooks; verify referenced helper example path

The updated guidance about shared helpers, distro env vars, and arrow-style action messages matches how the new Bash playbooks are written. One thing to confirm: the text references playbooks/package-manager.sh as an example, while this PR introduces playbooks/package-list.sh. If package-manager.sh no longer exists or isn’t the canonical example, it’s worth updating the reference.

Also applies to: 254-293

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b8d62aa and 04b6a42.

📒 Files selected for processing (16)
  • .cursor/rules/06-playbooks.mdc (8 hunks)
  • app/Console/Server/ServerInstallCommand.php (7 hunks)
  • app/Console/Server/ServerInstallPhpCommand.php (0 hunks)
  • app/Contracts/BaseCommand.php (2 hunks)
  • app/Services/HttpService.php (1 hunks)
  • app/SymfonyApp.php (0 hunks)
  • app/Traits/ServersTrait.php (1 hunks)
  • playbooks/demo-site.sh (9 hunks)
  • playbooks/helpers.sh (2 hunks)
  • playbooks/install-base.sh (1 hunks)
  • playbooks/install-bun.sh (1 hunks)
  • playbooks/install-deployer.sh (1 hunks)
  • playbooks/install-php.sh (8 hunks)
  • playbooks/package-list.sh (1 hunks)
  • playbooks/server-info.sh (7 hunks)
  • playbooks/server-install.sh (0 hunks)
💤 Files with no reviewable changes (3)
  • app/SymfonyApp.php
  • app/Console/Server/ServerInstallPhpCommand.php
  • playbooks/server-install.sh
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php

📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)

**/*.php: Eliminate single-use methods: inline if a method is called only once
Cache computed values: initialize expensive calculations in the constructor
Avoid method call overhead: prefer direct property access when appropriate

Files:

  • app/Contracts/BaseCommand.php
  • app/Services/HttpService.php
  • app/Traits/ServersTrait.php
  • app/Console/Server/ServerInstallCommand.php
🧠 Learnings (10)
📚 Learning: 2025-09-22T11:10:21.459Z
Learnt from: CR
Repo: deployer-plus/deployer-php PR: 0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-22T11:10:21.459Z
Learning: Applies to app/**/Command/**/*.php : Commands receive Services via constructor injection

Applied to files:

  • app/Contracts/BaseCommand.php
📚 Learning: 2025-10-24T20:00:14.534Z
Learnt from: CR
Repo: bigpixelrocket/deployer-php PR: 0
File: .cursor/rules/02-tests.mdc:0-0
Timestamp: 2025-10-24T20:00:14.534Z
Learning: Applies to tests/TestHelpers.php : When BaseCommand gains a new service, update mockCommandContainer() in tests/TestHelpers.php: add parameter, build/default it, and bind it

Applied to files:

  • app/Contracts/BaseCommand.php
📚 Learning: 2025-09-22T11:10:21.459Z
Learnt from: CR
Repo: deployer-plus/deployer-php PR: 0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-22T11:10:21.459Z
Learning: Applies to app/**/Command/**/*.php : Commands may only depend on Services (not other Commands)

Applied to files:

  • app/Contracts/BaseCommand.php
📚 Learning: 2025-09-22T11:10:21.459Z
Learnt from: CR
Repo: deployer-plus/deployer-php PR: 0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-22T11:10:21.459Z
Learning: Applies to app/**/@(Service|Services)/**/*.php : Extract complex orchestration shared by multiple Commands into dedicated Services

Applied to files:

  • app/Contracts/BaseCommand.php
📚 Learning: 2025-09-22T11:10:21.459Z
Learnt from: CR
Repo: deployer-plus/deployer-php PR: 0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-22T11:10:21.459Z
Learning: Applies to app/**/@(Service|Services)/**/*.php : Services receive other Services/utilities via constructor injection

Applied to files:

  • app/Contracts/BaseCommand.php
📚 Learning: 2025-09-22T11:10:21.459Z
Learnt from: CR
Repo: deployer-plus/deployer-php PR: 0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-22T11:10:21.459Z
Learning: Applies to app/**/@(Service|Services)/**/*.php : Services may depend on other Services or utilities

Applied to files:

  • app/Contracts/BaseCommand.php
📚 Learning: 2025-09-22T11:10:21.459Z
Learnt from: CR
Repo: deployer-plus/deployer-php PR: 0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-22T11:10:21.459Z
Learning: Applies to app/**/@(Service|Services)/**/*.php : Services provide atomic, reusable functionality and must not perform console I/O

Applied to files:

  • app/Contracts/BaseCommand.php
📚 Learning: 2025-09-22T11:10:21.459Z
Learnt from: CR
Repo: deployer-plus/deployer-php PR: 0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-22T11:10:21.459Z
Learning: Applies to app/**/@(Service|Services)/**/*.php : Services must be stateless and use dependency injection

Applied to files:

  • app/Contracts/BaseCommand.php
📚 Learning: 2025-09-22T11:10:21.459Z
Learnt from: CR
Repo: deployer-plus/deployer-php PR: 0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-22T11:10:21.459Z
Learning: Applies to app/**/@(Service|Services)/**/*.php : Services handle core business logic, external API calls, and file operations

Applied to files:

  • app/Contracts/BaseCommand.php
📚 Learning: 2025-09-22T11:10:21.459Z
Learnt from: CR
Repo: deployer-plus/deployer-php PR: 0
File: .cursor/rules/01-architecture.mdc:0-0
Timestamp: 2025-09-22T11:10:21.459Z
Learning: Applies to app/**/*.php : Use a ServiceContainer/DI container for all object creation

Applied to files:

  • app/Contracts/BaseCommand.php
🧬 Code graph analysis (8)
app/Contracts/BaseCommand.php (1)
app/Services/HttpService.php (1)
  • HttpService (14-41)
playbooks/package-list.sh (1)
playbooks/helpers.sh (2)
  • apt_get_with_retry (97-133)
  • run_cmd (18-24)
playbooks/demo-site.sh (3)
playbooks/helpers.sh (2)
  • detect_php_default (33-53)
  • run_cmd (18-24)
playbooks/server-install.sh (2)
  • setup_deploy_directories (342-380)
  • setup_caddy_structure (156-197)
playbooks/server-install-php.sh (2)
  • update_caddy_config (236-304)
  • configure_php_fpm (128-166)
playbooks/install-deployer.sh (1)
playbooks/helpers.sh (1)
  • run_cmd (18-24)
playbooks/install-php.sh (1)
playbooks/helpers.sh (2)
  • apt_get_with_retry (97-133)
  • run_cmd (18-24)
playbooks/install-bun.sh (1)
playbooks/helpers.sh (1)
  • run_cmd (18-24)
playbooks/server-info.sh (1)
playbooks/helpers.sh (1)
  • detect_php_default (33-53)
playbooks/install-base.sh (1)
playbooks/helpers.sh (2)
  • apt_get_with_retry (97-133)
  • run_cmd (18-24)
🔇 Additional comments (17)
playbooks/helpers.sh (1)

74-74: Message text cleanup looks good

Dropping the leading checkmark keeps these logs consistent with the new, less noisy progress style; no behavioral changes introduced.

Also applies to: 116-116

app/Contracts/BaseCommand.php (1)

14-15: HttpService injection aligns with the command/service architecture—verify wiring & tests

Adding HttpService as a constructor dependency on BaseCommand matches the pattern of “commands receive services via constructor injection” and keeps HTTP concerns in a dedicated service, which is good.

Because BaseCommand sits under all commands, please double‑check:

  • The DI/container wiring that constructs commands now passes an HttpService instance.
  • Test helpers (notably tests/TestHelpers::mockCommandContainer()) are updated to provide a default/mock HttpService, as per the project rule that any new BaseCommand dependency must be reflected there. Based on learnings.

Also applies to: 39-59

playbooks/demo-site.sh (2)

12-31: Env validation and new require_services() flow look consistent

Requiring DEPLOYER_DISTRO and adding require_services() in main() gives a clear, early failure path if Caddy/PHP/PHP config are missing and aligns with the playbook rules. Nothing blocking here.

Also applies to: 58-79, 234-239


88-136: setup_demo_site() is idempotent and safely tightens permissions

Directory creation, index.php provisioning, and ownership/permission changes are all guarded with existence checks and explicit error handling, so repeated runs are safe and predictable.

playbooks/install-base.sh (1)

68-127: Caddy base config and YAML output follow the documented playbook pattern

Directory creation, marker-based Caddyfile management, localhost snippet creation, and conditional reload are all idempotent and guarded with errors. The main function uses the standard main() + YAML-write pattern with proper error checking.

Also applies to: 133-147

playbooks/server-info.sh (2)

196-217: PHP extension detection and php.versions YAML structure look correct

detect_php_extensions() uses php<version> -m, filters section headers, and returns a comma-separated list which is rendered as extensions: [ext1,ext2,...]. That matches the new php: { default, versions: [...] } shape and is backward-compatible with the installPhp() logic that only requires the version field. No functional issues here; extensions are informative and won’t break consumers even if the list is long.

Also applies to: 465-487


417-444: Consolidated PHP-FPM metrics emission is coherent and guarded

Accumulating per-version FPM metrics into php_fpm_yaml and writing the block only when has_fpm_metrics is true avoids sparse structures and keeps the final YAML compact. Indentation under php_fpm: and fallback to {} when no metrics exist both look correct, and errors on writes are handled appropriately.

Also applies to: 488-516

playbooks/install-deployer.sh (2)

45-147: Deployer user/group and SSH setup is idempotent and secure

Group membership adjustments for caddy/www-data, service restarts, .ssh directory creation, key generation, and permission fixes are all guarded with existence checks and clear error handling. Re-running the playbook safely converges state without duplicating work.


153-160: Main deployer flow and YAML output are straightforward

Creating the deployer user if missing, resolving the home via getent, delegating to setup helpers, and emitting a small YAML payload with deploy_public_key matches the documented playbook pattern and should integrate cleanly with the command layer.

Also applies to: 166-204

playbooks/package-list.sh (1)

47-75: Caching helpers for apt and PHP metadata are sensible and bounded

smart_apt_update() and get_php_with_cache() use simple timestamp files in /tmp to throttle expensive operations and avoid redundant PHP detection, while honoring a force flag. Error handling on apt failures is explicit, and gating PHP gathering behind DEPLOYER_GATHER_PHP == 'true' keeps the script cheap by default.

Also applies to: 86-153

app/Console/Server/ServerInstallCommand.php (1)

35-39: New multi-stage install flow and replay options are coherent

Wiring package-list, install-base, install-php, install-bun, install-deployer, and demo-site through executePlaybook() with distro/permissions env vars produces a clear, linear flow, and the replay options now include php-version and php-extensions (with php-default only when actually prompted), which keeps command replay faithful to the interactive path. From the command side this looks consistent; correctness now mostly depends on the playbooks’ YAML contracts, which your new scripts appear to satisfy.

Also applies to: 81-119, 124-177, 182-201, 235-246

playbooks/install-php.sh (6)

1-41: Documentation and environment setup look solid.

The header clearly documents the playbook's scope, and the validation of all required environment variables is comprehensive. The new DEPLOYER_PHP_EXTENSIONS variable is properly documented and validated.


58-75: Dynamic extension parsing is clean and idempotent.

The extension parsing logic correctly splits, trims, and constructs package names. The array expansion to apt_get_with_retry is properly handled, and error handling is appropriate.


84-122: PHP-FPM configuration is idempotent and well-structured.

The socket ownership and permission settings are correctly applied via sed. The conditional enable check prevents redundant systemctl operations, and the final restart is idempotent (starts if not running, or restarts if already running).


131-144: Default version setup is gated correctly and suppresses non-critical errors appropriately.

The early return ensures idempotency, and the conditional error suppression is reasonable for optional system-level operations.


153-220: Caddy configuration update is idempotent and safely structured.

The script checks for existing configuration at multiple levels (file existence, endpoint presence, marker validation) to avoid redundant updates. Temporary file handling for safe in-place updates is correct, and the Caddy reload is appropriately gated to only run if the service is active.


226-240: Main routine is clean and straightforward.

The sequential task execution and YAML output are appropriate. Error handling from individual functions will propagate correctly via the set -o pipefail at the top of the script.

Comment thread app/Console/Server/ServerInstallCommand.php
Comment thread app/Console/Server/ServerInstallCommand.php
Comment thread app/Services/HttpService.php
Comment thread playbooks/install-base.sh
Comment thread playbooks/package-list.sh
- Add exception handling to HttpService::verifyUrl() for network failures
- Return proper error status (status_code: 0) for connection issues
- Improve IPv6 URL handling in ServerInstallCommand
- Filter empty extension values and use strict empty array checks
- Provide clearer error messages for connection vs HTTP status failures

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
app/Console/Server/ServerInstallCommand.php (2)

283-345: PHP version selection: consider explicit validation for CLI overrides

The discovery of available versions from packageList['php'] and support for both old/new info['php'] formats is nicely robust. One small improvement: when --php-version is provided on the CLI with a value not present in $phpVersions, the user currently receives a generic “No extensions available for PHP X” error. You could fail earlier with a clearer message by validating the chosen version against $phpVersions:

         $phpVersion = (string) $this->io->getOptionOrPrompt(
             'php-version',
             fn () => $this->io->promptSelect(
                 label: 'PHP version:',
                 options: $phpVersions,
                 default: $defaultVersion
             )
         );
+
+        // Ensure CLI-provided version is actually available
+        if (!in_array($phpVersion, $phpVersions, true)) {
+            $this->io->error(
+                "PHP version {$phpVersion} is not available. " .
+                'Available versions: ' . implode(', ', $phpVersions)
+            );
+
+            return Command::FAILURE;
+        }

This keeps error reporting tight and avoids routing “bad CLI version” through the “no extensions” branch.


328-372: Extension selection flow is strong; add validation for unknown CLI extensions

The default-extension filtering and the improved CSV parsing for --php-extensions (trimming + filtering out empties) are good. To catch typos in CLI-specified extensions, you might validate that all selected extensions exist in $availableExtensions before continuing:

         if (is_string($selectedExtensions)) {
             $selectedExtensions = array_filter(
                 array_map(trim(...), explode(',', $selectedExtensions)),
                 static fn (string $ext): bool => $ext !== ''
             );
         }
 
-        if (!is_array($selectedExtensions) || $selectedExtensions === []) {
+        if (!is_array($selectedExtensions)) {
+            $this->io->error('Invalid PHP extensions selection');
+
+            return Command::FAILURE;
+        }
+
+        // Ensure all selected extensions are known for this PHP version
+        $unknown = array_diff($selectedExtensions, $availableExtensions);
+        if ($unknown !== []) {
+            $this->io->error(
+                'Unknown PHP extensions for PHP ' . $phpVersion . ': ' . implode(', ', $unknown)
+            );
+
+            return Command::FAILURE;
+        }
+
+        if ($selectedExtensions === []) {
             $this->io->error('At least one extension must be selected');
 
             return Command::FAILURE;
         }

This keeps the Ansible layer from ever seeing invalid extension names and gives immediate, actionable feedback to the user.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 04b6a42 and 31d69d4.

📒 Files selected for processing (3)
  • .cursor/commands/review-pr-comment.md (1 hunks)
  • app/Console/Server/ServerInstallCommand.php (7 hunks)
  • app/Services/HttpService.php (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/Services/HttpService.php
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php

📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)

**/*.php: Eliminate single-use methods: inline if a method is called only once
Cache computed values: initialize expensive calculations in the constructor
Avoid method call overhead: prefer direct property access when appropriate

Files:

  • app/Console/Server/ServerInstallCommand.php
🔇 Additional comments (13)
.cursor/commands/review-pr-comment.md (1)

1-1: This is a meta-instruction file, not reviewable code.

The file contains an instruction template but lacks the actual PR comment content needed to perform the requested assessment. Please provide the PR comment that requires evaluation, or clarify if other modified files from PR #86 should be included in this review.

app/Console/Server/ServerInstallCommand.php (12)

7-9: ServerDTO import and typing are consistent

The added ServerDTO import matches the new installPhp signature and keeps the command’s server parameter strongly typed; no issues here.


31-39: New PHP-related CLI options are wired correctly

Using VALUE_NEGATABLE for php-default and a required CSV option for php-extensions cleanly matches the later prompting logic and replay generation.


81-119: package-list + install-base sequencing looks solid

Introducing the package-list playbook before install-base (with early return on integer status) gives you structured PHP/package data up front while preserving the existing failure-propagation pattern; this flow reads clean and cohesive.


124-135: installPhp orchestration and result unpacking are clear

Delegating the PHP-specific logic to installPhp() and then unpacking php_version, php_default(_prompted), and php_extensions for later steps keeps execute() readable while passing along all state needed for replay and downstream playbooks.


136-176: Bun and deployer-user steps integrate well; confirm return-code semantics

The new install-bun and install-deployer playbook calls mirror the existing pattern and early-return on integer results; the only nuance is that deployer failures return the underlying playbook code while demo-site failures normalize to Command::FAILURE. If that distinction is intentional (e.g., for more granular CI/debugging on the deployer step), the current handling is fine; otherwise you may want to standardize on one convention.


183-190: Passing distro into demo-site playbook is consistent

Adding DEPLOYER_DISTRO to the demo-site playbook vars aligns this step with the earlier playbooks and should simplify distro-specific behavior in Ansible.


206-215: IPv6 URL bracketing fixed; double-check host:port assumptions

Wrapping any host containing : in brackets correctly handles IPv6 literals, including ::1. This assumes $server->host never carries an IPv4 host:port (e.g. example.com:8080), which would otherwise become http://[example.com:8080]. If the domain model guarantees “host-only” here, you’re good; if not, you may want a more explicit IPv6 detection or separate port field.


236-247: Command replay now covers PHP extensions and conditional default

Including php-extensions in $replayOptions and only emitting php-default when the “default” decision path was actually in play ($phpDefaultPrompted) makes the replay command accurately reflect user choices without cluttering the happy-path case.


251-267: Single-use installPhp helper is justified despite the guideline

While installPhp() is technically a single-use private method, its size, branching, and dedicated responsibility (version discovery, extension selection, default handling, playbook invocation, and replay metadata) justify keeping it extracted for readability and testability rather than inlining into execute().


378-405: Default-version handling is backward-compatible and intuitive

The logic that auto-defaults the first PHP install, detects when the chosen version is already the default, and otherwise delegates to php-default (option or prompt) is clear and respects both the old php.default format and the newer php.versions structure.


410-444: install-php playbook invocation and return shape are consistent

Re-deriving $distro and $permissions from $info keeps installPhp() self-contained, and the playbook call uses a coherent var set (DEPLOYER_PHP_VERSION, DEPLOYER_PHP_SET_DEFAULT, DEPLOYER_PHP_EXTENSIONS). Returning a structured array alongside Command::SUCCESS integrates cleanly with the caller and replay logic.


458-504: HttpService-based verification is clean and user-oriented

Switching to $this->http->verifyUrl($url) and branching on success / status_code yields clear user messages for network failures, non-200 HTTP responses, and content mismatches, while the nextSteps lines give helpful guidance (and optionally surface the deploy key). Assuming verifyUrl always returns success, status_code, and body, this is a solid improvement over inlined HTTP handling.

…tensions

- Validate CLI-provided PHP version against available versions
- Validate CLI-provided PHP extensions against available extensions for selected version
- Provide clear error messages for invalid inputs instead of generic failures
@loadinglucian
loadinglucian merged commit a68f630 into main Nov 16, 2025
5 checks passed
@loadinglucian
loadinglucian deleted the refactor/server-install-modularization branch November 16, 2025 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant