feat: v2 containers by loks0n · Pull Request #63 · utopia-php/orchestration

loks0n · 2025-11-23T08:01:23Z

Summary by CodeRabbit

Release Notes

New Features
- Container management API supporting run, execute, list, and remove operations
- Network management capabilities including create, remove, list, connect, and disconnect
- Container usage and statistics monitoring
- Restart policy configuration options
- Mount support for Bind, Volume, and Tmpfs configurations
- DockerCLI and DockerAPI adapter implementations
Breaking Changes
- Project renamed from "Utopia Orchestration" to "Utopia Containers"
- Package name changed to utopia-php/containers
- Namespace updated to Utopia\Containers
- Complete API redesign with restructured class hierarchy

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-11-23T08:01:31Z

Walkthrough

This pull request comprehensively refactors the library from "Utopia Orchestration" to "Utopia Containers," involving a complete namespace migration, API restructuring, and reorganization of public interfaces. The old Orchestration class, adapters, exceptions, and data models are removed and replaced with a new Containers facade, abstract Adapter interface, concrete DockerAPI and DockerCLI adapters, a suite of new exception types, value objects (Container, Network, Usage, Mount variants), and wrapper classes (Networks). Configuration files and documentation are updated to reflect the new package name, namespace, and API. Tests are reorganized from tests/Orchestration to tests/Containers. The underlying container and network management functionality remains operationally similar but is restructured with different public method signatures, validation logic, and error handling patterns.

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Areas requiring extra attention:

src/Containers/Adapter/DockerCLI.php — Substantial new implementation with complex Docker CLI argument construction, output parsing (IO stats, network listing, container filtering), error handling across multiple paths, timeout handling, login authentication, and mount volume configuration. Requires careful validation of command construction, parsing logic, and exception handling.
src/Containers/Containers.php — Wrapper class with validation logic (name/image format checks) and environment variable sanitization. Review the sanitization patterns and validation boundaries to ensure correctness.
src/Containers/Adapter.php — Abstract interface defining the contract for all adapters. Verify method signatures, parameter types, return types, and docblock accuracy are consistent with implementations.
composer.json — Package metadata and namespace mapping changes; verify autoload PSR-4 paths (Utopia\Containers) and new dependency (utopia-php/console 0.0.*) are correct.
tests/Containers/Base.php — Abstract test base class that all adapter tests extend. Ensure resource paths, setup/teardown logic, and test patterns correctly exercise the new Containers API.
Mount interface and implementations (Bind, Volume, Tmpfs) — New mount type abstraction; verify consistency with DockerCLI's mount handling in the run method.
Exception hierarchy — Five new exception types with specific roles (ContainersAuthException, ContainersDuplicateException, ContainersNotFoundException, ContainersTimeoutException); verify they are thrown appropriately throughout adapters and used correctly in tests.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 58.70% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'feat: v2 containers' clearly summarizes the primary change: a major version overhaul from Orchestration to Containers, including complete namespace restructuring, new class definitions, and removal of legacy Orchestration code.

✨ 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 feat-v2-containers

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

Provide your own instructions using the high_level_summary_instructions setting.
Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

📝 Description — Summarize the main change in 50–60 words, explaining what was done.

📓 References — List relevant issues, discussions, documentation, or related PRs.

📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.

📊 Contributor Summary — Include a Markdown table showing contributions:
| Contributor | Lines Added | Lines Removed | Files Changed |

✔️ Additional Notes — Add any extra reviewer context.
Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

coderabbitai

Actionable comments posted: 20

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

README.md (1)
169-183: Update README.md example to use correct variable name and method signature

The example uses $orchestraton->execute(...) (should be $containers) and incorrectly shows $stdout and $stderr as separate reference parameters. The actual Containers::execute() method signature (src/Containers/Containers.php:131-137) accepts a single string &$output parameter, not two separate streams.

Update the example to:
$output = '';

$containers->execute(
    'container_id',
    ['echo', 'Hello World!'],
    $output,
    ['VAR' => 'VALUE'],
    10,
)

♻️ Duplicate comments (1)

tests/Containers/Adapter/DockerCLITest.php (1)

11-32: Same optional static property typing consideration as in DockerAPITest

This class mirrors DockerAPITest; the same optional refactor to type‑hint public static ?Containers $containers instead of using only phpdoc would apply here as well, but it’s not required.

🧹 Nitpick comments (7)

tests/Containers/Adapter/DockerAPITest.php (1)
11-32: Optional: type‑hint the static $containers property

Implementation is fine and mirrors DockerCLITest. If you’re on PHP 8+, you could rely on a typed static property instead of the phpdoc, but this is purely optional:
-    /**
-     * @var Containers|null
-     */
-    public static $containers = null;
+    public static ?Containers $containers = null;
src/Containers/Adapter/DockerAPI.php (1)

183-189: Curl timeout vs generic failure exception types are good, but consider using the new timeout exception consistently

You correctly distinguish cURL timeouts from other errors, throwing Timeout on CURLE_OPERATION_TIMEOUTED and Containers otherwise (Lines 183–188). That matches the new exception hierarchy.

Once the adapter contract is stabilized, ensure all public methods that can time out (e.g., run, execute, getStats) surface Timeout or a dedicated ContainersTimeoutException consistently, so callers have a single way to detect timeouts across adapters.
src/Containers/Networks.php (1)
15-37: Networks wrapper delegates correctly but return types could be explicit

The methods create(), remove(), list(), connect(), disconnect(), and exists() cleanly forward to the underlying adapter, which is good.

Two minor improvements:

Add explicit : void return types to create() and remove() for clarity and consistency with the abstract Adapter signatures.

Consider adding a return type for parseIOStats style helpers in other files; here list() and disconnect()/exists() are already typed.

Example:
public function create(string $name, bool $internal = false): void
{
    $this->adapter->createNetwork($name, $internal);
}

public function remove(string $name): void
{
    $this->adapter->removeNetwork($name);
}
Also applies to: 46-57, 62-65
src/Containers/Adapter/DockerCLI.php (1)

371-399: Container run/execute/remove implementations look solid, with minor robustness nits

The core container lifecycle methods using the Docker CLI (run, execute, remove) are generally well-structured:

run builds a docker run -d command with support for --rm, network, entrypoint (as array), CPU/memory/swap limits, restart policy via RestartPolicy enum, container name, mounts (Bind/Volume/Tmpfs), labels, workdir, hostname, and environment variables.

execute escapes env values, handles timeout via exit code 124 mapped to ContainersTimeoutException, and wraps other non‑zero results in ContainersException.

remove uses docker rm with optional --force and checks both output content and return code.

Two optional improvements:

In run, consider quoting/escaping the container name and hostname (currently interpolated directly into "--name={$name}" and --hostname), to avoid issues with unexpected characters (name validation is done earlier in Containers::run, so this may be acceptable, but explicit safety doesn’t hurt).

In remove, you already trust that str_starts_with($output, $name) and lack of “No such container” implies success, so returning !$result is fine; just ensure this convention is documented since it differs slightly from other methods that treat any non‑zero code as error.

Overall, this implementation is a good fit for the new adapter contract once the remaining signature issues are resolved.

Also applies to: 435-438, 485-486, 505-511
src/Containers/Containers.php (3)
7-7: Consider making the $networks property readonly.

The public property allows reassignment from external code. Since Networks is instantiated once in the constructor, marking it readonly would prevent accidental reassignment.

Apply this diff:
-    public Networks $networks;
+    public readonly Networks $networks;
47-50: Consider adding validation for the $name parameter.

The remove method doesn't validate the container name, while the run method has thorough validation (lines 91-96). For consistency and to provide clearer error messages, consider adding similar validation here.

Apply this diff:
 public function remove(string $name, bool $force = false): bool
 {
+    if (empty($name)) {
+        throw new \InvalidArgumentException('Container name cannot be empty');
+    }
     return $this->adapter->remove($name, $force);
 }
131-139: Consider adding validation for the $container parameter.

For consistency with other methods, consider validating that the container name is not empty.

Apply this diff:
 public function execute(
     string $container,
     array $command,
     string &$output,
     array $vars = [],
     int $timeout = -1
 ): bool {
+    if (empty($container)) {
+        throw new \InvalidArgumentException('Container name cannot be empty');
+    }
     return $this->adapter->execute($container, $command, $output, $vars, $timeout);
 }

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e8d8d04 and 1787eb8.

📒 Files selected for processing (35)

.gitignore (1 hunks)
README.md (11 hunks)
composer.json (2 hunks)
docker-compose.yml (1 hunks)
src/Containers/Adapter.php (1 hunks)
src/Containers/Adapter/DockerAPI.php (14 hunks)
src/Containers/Adapter/DockerCLI.php (1 hunks)
src/Containers/Container.php (1 hunks)
src/Containers/Containers.php (1 hunks)
src/Containers/Exception/ContainersAuthException.php (1 hunks)
src/Containers/Exception/ContainersDuplicateException.php (1 hunks)
src/Containers/Exception/ContainersException.php (1 hunks)
src/Containers/Exception/ContainersNotFoundException.php (1 hunks)
src/Containers/Exception/ContainersTimeoutException.php (1 hunks)
src/Containers/Mount.php (1 hunks)
src/Containers/Mount/Bind.php (1 hunks)
src/Containers/Mount/Tmpfs.php (1 hunks)
src/Containers/Mount/Volume.php (1 hunks)
src/Containers/Network.php (1 hunks)
src/Containers/Networks.php (1 hunks)
src/Containers/RestartPolicy.php (1 hunks)
src/Containers/Usage.php (1 hunks)
src/Orchestration/Adapter.php (0 hunks)
src/Orchestration/Adapter/DockerCLI.php (0 hunks)
src/Orchestration/Container.php (0 hunks)
src/Orchestration/Container/Stats.php (0 hunks)
src/Orchestration/Exception/Orchestration.php (0 hunks)
src/Orchestration/Exception/Timeout.php (0 hunks)
src/Orchestration/Network.php (0 hunks)
src/Orchestration/Orchestration.php (0 hunks)
tests/Containers/Adapter/DockerAPITest.php (1 hunks)
tests/Containers/Adapter/DockerCLITest.php (1 hunks)
tests/Containers/Base.php (38 hunks)
tests/Orchestration/Adapter/DockerAPITest.php (0 hunks)
tests/Orchestration/Adapter/DockerCLITest.php (0 hunks)

💤 Files with no reviewable changes (10)

src/Orchestration/Container/Stats.php
src/Orchestration/Orchestration.php
src/Orchestration/Network.php
src/Orchestration/Exception/Timeout.php
src/Orchestration/Adapter/DockerCLI.php
tests/Orchestration/Adapter/DockerAPITest.php
src/Orchestration/Adapter.php
tests/Orchestration/Adapter/DockerCLITest.php
src/Orchestration/Exception/Orchestration.php
src/Orchestration/Container.php

🧰 Additional context used

🧬 Code graph analysis (20)

src/Containers/Exception/ContainersNotFoundException.php (1)

src/Containers/Exception/ContainersException.php (1)

ContainersException (5-7)

src/Containers/Mount/Bind.php (4)

src/Containers/Containers.php (2)

Containers (5-162)

__construct (12-15)

src/Containers/Mount/Tmpfs.php (3)

__construct (8-11)

getTarget (13-13)

isReadOnly (14-14)

src/Containers/Mount/Volume.php (3)

__construct (8-12)

getTarget (14-14)

isReadOnly (15-15)

src/Containers/Mount.php (2)

getTarget (6-6)

isReadOnly (7-7)

src/Containers/RestartPolicy.php (1)

src/Containers/Containers.php (1)

Containers (5-162)

src/Containers/Exception/ContainersAuthException.php (1)

src/Containers/Exception/ContainersException.php (1)

ContainersException (5-7)

src/Containers/Exception/ContainersTimeoutException.php (2)

src/Containers/Containers.php (1)

Containers (5-162)

src/Containers/Exception/ContainersException.php (1)

ContainersException (5-7)

src/Containers/Network.php (2)

src/Containers/Container.php (1)

__construct (10-16)

src/Containers/Networks.php (1)

__construct (7-9)

src/Containers/Exception/ContainersDuplicateException.php (2)

src/Containers/Containers.php (1)

Containers (5-162)

src/Containers/Exception/ContainersException.php (1)

ContainersException (5-7)

src/Containers/Container.php (1)

src/Containers/Containers.php (2)

Containers (5-162)

__construct (12-15)

src/Containers/Mount.php (4)

src/Containers/Containers.php (1)

Containers (5-162)

src/Containers/Mount/Bind.php (2)

getTarget (14-14)

isReadOnly (15-15)

src/Containers/Mount/Tmpfs.php (2)

getTarget (13-13)

isReadOnly (14-14)

src/Containers/Mount/Volume.php (2)

getTarget (14-14)

isReadOnly (15-15)

tests/Containers/Adapter/DockerAPITest.php (4)

src/Containers/Containers.php (1)

Containers (5-162)

src/Containers/Adapter/DockerAPI.php (1)

DockerAPI (13-648)

tests/Containers/Base.php (3)

Base (9-703)

getAdapterName (13-13)

getContainers (11-11)

tests/Containers/Adapter/DockerCLITest.php (2)

getAdapterName (19-22)

getContainers (24-33)

src/Containers/Usage.php (1)

src/Containers/Containers.php (2)

Containers (5-162)

__construct (12-15)

src/Containers/Mount/Tmpfs.php (3)

src/Containers/Mount/Bind.php (3)

__construct (8-12)

getTarget (14-14)

isReadOnly (15-15)

src/Containers/Mount/Volume.php (3)

__construct (8-12)

getTarget (14-14)

isReadOnly (15-15)

src/Containers/Mount.php (2)

getTarget (6-6)

isReadOnly (7-7)

tests/Containers/Adapter/DockerCLITest.php (5)

src/Containers/Adapter.php (1)

Adapter (7-68)

src/Containers/Containers.php (1)

Containers (5-162)

src/Containers/Adapter/DockerCLI.php (1)

DockerCLI (21-513)

tests/Containers/Base.php (3)

Base (9-703)

getAdapterName (13-13)

getContainers (11-11)

tests/Containers/Adapter/DockerAPITest.php (2)

getAdapterName (19-22)

getContainers (24-33)

src/Containers/Mount/Volume.php (3)

src/Containers/Mount/Bind.php (3)

__construct (8-12)

getTarget (14-14)

isReadOnly (15-15)

src/Containers/Mount/Tmpfs.php (3)

__construct (8-11)

getTarget (13-13)

isReadOnly (14-14)

src/Containers/Mount.php (2)

getTarget (6-6)

isReadOnly (7-7)

src/Containers/Adapter.php (5)

src/Containers/Containers.php (8)

Containers (5-162)

getUsage (147-150)

listUsage (158-161)

pull (23-26)

list (34-37)

run (74-123)

remove (47-50)

execute (131-139)

src/Containers/Usage.php (1)

Usage (5-19)

src/Containers/Adapter/DockerAPI.php (9)

createNetwork (204-223)

removeNetwork (228-239)

networkExists (287-292)

listNetworks (388-412)

pull (417-430)

list (438-473)

run (486-571)

remove (638-647)

execute (579-633)

src/Containers/Adapter/DockerCLI.php (13)

createNetwork (50-61)

removeNetwork (63-74)

networkExists (108-117)

connect (76-87)

disconnect (89-103)

listNetworks (243-271)

getUsage (119-134)

listUsage (136-186)

pull (273-284)

list (292-336)

run (350-455)

remove (501-512)

execute (463-496)

src/Containers/Networks.php (4)

connect (46-49)

disconnect (54-57)

list (34-37)

remove (24-27)

src/Containers/Adapter/DockerAPI.php (2)

src/Containers/Containers.php (1)

Containers (5-162)

src/Containers/Adapter.php (1)

Adapter (7-68)

src/Containers/Adapter/DockerCLI.php (9)

src/Containers/Containers.php (9)

Containers (5-162)

__construct (12-15)

execute (131-139)

getUsage (147-150)

listUsage (158-161)

list (34-37)

pull (23-26)

run (74-123)

remove (47-50)

src/Containers/Adapter.php (14)

Adapter (7-68)

execute (62-62)

createNetwork (9-9)

removeNetwork (11-11)

connect (15-15)

disconnect (17-17)

networkExists (13-13)

getUsage (24-24)

listUsage (30-30)

listNetworks (22-22)

list (38-38)

pull (32-32)

run (40-56)

remove (67-67)

src/Containers/Usage.php (2)

Usage (5-19)

__construct (12-18)

src/Containers/Exception/ContainersAuthException.php (1)

ContainersAuthException (5-7)

src/Containers/Exception/ContainersException.php (1)

ContainersException (5-7)

src/Containers/Exception/ContainersTimeoutException.php (1)

ContainersTimeoutException (5-7)

src/Containers/Mount/Bind.php (3)

Bind (7-16)

__construct (8-12)

isReadOnly (15-15)

src/Containers/Mount/Tmpfs.php (3)

Tmpfs (7-15)

__construct (8-11)

isReadOnly (14-14)

src/Containers/Mount/Volume.php (3)

Volume (7-16)

__construct (8-12)

isReadOnly (15-15)

src/Containers/Containers.php (3)

src/Containers/Networks.php (4)

Networks (5-67)

__construct (7-9)

list (34-37)

remove (24-27)

src/Containers/Adapter/DockerAPI.php (6)

__construct (18-28)

pull (417-430)

list (438-473)

remove (638-647)

run (486-571)

execute (579-633)

src/Containers/Adapter.php (8)

Adapter (7-68)

pull (32-32)

list (38-38)

remove (67-67)

run (40-56)

execute (62-62)

getUsage (24-24)

listUsage (30-30)

src/Containers/Networks.php (2)

src/Containers/Containers.php (4)

Containers (5-162)

__construct (12-15)

remove (47-50)

list (34-37)

src/Containers/Adapter.php (9)

Adapter (7-68)

createNetwork (9-9)

remove (67-67)

removeNetwork (11-11)

list (38-38)

listNetworks (22-22)

connect (15-15)

disconnect (17-17)

networkExists (13-13)

tests/Containers/Base.php (3)

src/Containers/Containers.php (6)

Containers (5-162)

pull (23-26)

run (74-123)

remove (47-50)

execute (131-139)

list (34-37)

src/Containers/Adapter.php (10)

Adapter (7-68)

pull (32-32)

run (40-56)

remove (67-67)

createNetwork (9-9)

listNetworks (22-22)

removeNetwork (11-11)

execute (62-62)

list (38-38)

networkExists (13-13)

src/Containers/Adapter/DockerAPI.php (11)

DockerAPI (13-648)

pull (417-430)

run (486-571)

remove (638-647)

createNetwork (204-223)

listNetworks (388-412)

removeNetwork (228-239)

execute (579-633)

list (438-473)

getStats (300-383)

networkExists (287-292)

🪛 GitHub Actions: Linter

src/Containers/Mount/Bind.php

[error] 1-1: Pint/PSR-12 violations detected: class_definition, braces_position, statements

src/Containers/Network.php

[error] 1-1: Pint/PSR-12 violation detected: braces_position

src/Containers/Mount.php

[error] 1-1: Pint/PSR-12 violation detected: class_definition

src/Containers/Usage.php

[error] 1-1: Pint/PSR-12 violations detected: method_argument_space, braces_position

src/Containers/Mount/Tmpfs.php

[error] 1-1: Pint/PSR-12 violations detected: class_definition, braces_position, statements

src/Containers/Mount/Volume.php

[error] 1-1: Pint/PSR-12 violations detected: class_definition, braces_position, statements

src/Containers/Adapter.php

[error] 1-1: Pint/PSR-12 violations detected: no_unused_imports, blank_line_after_namespace

src/Containers/Adapter/DockerCLI.php

[error] 1-1: Pint/PSR-12 violations detected: no_unused_imports, ordered_imports

src/Containers/Containers.php

[error] 1-1: Pint/PSR-12 violation detected: function_declaration

🪛 LanguageTool

README.md

[grammar] ~9-~9: Use a hyphen to join words.
Context: ...-php/framework) project it is dependency free and can be used as standalone with ...

(QB_NEW_EN_HYPHEN)

[grammar] ~63-~63: Ensure spelling is correct
Context: ... private images from Docker Hub. - ### DockerCLI Uses the Docker CLI to communicate to th...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

🪛 PHPMD (2.15.0)

src/Containers/Adapter/DockerCLI.php

131-131: Avoid unused local variables such as '$stats'. (undefined)

(UnusedLocalVariable)

150-150: Avoid unused local variables such as '$containersString'. (undefined)

(UnusedLocalVariable)

215-215: Avoid unused local variables such as '$value'. (undefined)

(UnusedLocalVariable)

🔇 Additional comments (10)

.gitignore (1)

4-5: Good additions to .gitignore.

Both entries follow standard best practices: .DS_Store should always be ignored in version control (macOS system metadata), and *.phpdisabled appropriately ignores temporary PHP-generated files that may appear during refactoring.

docker-compose.yml (1)

6-9: Network rename and build indentation look correct

The tests service now attaches to the containers network and the build block indentation is fixed; both are consistent with the new naming and have no functional downsides.

Also applies to: 17-17

src/Containers/Container.php (1)

5-16: Readonly Container DTO looks good

The value object shape (name/id/status/labels) and constructor promotion are clear and PSR-12-compliant; no changes needed.

src/Containers/Exception/ContainersAuthException.php (1)

3-7: Auth-specific exception wiring is appropriate

Having a dedicated ContainersAuthException extending ContainersException cleanly separates authentication failures from other container errors; implementation is fine as-is.

src/Containers/Exception/ContainersException.php (1)

3-7: Base ContainersException is a solid root for the hierarchy

Using a single ContainersException base type for all container-related errors is idiomatic and keeps catching/handling straightforward.

src/Containers/Exception/ContainersTimeoutException.php (1)

3-7: Timeout-specific exception is well-scoped

ContainersTimeoutException cleanly models timeout cases as a dedicated subtype of ContainersException; no further implementation is necessary.

src/Containers/Exception/ContainersNotFoundException.php (1)

3-7: Not-found exception aligns with the new hierarchy

ContainersNotFoundException is a useful, specific subtype of ContainersException for missing resources; the minimal implementation is sufficient.

src/Containers/RestartPolicy.php (1)

5-10: Enum definition looks solid

Enum cases map cleanly to Docker restart policies and integrate well with the Containers::run signature. No changes needed.

src/Containers/Exception/ContainersDuplicateException.php (1)

5-7: Marker exception looks good

Dedicated ContainersDuplicateException cleanly extends the shared base and keeps the hierarchy consistent. No changes needed.

src/Containers/Adapter/DockerCLI.php (1)

5-18: Rewritten comment is not applicable - the review comment is incorrect

The imports are already correctly ordered alphabetically. Additionally, Mount is actually used in the file (type hint in docstring at line 347), and only TmpfsMount and VolumeMount are genuinely unused. The review's assessment of which imports are unused and the ordering issue are both inaccurate.

Likely an incorrect or invalid review comment.

coderabbitai · 2025-11-23T08:12:26Z

composer.json

+    "name": "utopia-php/containers",
+    "description": "Lite & fast micro PHP abstraction library for container containers",
    "type": "library",
-    "keywords": ["php","framework", "upf", "utopia", "orchestration", "docker", "swarm", "kubernetes"],
+    "keywords": ["php","framework", "upf", "utopia", "containers", "docker", "swarm", "kubernetes"],
    "license": "MIT",


⚠️ Potential issue | 🟡 Minor

Minor metadata fixes: description wording and consistency

The composer.json changes generally look good (namespace and test paths line up with the new Containers layout), but there is one small issue:

"description": "Lite & fast micro PHP abstraction library for container containers" has a duplicated word. Consider changing to something like "Lite & fast micro PHP abstraction library for containers" or "for container orchestration".

Otherwise the autoload mappings and keywords are consistent with the new namespace.

Also applies to: 14-19

🤖 Prompt for AI Agents

In composer.json around lines 2-6 (and similarly lines 14-19), the package description contains a duplicated word "container containers"; update the "description" value to remove the duplication and improve clarity (e.g., "Lite & fast micro PHP abstraction library for containers" or "Lite & fast micro PHP abstraction library for container orchestration") so the metadata reads cleanly and consistently.

coderabbitai · 2025-11-23T08:12:26Z

README.md

+Utopia framework containers library is simple and lite library for abstracting the interaction with multiple container orchestrators. This library is aiming to be as simple and easy to learn and use. This library is maintained by the [Appwrite team](https://appwrite.io).

 Although this library is part of the [Utopia Framework](https://github.com/utopia-php/framework) project it is dependency free and can be used as standalone with any other PHP project or framework.


⚠️ Potential issue | 🟡 Minor

Minor language fixes in introduction

Two small text issues flagged by tooling:

“it is dependency free and can be used as standalone” → “it is dependency‑free and can be used as a standalone library”.

Consider re‑reading the surrounding paragraph for typos after the DockerCLI section; the linter flagged an orthography issue there.

Not critical, but easy polish for the new package branding.

🧰 Tools

🪛 LanguageTool

[grammar] ~9-~9: Use a hyphen to join words.
Context: ...-php/framework) project it is dependency free and can be used as standalone with ...

(QB_NEW_EN_HYPHEN)

🤖 Prompt for AI Agents

In README.md around lines 7 to 9, update the phrasing to fix minor language issues: change “it is dependency free and can be used as standalone” to “it is dependency‑free and can be used as a standalone library”, and review the surrounding paragraph (especially after the DockerCLI section) for any remaining typos or orthography issues and correct them for consistency and clarity.

coderabbitai · 2025-11-23T08:12:26Z

README.md

+Utopia framework containers library is simple and lite library for abstracting the interaction with multiple container orchestrators. This library is aiming to be as simple and easy to learn and use. This library is maintained by the [Appwrite team](https://appwrite.io).

 Although this library is part of the [Utopia Framework](https://github.com/utopia-php/framework) project it is dependency free and can be used as standalone with any other PHP project or framework.

 ## Getting Started

 Install using composer:
 ```bash
-composer require utopia-php/orchestration
+composer require utopia-php/containers
 ```


⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

README still documents v1 pull() and run() semantics

There are a couple of important drifts between the README and the v2 code:

The “Pulling an image” section (Lines 75–80) still says the method “will return a boolean value indicating if the image was pulled successfully,” but the new Containers/Adapter contract is moving toward a void/exception model while DockerAPI::pull() still returns bool. This needs to be clarified once you settle on the final contract.

The “Running a container” example (Lines 95–112) continues to use the old positional run signature (image, name, command, entrypoint, workdir, volumes, env, mountFolder, labels, hostname, remove), while the new adapter contract uses run(string $name, string $image, string $hostname, array $command, array $entrypoint, ?string $workdir, array $environment, array $mounts, array $labels, ?string $network, ?float $cpus, ?int $memory, ?int $swap, RestartPolicy $restart, bool $remove) and Mount objects instead of raw volume strings.

Once the v2 API is finalized, please update these sections to match the actual Containers::pull() and Containers::run() signatures and semantics (including examples using Bind/Volume/Tmpfs and RestartPolicy).

Also applies to: 75-112

I'll verify the review comment by checking the actual README content and comparing it with the current API implementation.

🏁 Script executed:

#!/bin/bash # Read the full README to see lines 75-112 cat README.md | head -120 | tail -50

Length of output: 1208

🏁 Script executed:

#!/bin/bash # Search for Containers class and pull/run method signatures find . -type f -name "*.php" | head -20

Length of output: 789

🏁 Script executed:

#!/bin/bash # Look for the main Containers class file fd -t f "\.php$" -x grep -l "class Containers" {} \;

Length of output: 314

🏁 Script executed:

#!/bin/bash # Search for RestartPolicy, Mount, Volume classes rg -nP "class (RestartPolicy|Mount|Volume|Tmpfs|Bind)" --type php

Length of output: 236

🏁 Script executed:

#!/bin/bash # Find method signatures for pull and run rg -nP "function (pull|run)\s*\(" --type php -A 3 -B 1

Length of output: 2371

I need to see the complete run() method signature to verify the parameter structure.

🏁 Script executed:

#!/bin/bash # Get full run() signature from Containers.php sed -n '74,95p' src/Containers/Containers.php

Length of output: 850

🏁 Script executed:

#!/bin/bash # Get full run() signature from Adapter.php sed -n '40,65p' src/Containers/Adapter.php

Length of output: 660

🏁 Script executed:

#!/bin/bash # Check RestartPolicy class cat src/Containers/RestartPolicy.php

Length of output: 193

🏁 Script executed:

#!/bin/bash # Check Mount interface cat src/Containers/Mount.php

Length of output: 137

🏁 Script executed:

#!/bin/bash # Check Bind class structure cat src/Containers/Mount/Bind.php

Length of output: 411

🏁 Script executed:

#!/bin/bash # Get full DockerAPI run() signature for comparison sed -n '486,510p' src/Containers/Adapter/DockerAPI.php

Length of output: 847

README documentation drift confirmed; also note DockerAPI implementation inconsistency

The review comment is correct. The README still documents the v1 API while the Containers/Adapter contract has evolved to v2:

Lines 75–80 claim pull() "will return a boolean value," but Containers::pull(string): void and Adapter::pull(string): void now return nothing (throws exceptions on error).

Lines 95–112 use the old run() signature with positional parameters (image, name, command, entrypoint, workdir, volumes, env, mountFolder, labels, hostname, remove) and raw volume strings. The new contract is run(string $name, string $image, string $hostname, array $command, array $entrypoint, ?string $workdir, array $environment, array $mounts, array $labels, ?string $network, ?float $cpus, ?int $memory, ?int $swap, RestartPolicy $restart, bool $remove): string, which uses Mount objects (Bind, Volume, Tmpfs) for $mounts and RestartPolicy enum for $restart.

Also note: DockerAPI implementation (src/Containers/Adapter/DockerAPI.php) does not conform to the Adapter contract—it still has the old signatures (pull() returns bool, run() takes image as the first parameter instead of name). This incomplete refactoring should be resolved alongside the README update.

Update README lines 75–112 to document the current v2 API, and ensure DockerAPI implements the Adapter contract correctly.

🧰 Tools

🪛 LanguageTool

[grammar] ~9-~9: Use a hyphen to join words.
Context: ...-php/framework) project it is dependency free and can be used as standalone with ...

(QB_NEW_EN_HYPHEN)

🤖 Prompt for AI Agents

README.md lines 7-16: the README documents the old v1 Containers API and mismatches the current v2 contract; update the README to reflect v2 signatures: note that pull(string) now returns void and throws on error (remove any boolean return descriptions), replace the old run(...) positional signature with the v2 definition run(string $name, string $image, string $hostname, array $command, array $entrypoint, ?string $workdir, array $environment, array $mounts, array $labels, ?string $network, ?float $cpus, ?int $memory, ?int $swap, RestartPolicy $restart, bool $remove): string and describe that mounts are Mount objects (Bind, Volume, Tmpfs) and restart is a RestartPolicy enum, update all examples/snippets to use named/typed parameters and Mount objects and to expect a string container id on success; concurrently fix src/Containers/Adapter/DockerAPI.php to match the Adapter contract (make pull return void and throw on error, change run signature to accept name first and the new typed parameters, accept Mount objects and RestartPolicy, and return the container id string) so implementation and README are consistent.

coderabbitai · 2025-11-23T08:12:27Z

src/Containers/Adapter.php

+namespace Utopia\Containers;
+
+use Utopia\Containers\Usage;
+
+abstract class Adapter


⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

Adapter contract mostly looks good, but clean up imports and ensure getUsage is implemented consistently

A few points on this new abstraction:

use Utopia\Containers\Usage; is redundant in the same namespace and flagged by Pint (no_unused_imports); you can drop the use and keep the unqualified Usage type hint.

getUsage(string $container): ?Usage is part of the contract, but DockerCLI::getUsage currently returns array and throws a placeholder \Exception('getUsage'). That will be incompatible with this signature and effectively unimplemented.

disconnect(string $container, string $network, bool $force) has no declared return type, but Networks::disconnect() returns bool based on the adapter result. For clarity, consider adding : bool here and in all implementations.

After fixing the import, aligning getUsage across adapters (returning a Usage|null or throwing on error), and making disconnect’s return type explicit, this interface will provide a solid v2 contract for adapters.

Also applies to: 19-31

🏁 Script executed:

#!/bin/bash # View Adapter.php file to see full contract cat src/Containers/Adapter.php

Length of output: 1785

🏁 Script executed:

#!/bin/bash # Find and view DockerCLI implementation find . -name "DockerCLI.php" -o -name "DockerAPI.php" | head -5

Length of output: 77

🏁 Script executed:

#!/bin/bash # Find Networks class find . -name "Networks.php" | head -5

Length of output: 29

🏁 Script executed:

#!/bin/bash # Search for other adapter implementations find . -path "*/Containers/*" -name "*.php" -type f | grep -i adapter

Length of output: 196

🏁 Script executed:

#!/bin/bash # Check PHP version and Pint configuration for linting ls -la | grep -E 'pint|phpstan|composer'

Length of output: 187

🏁 Script executed:

#!/bin/bash # View DockerCLI implementation cat src/Containers/Adapter/DockerCLI.php

Length of output: 14679

🏁 Script executed:

#!/bin/bash # View DockerAPI implementation cat src/Containers/Adapter/DockerAPI.php

Length of output: 21754

🏁 Script executed:

#!/bin/bash # View Networks class cat src/Containers/Networks.php

Length of output: 1350

🏁 Script executed:

#!/bin/bash # Check DockerAPI class declaration and inheritance head -20 src/Containers/Adapter/DockerAPI.php

Length of output: 508

🏁 Script executed:

#!/bin/bash # Check pint configuration cat pint.json

Length of output: 391

Fix Adapter contract: clean import, add return types, align getUsage implementation across adapters

Three confirmed issues:

use Utopia\Containers\Usage; (line 5) is redundant in the same namespace and flagged by Pint; remove it and keep the unqualified ?Usage type hint.

disconnect(string $container, string $network, bool $force) (line 16) lacks a return type, but Networks::disconnect() returns the adapter's result as bool. Add : bool to the abstract signature and all implementations.

getUsage(string $container): ?Usage (line 22) is the contract, but DockerCLI::getUsage() returns array and throws \Exception('getUsage') instead. Implement it to return a Usage object or null, not an array.

coderabbitai · 2025-11-23T08:12:27Z

src/Containers/Adapter/DockerAPI.php

+namespace Utopia\Containers\Adapter;

 use stdClass;
-use Utopia\Orchestration\Adapter;
-use Utopia\Orchestration\Container;
-use Utopia\Orchestration\Container\Stats;
-use Utopia\Orchestration\Exception\Orchestration;
-use Utopia\Orchestration\Exception\Timeout;
-use Utopia\Orchestration\Network;
+use Utopia\Containers\Adapter;
+use Utopia\Containers\Container;
+use Utopia\Containers\Container\Stats;
+use Utopia\Containers\Exception\Containers;
+use Utopia\Containers\Exception\Timeout;
+use Utopia\Containers\Network;

 class DockerAPI extends Adapter


⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

DockerAPI is still on the old adapter contract and does not match Utopia\Containers\Adapter

This class mixes v2 types (namespaces, exception classes, Stats value object) with largely v1 method signatures:

public function pull(string $image): bool (Lines 417–430) vs Adapter::pull(string $image): void.

public function run(string $image, string $name, array $command = [], string $entrypoint = '', string $workdir = '', array $volumes = [], array $vars = [], string $mountFolder = '', array $labels = [], string $hostname = '', bool $remove = false, string $network = '', string $restart = self::RESTART_NO): string (Lines 486–571) vs the new Adapter::run(string $name, string $image, string $hostname, array $command, array $entrypoint, ?string $workdir, array $environment, array $mounts, array $labels, ?string $network, ?float $cpus, ?int $memory, ?int $swap, RestartPolicy $restart, bool $remove): string.

There are only networkConnect() / networkDisconnect() methods (Lines 244–282), but the abstract adapter declares connect(string $container, string $network): void and disconnect(string $container, string $network, bool $force); these are missing here entirely.

listNetworks(), list(), and remove() still use the old boolean / array semantics but are at least compatible with the Adapter signatures.

This leads to several hard problems:

Inheritance/type compatibility: PHP will complain that DockerAPI::pull() and DockerAPI::run() have incompatible signatures with the abstract methods declared in Utopia\Containers\Adapter, and that abstract methods like connect/disconnect are not implemented.

Facade mismatch: Even if you bypass the abstract adapter, the new Containers::run() implementation (which expects v2 parameter order and Mount[]/RestartPolicy) cannot safely delegate to this v1‑style run() without a translation layer.

Tests vs adapter: tests/Containers/Base.php currently assumes exactly these v1 signatures for run() and restart constants, which contradicts the new v2 abstraction.

You’ll need to choose one direction and fully apply it here:

Option A (recommended for v2): Migrate DockerAPI to the new adapter contract:

Change pull() to void (or to bool and adjust the abstract contract), and throw ContainersException on error.

Change run() to match the new signature and semantics (use Mount objects, RestartPolicy enum, environment array, etc.), adapting the existing Docker API body accordingly.

Implement connect() and disconnect() (delegating to the existing networkConnect()/networkDisconnect() methods), and optionally deprecate/remove the network* variants.

Add getUsage() / listUsage() implementations as per the Adapter contract, or ensure they exist and match signatures.

Option B (transitional): Keep this as a v1‑only adapter:

Leave this class on the old contract and keep it independent of the new Utopia\Containers\Adapter abstraction.

Introduce a separate v2 adapter implementing the new contract and update Containers + tests to use that instead.

As it stands, this file cannot cleanly compile against the new Adapter base class and will block the v2 containers release.

Also applies to: 204-239, 244-282, 300-383, 388-412, 417-430, 438-473, 486-571, 579-633, 638-647

🏁 Script executed:

#!/bin/bash # Find and examine the abstract Adapter contract fd -e php Adapter | grep -E "src/Containers/Adapter\.php$" | head -1 | xargs cat

Length of output: 1785

🏁 Script executed:

#!/bin/bash # Examine DockerAPI implementation for pull, run, connect/disconnect methods fd -e php DockerAPI | grep -E "src/Containers/Adapter/DockerAPI\.php$" | head -1 | xargs cat

Length of output: 21754

🏁 Script executed:

#!/bin/bash # Check test expectations in tests/Containers/Base.php fd -e php Base.php | grep -E "tests/Containers/Base\.php$" | head -1 | xargs cat

Length of output: 20876

🏁 Script executed:

#!/bin/bash # Look for Containers facade that uses the adapter fd -e php Containers | grep -E "src/Containers\.php$" | head -1 | xargs cat

Length of output: 0

🏁 Script executed:

#!/bin/bash # Check exception and Stats classes fd -e php -path "*Containers/Exception*" | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && cat {}'

Length of output: 168

🏁 Script executed:

#!/bin/bash # Find the Containers class (main facade) fd -e php "Containers.php" | grep -v "Adapter" | grep -v "test" | xargs cat 2>/dev/null | head -200

Length of output: 5123

🏁 Script executed:

#!/bin/bash # Check for RestartPolicy enum/class fd -e php | xargs grep -l "RestartPolicy" | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && head -50 {}'

Length of output: 5688

🏁 Script executed:

#!/bin/bash # Check for Mount and Usage classes fd -e php | xargs grep -l "class Mount\|class Usage" | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && head -50 {}'

Length of output: 458

🏁 Script executed:

#!/bin/bash # Check for connect/disconnect methods in DockerAPI to see if they exist at all rg "public function (connect|disconnect)" src/ -A 2

Length of output: 1226

🏁 Script executed:

#!/bin/bash # Verify inheritance - does DockerAPI actually declare class extends Adapter? rg "class DockerAPI" src/ -A 1

Length of output: 107

DockerAPI does not implement the abstract Adapter contract and will fail at runtime

The verification confirms the review comment's core findings:

Missing implementations: DockerAPI lacks connect(), disconnect(), getUsage(), and listUsage() methods required by the abstract Adapter class. It has networkConnect()/networkDisconnect() instead, which do not match the abstract signatures.

Signature mismatches:

pull(string $image): bool vs abstract pull(string $image): void

run(string $image, string $name, array $command = [], ..., string $restart = self::RESTART_NO): string vs abstract run(string $name, string $image, ..., RestartPolicy $restart, ...): string — parameter order differs, $restart is a string constant instead of the RestartPolicy enum

PHP inheritance failure: Since DockerAPI extends Adapter but does not implement all abstract methods with compatible signatures, this will cause a fatal error at instantiation time.

Additional evidence: DockerCLI (another adapter in the same codebase) correctly implements the full v2 contract, including connect(), disconnect(), getUsage(), and listUsage() methods, and uses the RestartPolicy enum—confirming the v2 contract design is finalized.

The only path forward is to migrate DockerAPI to the v2 contract (as suggested in the original comment's Option A), or retire it in favor of DockerCLI. As written, this code cannot function.

🤖 Prompt for AI Agents

In src/Containers/Adapter/DockerAPI.php around lines 3 to 13, DockerAPI does not satisfy the Adapter v2 abstract contract: implement the missing abstract methods connect(), disconnect(), getUsage(), and listUsage() with the exact signatures defined in Adapter; rename or add wrappers so networkConnect()/networkDisconnect() map to the Adapter expected method names/signatures; change pull(string $image): bool to match the abstract pull(string $image): void; reorder run parameters to match run(string $name, string $image, ...) and replace the string $restart parameter with the RestartPolicy enum type used by the Adapter; use DockerCLI as a reference implementation for exact method signatures and behaviours or retire this class in favour of DockerCLI if you prefer not to migrate.

coderabbitai · 2025-11-23T08:12:29Z

src/Containers/Network.php

+final readonly class Network
+{
+    public function __construct(
+        public string $name = '',
+        public string $id = '',
+        public string $driver = '',
+        public string $scope = ''
+    )
+    {}


⚠️ Potential issue | 🟠 Major

Fix PSR‑12 brace positioning for Network constructor

Pint reports a braces_position violation; expanding the constructor body and moving braces to their own lines (like Container) should fix CI.

-final readonly class Network -{ - public function __construct( - public string $name = '', - public string $id = '', - public string $driver = '', - public string $scope = '' - ) - {} -} +final readonly class Network +{ + public function __construct( + public string $name = '', + public string $id = '', + public string $driver = '', + public string $scope = '' + ) { + } +}

🤖 Prompt for AI Agents

In src/Containers/Network.php around lines 5 to 13, the constructor's braces are on the same line causing a PSR-12 braces_position violation; expand the constructor body so the opening brace sits on its own line after the parameter list and the closing brace is on its own line (match the style used in Container), moving the empty body to a separate pair of lines to satisfy Pint/CI.

coderabbitai · 2025-11-23T08:12:29Z

src/Containers/Networks.php

+final readonly class Networks
+{
+    private function __construct(private Adapter $adapter)
+    {
+    }


⚠️ Potential issue | 🔴 Critical

Networks cannot be instantiated due to private constructor

Networks currently declares a private constructor:

final readonly class Networks { private function __construct(private Adapter $adapter) { } }

but your Containers class (per the provided snippet) does:

$this->networks = new Networks($adapter);

Since PHP enforces private visibility per‑class (not per‑namespace), this new Networks($adapter) call from Containers will fail with “Cannot access private constructor” at runtime.

You likely want one of:

Make the constructor public:

public function __construct(private Adapter $adapter) {}

Or keep it private but add a public static factory that Containers uses instead, e.g. Networks::fromAdapter(Adapter $adapter): self.

Without this, Containers::$networks can never be initialised and the wrapper is unusable.

🤖 Prompt for AI Agents

In src/Containers/Networks.php around lines 5 to 9, the class defines a private constructor which prevents external instantiation (Containers uses new Networks($adapter)); either change the constructor visibility to public so external code can call new Networks($adapter), or keep it private and add a public static factory method (eg. fromAdapter(Adapter $adapter): self) that constructs and returns a Networks instance; update Consumers (Containers) to use the chosen public constructor or factory method accordingly and ensure the signature accepts the Adapter dependency.

coderabbitai · 2025-11-23T08:12:29Z

src/Containers/Usage.php

+    public function __construct(
+        public float $cpuUsage,
+        public float $memoryUsage,
+        public array $diskIO,
+        public array $memoryIO,
+        public array $networkIO)
+    {}


🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Fix PSR-12 constructor formatting to satisfy Pint

The constructor formatting is triggering method_argument_space / braces_position errors in the linter. Moving the brace to its own line and using a trailing comma on the last parameter will align with PSR-12 and the rest of the codebase.

- public function __construct( - public float $cpuUsage, - public float $memoryUsage, - public array $diskIO, - public array $memoryIO, - public array $networkIO) - {} + public function __construct( + public float $cpuUsage, + public float $memoryUsage, + public array $diskIO, + public array $memoryIO, + public array $networkIO, + ) { + }

Please re-run Pint after this change to confirm the linter passes.

I'll verify the review comment by examining the actual file and validating the PSR-12 formatting recommendations.

--- 🏁 Script executed: ```shell #!/bin/bash # Check if the file exists and show its current state if [ -f "src/Containers/Usage.php" ]; then echo "=== File exists ===" sed -n '1,25p' src/Containers/Usage.php else echo "File not found at src/Containers/Usage.php" # Try to find Usage.php in the repository find . -name "Usage.php" -type f 2>/dev/null | head -20 fi

Length of output: 445

🏁 Script executed:

#!/bin/bash # Check for Pint configuration echo "=== Checking for Pint/PHP-CS-Fixer configuration ===" for config in pint.json .pint.json php-cs-fixer.php .php-cs-fixer.php .php-cs-fixer.dist.php; do if [ -f "$config" ]; then echo "Found: $config" cat "$config" | head -50 fi done # Check composer.json for scripts or config if [ -f "composer.json" ]; then echo "=== composer.json (relevant sections) ===" jq '.scripts // .config' composer.json 2>/dev/null || head -30 composer.json fi

Length of output: 702

🏁 Script executed:

#!/bin/bash # Search for any PSR-12 or formatting related files echo "=== Looking for formatting/linting configuration ===" find . -maxdepth 2 -type f $ -name "*.json" -o -name "*.php" -o -name ".*" $ | grep -E "(pint|fixer|lint|standard)" | head -20

Length of output: 64

🏁 Script executed:

#!/bin/bash # Check PHP version to understand promoted properties support if [ -f "composer.json" ]; then echo "=== PHP version requirement ===" jq '.require.php' composer.json 2>/dev/null fi

Length of output: 39

🌐 Web query:

PSR-12 constructor formatting trailing comma promoted properties PHP 8

💡 Result:

Short answer: Yes — PHP 8 allows a trailing comma in constructor parameter lists (so it also applies to promoted properties), and PSR‑12 does not forbid it.

Sources:

PSR‑12 (doesn't explicitly mention trailing-comma rule for constructors; shows multi-line constructor examples). [1]

PHP RFC: "Allow trailing comma in parameter list" (implemented for PHP 8.0). [2]

PHP manual: "As of PHP 8.0.0, the list of function parameters may include a trailing comma." (example). [3]

Recommendation: When you format multi-line constructor property promotions, you may include a trailing comma after the last promoted parameter for cleaner diffs and consistency with argument lists.

🌐 Web query:

Pint Laravel trailing comma constructor promoted properties

💡 Result:

Short answer: Yes — you can have a trailing comma for multiline constructor property-promoted parameters with Pint.

Why: PHP (since 8.0) permits a trailing comma in parameter lists, and Pint is a thin wrapper around PHP-CS-Fixer so you can enable the PHP-CS-Fixer rules that enforce trailing commas and multiline promoted properties. (wiki.php.net)

How to enable (example pint.json)

enable PHP-CS-Fixer rules: "trailing_comma_in_multiline" (include "parameters") and "multiline_promoted_properties".

Example pint.json:
{
"preset": "laravel",
"rules": {
"trailing_comma_in_multiline": {"elements": ["parameters","arguments","arrays"]},
"multiline_promoted_properties": true
}
}

References:

PHP trailing-comma in parameter lists (PHP 8.0 RFC). [1]. (wiki.php.net)

PHP-CS-Fixer: trailing_comma_in_multiline rule. [2]. (cs.symfony.com)

PHP-CS-Fixer: multiline_promoted_properties rule. [3]. (cs.symfony.com)

Laravel Pint documentation (Pint uses PHP-CS-Fixer and supports configuring these rules). [4]. (laravel.com)

If you want, I can produce a full pint.json tuned to your project or show a before/after example.

Citations:

1: https://wiki.php.net/rfc/trailing_comma_in_parameter_list?utm_source=openai

2: https://wiki.php.net/rfc/trailing_comma_in_parameter_list?utm_source=openai

3: https://cs.symfony.com/doc/rules/control_structure/trailing_comma_in_multiline.html?utm_source=openai

4: https://cs.symfony.com/doc/rules/function_notation/multiline_promoted_properties.html?utm_source=openai

5: https://laravel.com/docs/10.x/pint?utm_source=openai

I need to verify what the PSR-12 preset actually enforces for constructor formatting.

🌐 Web query:

PSR-12 preset Pint brace position closing brace parameter list

💡 Result:

PSR‑12 requires that when a function/closure/method parameter (or argument) list is split across multiple lines, the closing parenthesis and the opening brace must appear together on their own line with one space between them. Pint’s "psr12" preset (Pint is built on PHP‑CS‑Fixer) uses the PSR‑12 rules, so enabling the psr12 preset will enforce that behavior. [1][2]

Sources:

PSR‑12 specification (PHP‑FIG) — parameter/argument list multiline rule. [1]

Laravel Pint documentation — supports the "psr12" preset and is built on PHP‑CS‑Fixer. [2]

🌐 Web query:

PHP-CS-Fixer psr12 preset constructor multiline parameters rules

💡 Result:

Short answer — PHP-CS-Fixer's PSR‑12 preset controls constructor (and any method/function) multiline parameter formatting mainly via the method_argument_space_strict fixer (and related rules like psr12_braces and trailing_comma_in_multiline_call). PSR‑12 preset enables the strict variant so multiline argument lists are handled according to PSR‑12 (one argument per line, indentation rules, spacing around commas). [1][2]

If you need to tweak behavior (for example allow/ignore multi‑line argument formatting or require fully multiline with trailing commas), override method_argument_space (or method_argument_space_strict) options in your .php-cs-fixer.php config — e.g. set 'on_multiline' => 'ensure_fully_multiline' or 'ignore' — or enable/disable trailing_comma_in_multiline_call as desired. See the PHP‑CS‑Fixer rules list and preset changes for details. [1][2][3]

Sources:

PHP‑CS‑Fixer rules list (method_argument_space, trailing comma, etc.). [1]

Preset changes / PSR‑12 preset uses method_argument_space_strict + psr12_braces (changelog). [2]

Usage / config docs (how to set rule options in .php-cs-fixer.php). [3]

References:
[1] cs.symfony.com — List of Available Rules (method_argument_space).
[2] StyleCI / PHP‑CS‑Fixer preset changelog (PSR‑12 uses method_argument_space_strict).
[3] cs.symfony.com — Usage / configuration.

I need to clarify whether PSR-12 itself requires the trailing comma or if it's optional.

🌐 Web query:

PSR-12 standard trailing comma multiline parameter list requirement

💡 Result:

Short answer: PSR-12 requires a trailing comma on multiline lists (including parameter/argument lists). If the list is on a single line the last item MUST NOT have a trailing comma; if the list is split across multiple lines the last item MUST have a trailing comma. [1][2]

Sources:

PSR‑12 (Extended Coding Style). [1]

PHP‑FIG PER: Coding Style (clarifies examples, including parameter lists). [2]

Fix PSR-12 constructor formatting to satisfy Pint

The constructor violates PSR-12 requirements: the closing parenthesis and opening brace must be on their own line with one space between them, and a trailing comma is required on the last parameter in multiline lists. Pint will enforce this via the psr12 preset.

- public function __construct( - public float $cpuUsage, - public float $memoryUsage, - public array $diskIO, - public array $memoryIO, - public array $networkIO) - {} + public function __construct( + public float $cpuUsage, + public float $memoryUsage, + public array $diskIO, + public array $memoryIO, + public array $networkIO, + ) { + }

🤖 Prompt for AI Agents

In src/Containers/Usage.php around lines 12 to 18, the constructor violates PSR-12: ensure each parameter stays on its own line, add a trailing comma after the last parameter (public array $networkIO,), and place the closing parenthesis and opening brace together on their own line with a single space between them like ") {" so the multi-line constructor matches Pint's psr12 rules.

coderabbitai · 2025-11-23T08:12:29Z

tests/Containers/Base.php

+use Utopia\Containers\Adapter\DockerAPI;
+use Utopia\Containers\Containers;


⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

pull() return type and semantics are inconsistent across Containers, Adapter, DockerAPI, and tests

In this base test, static::getContainers()->pull() is treated as returning a boolean and used in assertions (Lines 38–52). However, the new Adapter contract and Containers wrapper are declared with a void return type for pull, while DockerAPI::pull() still returns bool. This leaves the stack in an inconsistent state:

Adapter::pull(string $image): void (per src/Containers/Adapter.php).

Containers::pull(string $image): void in the provided snippet (delegating and discarding any result).

DockerCLI::pull(string $image): void throws on failure.

DockerAPI::pull(string $image): bool returns true/false without throwing.

Tests expect bool and differentiate success/failure based on the return value.

You should decide on a single v2 contract for pull and align all layers:

Either:

Change Adapter::pull and Containers::pull to return bool, update DockerCLI::pull to return true on success and throw on error (or return false), and keep the tests’ semantics, or

Standardize on "throw on failure, void on success":

Change DockerAPI::pull to void and throw ContainersException on non‑200/204 codes.

Update tests to just call $containers->pull() for success cases and use $this->expectException(...) for failure cases instead of asserting false.

Right now this mismatch will either cause type incompatibilities or logically broken tests.

Also applies to: 33-52

I'll verify the return type inconsistency claim by examining the actual code in the repository.

--- 🏁 Script executed: ```shell #!/bin/bash # Find all pull() method signatures across Adapter, Containers, and DockerAPI classes echo "=== Searching for pull() method definitions ===" rg -nA5 "function pull\s*\(" --type php src/

Length of output: 1574

🏁 Script executed:

#!/bin/bash # Examine the test file to see how pull() is actually used echo "=== Examining test usage of pull() ===" cat tests/Containers/Base.php | head -60

Length of output: 1729

🏁 Script executed:

#!/bin/bash # Check if RestartPolicy enum exists and how it's defined echo "=== Searching for RestartPolicy enum ===" rg -n "enum RestartPolicy\|class RestartPolicy" --type php src/

Length of output: 40

🏁 Script executed:

#!/bin/bash # Check run() method signatures to understand the parameters echo "=== Searching for run() method definitions ===" rg -nA10 "function run\s*\(" --type php src/ | head -100

Length of output: 2827

pull() return type mismatch causes test failures

Verification confirms the critical inconsistency. The tests in tests/Containers/Base.php:33–51 assign the result of pull() to $response and assert its value as a boolean, but:

Containers::pull(string $image): void (src/Containers/Containers.php:23)

Adapter::pull(string $image): void (src/Containers/Adapter.php:32)

DockerAPI::pull(string $image): bool (src/Containers/Adapter/DockerAPI.php:417)

DockerCLI::pull(string $image): void (src/Containers/Adapter/DockerCLI.php:273)

The test calls $response = static::getContainers()->pull(...) and then asserts $this->assertEquals(true, $response) (line 39, 45) and $this->assertEquals(false, $response) (line 51), but pull() returns void, making $response null. These assertions will fail.

Choose one approach:

Make Adapter::pull() and Containers::pull() return bool and update DockerCLI::pull() to match.

Or standardize on void with exception-on-failure: make DockerAPI::pull() throw ContainersException on non-200 codes, update tests to use expectException() for failures instead of asserting false.

🤖 Prompt for AI Agents

In tests/Containers/Base.php around lines 6–7 and referencing tests at lines 33–51, the test expects pull() to return a boolean but current implementations have mixed return types (Containers::pull and Adapter::pull return void while DockerAPI::pull returns bool and DockerCLI::pull returns void); update src/Containers/Containers.php (around line 23) and src/Containers/Adapter.php (around line 32) to change the pull(string $image) signature to return bool, and modify src/Containers/Adapter/DockerCLI.php (around line 273) to return a boolean consistent with DockerAPI::pull (ensure it returns true on success, false on failure), update any docblocks/type hints and run tests to confirm assertions in tests/Containers/Base.php now receive boolean values.

coderabbitai · 2025-11-23T08:12:29Z

tests/Containers/Base.php

 abstract class Base extends TestCase
 {
-    abstract protected static function getOrchestration(): Orchestration;
+    abstract protected static function getContainers(): Containers;


⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Base tests still use the old run/restart/mount/network API and will not match the new Adapter/Containers contract

Across testCreateContainer, testNetworkConnect, testTimeoutContainer, testRunRemove, and testUsageStats, the calls to static::getContainers()->run() and network helpers are still written for the v1 DockerAPI::run signature:

Arguments are passed as (image, name, command, entrypoint, workdir, volumes, vars, mountFolder, labels, hostname, remove, network, restart), including:

Positional order where the first argument is the image ('appwrite/runtime-for-php:8.0') and second is the container name ('TestContainer').

A mountFolder argument (and string‑based host:container:mode volume specs) that no longer exist in the new Adapter::run signature.

Restart policy passed as DockerAPI::RESTART_ALWAYS / DockerAPI::RESTART_NO constants.

In contrast, the new abstraction (src/Containers/Adapter.php and DockerCLI::run) uses:

public function run( string $name, string $image, string $hostname, array $command, array $entrypoint, ?string $workdir, array $environment, array $mounts, // Mount[] array $labels, ?string $network, ?float $cpus, ?int $memory, ?int $swap, RestartPolicy $restart, bool $remove, ): string

Implications:

All existing run() calls here are currently passing parameters in the wrong order and with incompatible shapes (e.g., hostname as array, workdir as array, mounts as strings, restart as string constants), which will be a runtime/type error once wired to the new facade.

Tests still rely on DockerAPI restart constants instead of the new RestartPolicy enum (RestartPolicy::No, etc.).

The usage‑stats tests still pass mountFolder: named arguments and string mounts, but the v2 API expects Mount instances like Bind, Volume, and Tmpfs.

To make this base test compatible with the v2 Containers API you likely need to:

Update all run() call sites to match the new order and types, e.g. (roughly):

use Utopia\Containers\Mount\Bind; use Utopia\Containers\RestartPolicy; // Example for the first container in testCreateContainer: $response = static::getContainers()->run( name: 'TestContainer', image: 'appwrite/runtime-for-php:8.0', hostname: 'test-container', command: [ 'sh', '-c', 'cp /tmp/php.tar.gz /usr/local/src/php.tar.gz && tar -zxf /usr/local/src/php.tar.gz --strip 1 && tail -f /dev/null', ], entrypoint: [], workdir: '/usr/local/src/', environment: [], mounts: [ new Bind(\getenv('HOST_DIR').'/tests/Containers/Resources', '/test', readOnly: false), ], labels: [], network: null, cpus: null, memory: null, swap: null, restart: RestartPolicy::No, remove: false, );

Switch restart policy usage in tests from DockerAPI::RESTART_* constants to the new enum, e.g. RestartPolicy::Always / RestartPolicy::No, to match the adapter signature.

Decide how network APIs surface on the Containers facade:

Either keep convenience methods like networkConnect, networkDisconnect, createNetwork, removeNetwork, listNetworks, networkExists on Containers itself (and adapt them internally to the new Adapter/Networks wrapper), or

Move tests to use $containers->networks->create(...), $containers->networks->connect(...), etc. (once Networks is instantiable).

Make usage‑stats tests use the new IO and CPU model consistently (e.g., verifying Stats/Usage objects per the new adapter).

Until these tests are updated, they will be tightly coupled to the old DockerAPI v1 semantics and won’t exercise the new v2 containers abstraction correctly.

Also applies to: 57-181, 183-520, 522-557, 562-682

--- 🏁 Script executed: ```shell #!/bin/bash # Examine the test file to see actual run() calls and their parameters echo "=== tests/Containers/Base.php ===" cat tests/Containers/Base.php | head -50

Length of output: 1476

🏁 Script executed:

#!/bin/bash # Look at the Adapter class signature echo "=== src/Containers/Adapter.php run() method ===" rg -A20 "public function run\(" src/Containers/Adapter.php

Length of output: 549

🏁 Script executed:

#!/bin/bash # Find DockerAPI class and its run() signature echo "=== DockerAPI run() signature ===" rg -A15 "class DockerAPI" --type php rg -A20 "public function run\(" src/ --type php -B2

Length of output: 7774

🏁 Script executed:

#!/bin/bash # Search for RestartPolicy enum/class echo "=== RestartPolicy definition ===" fd -e php | xargs grep -l "RestartPolicy" | head -5 rg "class RestartPolicy|enum RestartPolicy" --type php -A10

Length of output: 584

🏁 Script executed:

#!/bin/bash # Search for Mount classes echo "=== Mount class definitions ===" rg "class Bind|class Volume|class Tmpfs" --type php -A5

Length of output: 1156

🏁 Script executed:

#!/bin/bash # Examine actual test methods mentioned in the comment echo "=== Test methods in Base.php ===" rg "testCreateContainer|testNetworkConnect|testTimeoutContainer|testRunRemove|testUsageStats" tests/Containers/Base.php -A5

Length of output: 1796

🏁 Script executed:

#!/bin/bash # Check for network methods on Containers facade echo "=== Network methods in Containers ===" rg "networkConnect|networkDisconnect|createNetwork|removeNetwork|listNetworks|networkExists" src/Containers/ --type php

Length of output: 1714

--- 🏁 Script executed: ```shell #!/bin/bash # Get full test method to see all parameters echo "=== Full testCreateContainer method ===" rg "public function testCreateContainer" tests/Containers/Base.php -A30

Length of output: 1030

🏁 Script executed:

#!/bin/bash # Get full testNetworkConnect method echo "=== Full testNetworkConnect method ===" rg "public function testNetworkConnect" tests/Containers/Base.php -A30

Length of output: 836

🏁 Script executed:

#!/bin/bash # Check if Containers facade has networkConnect/networkDisconnect methods echo "=== Containers facade methods ===" rg "public function.*network" src/Containers/Containers.php -A5

Length of output: 33

🏁 Script executed:

#!/bin/bash # Check complete DockerAPI run signature and its relationship to abstract echo "=== DockerAPI run() full signature ===" rg "public function run\(" src/Containers/Adapter/DockerAPI.php -A20 | head -30

Length of output: 741

🏁 Script executed:

#!/bin/bash # Check if DockerAPI class declaration and inheritance echo "=== DockerAPI class declaration ===" rg "class DockerAPI extends Adapter" src/Containers/Adapter/DockerAPI.php -A5

Length of output: 211

🏁 Script executed:

#!/bin/bash # Get full testRunRemove code echo "=== testRunRemove method ===" rg "public function testRunRemove" tests/Containers/Base.php -A20

Length of output: 565

🏁 Script executed:

#!/bin/bash # Get full testUsageStats code echo "=== testUsageStats method ===" rg "public function testUsageStats" tests/Containers/Base.php -A25

Length of output: 989

DockerAPI has an incompatible signature and tests use the wrong parameter order

The review comment is confirmed. DockerAPI extends Adapter but implements a different run() signature:

DockerAPI signature: run(string $image, string $name, array $command, string $entrypoint, string $workdir, array $volumes, array $vars, string $mountFolder, ...)

Adapter abstract signature: run(string $name, string $image, string $hostname, array $command, array $entrypoint, ?string $workdir, array $environment, array $mounts, ...)

Tests call getContainers()->run() with positional arguments matching the old DockerAPI order (image, name), but the Containers facade delegates to Adapter which expects (name, image, hostname, ...). Additionally:

testCreateContainer, testNetworkConnect, testTimeoutContainer, and testRunRemove pass positional arguments in the incompatible old order

testNetworkConnect calls getContainers()->networkConnect(...), but this method does not exist on the Containers facade (network methods are only on the Adapter level)

testUsageStats uses named arguments workdir: and mountFolder: — mountFolder no longer exists in the new signature

Tests use string restart values (e.g., self::RESTART_NO) instead of RestartPolicy enum values (RestartPolicy::No)

All calls across lines 57-181, 183-520, 522-557, 562-682 must be refactored to:

Use the new parameter order and types (name before image, Mount objects instead of volume strings, RestartPolicy enum instead of constants)

Access network operations via a dedicated Networks interface or move them to a proper location on Containers

Remove references to mountFolder and use mounts with Mount instances

Meldiron · 2025-12-31T11:11:41Z

src/Containers/Mount/Bind.php

+        public string $hostPath,
+        public string $containerPath,
+        public bool $readOnly = false


I would mark them protected instead, so you cannot directly read, or change them. And must use get or set methods.

feat-v2-containers

1787eb8

coderabbitai bot reviewed Nov 23, 2025

View reviewed changes

Meldiron reviewed Dec 31, 2025

View reviewed changes

		Utopia framework containers library is simple and lite library for abstracting the interaction with multiple container orchestrators. This library is aiming to be as simple and easy to learn and use. This library is maintained by the [Appwrite team](https://appwrite.io).

		Although this library is part of the [Utopia Framework](https://github.com/utopia-php/framework) project it is dependency free and can be used as standalone with any other PHP project or framework.

		use Utopia\Containers\Adapter\DockerAPI;
		use Utopia\Containers\Containers;

Conversation

loks0n commented Nov 23, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Release Notes

Uh oh!

coderabbitai bot commented Nov 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Estimated code review effort

Areas requiring extra attention:

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

Meldiron Dec 31, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

loks0n commented Nov 23, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 23, 2025 •

edited

Loading