Refactor to standard classes for stack/container data#53
Open
Refactor to standard classes for stack/container data#53
Conversation
Introduce centralized ContainerInfo and StackInfo PHP classes in util.php that normalize container identity and stack metadata across the codebase. PHP changes: - ContainerInfo: PascalCase->camelCase normalization, fromDockerInspect(), fromUpdateResponse(), mergeUpdateStatus(), toArray(), toUpdateArray(), automatic isPinned derivation from @sha256: in image reference - StackInfo: eager identity (project, sanitizedName, path, composeSource, composeFilePath, isIndirect, overrideInfo) with lazy metadata getters (getName, getDescription, getEnvFilePath, getIconUrl, getWebUIUrl, getDefaultProfiles, getAutostart, getStartedAt, getProfiles), getDefinedServices(), buildComposeArgs(), pruneOrphanOverrideServices() Migrated PHP callers: - exec.php: getStackContainers, checkStackUpdates, checkAllStacksUpdates - exec_functions.php: buildComposeArgs() now thin deprecated wrapper - compose_util_functions.php: echoComposeCommand, echoComposeCommandMultiple - compose_list.php: all inline metadata reads replaced with StackInfo - dashboard_stacks.php: metadata reads replaced with StackInfo JS changes: - Added createContainerInfo(), createStackInfo(), mergeStackUpdateStatus() factory functions for consistent client-side normalization - Migrated buildStackInfoFromCache() to use createStackInfo() - Migrated checkStackUpdates() inline construction to createStackInfo() - Migrated updateParentStackFromContainers() to mergeStackUpdateStatus() - Replaced both PascalCase normalization blocks with createContainerInfo() - Updated mergeUpdateStatus() to use createContainerInfo() for name matching Tests: - ContainerInfoTest: 22 tests covering both factories, merge, serialization - StackInfoTest: 33 tests covering identity, compose resolution, metadata, caching, and buildComposeArgs - All 304 existing tests pass (0 failures, 7 pre-existing skips)
- Remove unnecessary Object.assign merging of PascalCase originals - Align JS createContainerInfo shell default to '/bin/bash' - Add @ to file_get_contents in StackInfo constructor - Make OverrideInfo::getDefinedServices() private - Fix htmlspecialchars(null) deprecation in compose_list.php - Remove extra blank doc-comment lines in OverrideInfo::resolve()
- Add OverrideInfo::fromStackInfo() as primary factory that accepts pre-resolved StackInfo fields, eliminating duplicate filesystem I/O - Extract resolveOverride() as shared core for both factory paths - Deprecate OverrideInfo::fromStack() (kept for backward compat) - Remove $composeRoot, getProjectPath(), getDefinedServices() from OverrideInfo — these were stack-level concerns - Simplify pruneOrphanServices() to accept string[] instead of resolving services internally - Add getMainFileServices() to StackInfo for pruning without override - Replace remaining OverrideInfo::fromStack() calls in exec.php with StackInfo::fromProject() - Fix addStack passing display name instead of folder basename - Add StackInfo::clearCache() to test setUp methods
Move stack directory creation logic from the exec.php addStack handler into a new StackInfo::createNew() static factory method. This centralizes folder naming, collision avoidance, compose/indirect file wiring, metadata writes, and override initialization inside the domain class that already models these artifacts. - Add StackInfo::createNew() with folder sanitization, collision handling, indirect support, and metadata file creation - Move sanitizeFolderName() to util.php (backward-compat stub in exec_functions.php) - Reduce addStack case to input validation + factory call + JSON response - Add 10 unit tests covering createNew() scenarios (basic, indirect, collision, description, caching, override init)
- Prevent compounded random suffixes on folder name collisions by always appending a single random suffix to the base name - Add safety cap to collision attempts to avoid infinite loops - Validate empty and whitespace stack names, throwing clear exceptions - Add tests for empty name, whitespace name, and collision suffix behavior
…nd improved HTML escaping
There was a problem hiding this comment.
Pull request overview
This PR refactors Compose Manager’s stack/container metadata handling away from ad-hoc arrays into standardized PHP data classes (StackInfo, ContainerInfo, and enhanced OverrideInfo), then wires those objects through exec and UI flows while expanding PHPUnit coverage.
Changes:
- Introduces
StackInfoandContainerInfoinutil.php, plus override-service pruning utilities and enhancedOverrideInfofactories. - Refactors multiple execution/UI paths (exec endpoints, compose command builders, list/dashboard rendering) to use
StackInfoand normalized container output. - Adds/expands PHPUnit tests for stack/container/override behavior and regression coverage (override pruning, caching, pinned digest display).
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/UtilTest.php | Adds unit tests for pruning orphaned override services content. |
| tests/unit/StackInfoTest.php | New comprehensive tests for StackInfo identity, metadata, caching, compose args, and createNew(). |
| tests/unit/OverrideInfoTest.php | Adds composeFilePath resolution tests and pruneOrphanServices tests; clears StackInfo cache in setup. |
| tests/unit/ExecActionsTest.php | Ensures StackInfo cache is cleared between tests. |
| tests/unit/ContainerInfoTest.php | New tests covering normalization, update merge behavior, and serialization. |
| source/compose.manager/php/util.php | Adds StackInfo/ContainerInfo, override pruning, sanitizeFolderName, and OverrideInfo refactor. |
| source/compose.manager/php/exec_functions.php | Deprecates legacy helpers and routes buildComposeArgs through StackInfo. |
| source/compose.manager/php/exec.php | Uses StackInfo/ContainerInfo in addStack + container/status/update flows; normalizes container output. |
| source/compose.manager/php/dashboard_stacks.php | Switches stack metadata reads to StackInfo getters. |
| source/compose.manager/php/compose_util_functions.php | Uses StackInfo for compose args; prunes orphaned override services before up. |
| source/compose.manager/php/compose_manager_main.php | Adds JS factory/merge helpers for normalized container/stack objects and updates call sites. |
| source/compose.manager/php/compose_list.php | Uses StackInfo for metadata and defined service counts; simplifies file/env resolution. |
| source/compose.manager/compose.manager.dashboard.page | Improves client tolerance for camelCase/PascalCase fields and safer ID handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+67
to
+76
| function sanitizeFolderName(string $stackName): string | ||
| { | ||
| $folderName = str_replace('"', '', $stackName); | ||
| $folderName = str_replace("'", '', $folderName); | ||
| $folderName = str_replace('&', '', $folderName); | ||
| $folderName = str_replace('(', '', $folderName); | ||
| $folderName = str_replace(')', '', $folderName); | ||
| $folderName = preg_replace('/ {2,}/', ' ', $folderName); | ||
| $folderName = preg_replace('/\s/', '_', $folderName); | ||
| return $folderName; |
There was a problem hiding this comment.
sanitizeFolderName() does not remove path separators or dot-segments (e.g., "../"), so StackInfo::createNew() can create directories outside $composeRoot when stackName is user-controlled (addStack POST). Please restrict folder names to a safe character set (or explicitly reject "..", "/", "\") and verify the final resolved path stays under $composeRoot.
Comment on lines
+1059
to
+1062
| // 0. Validate stack name is not empty | ||
| if (trim($stackName) === '') { | ||
| throw new \RuntimeException("Stack name cannot be empty."); | ||
| } |
There was a problem hiding this comment.
StackInfo::createNew() throws "Stack name cannot be empty." (with a trailing period), but the new unit tests expect "Stack name cannot be empty". This will cause test failures unless the message or the tests are aligned.
Comment on lines
+977
to
+982
| public function buildComposeArgs(): array | ||
| { | ||
| $composeFile = $this->composeFilePath ?? ($this->composeSource . '/compose.yaml'); | ||
|
|
||
| $files = "-f " . escapeshellarg($composeFile); | ||
|
|
There was a problem hiding this comment.
StackInfo::buildComposeArgs() falls back to "$this->composeSource . '/compose.yaml'" when composeFilePath is null. Elsewhere the codebase uses COMPOSE_FILE_NAMES[0] for the default compose filename; using a hard-coded value can diverge if the preferred default changes.
Comment on lines
+55
to
+60
| try { | ||
| $stack = StackInfo::createNew($compose_root, $stackName, $stackDesc, $indirect); | ||
| } catch (\RuntimeException $e) { | ||
| clientDebug('[stack] Failed to create stack: ' . $e->getMessage(), null, 'daemon', 'error'); | ||
| echo json_encode(['result' => 'error', 'message' => $e->getMessage()]); | ||
| break; |
There was a problem hiding this comment.
In addStack, the JSON error response returns the raw RuntimeException message to the client. Some of these messages include filesystem paths (e.g., "Failed to create stack directory: ..."), which is better kept in logs; consider returning a generic user-facing message and logging the detailed exception server-side.
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
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
This branch introduces standard PHP data classes for stack and container metadata, wires them through the compose manager execution paths, and adds broad unit test coverage. It also includes follow-up fixes for regressions found during the refactor, including container discovery and pinned digest display.
What changed
Added standard classes for core domain objects:
Tests
Added and expanded PHPUnit coverage for: