0.18.x

[loks0n]

Summary by CodeRabbit

• New Features

  • AWS S3 storage device, storage telemetry wrapper, and configurable S3 retry controls; ZIP accepted by validator.

• Bug Fixes

  • Precise NotFound errors for missing files; idempotent multipart/chunk upload retry handling; safer local recursive delete and read behavior.

• Chores

  • PHP requirement bumped to 8.1; composer runtime extensions and plugin config updated; container base image updated.

• Tests

  • Tests added/updated for AWS adapter, not-found scenarios, multipart retry, and filename/extension validation.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Updates PHP to 8.1 (Docker, composer) and adds ext-curl/ext-simplexml; composer dependencies and config adjusted. Introduces telemetry integration (Histogram) in Device and a Telemetry device wrapper. Adds NotFoundException. Refactors S3 (fqdn-based requests, retry settings, error parsing, multipart behavior, constructor/signature changes) and adds a new AWS device class. Other providers (Backblaze, DOSpaces, Linode, Wasabi) move host construction into constructors. Local device gains idempotent chunk handling, read now throws NotFoundException, and upload/read signatures updated. Tests and validators updated accordingly.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The title '0.18.x' is a version number that does not describe the actual changes in the pull request, which include PHP upgrades, telemetry integration, AWS S3 device support, and various API improvements.	Replace the title with a descriptive summary of the main changes, such as 'Add AWS S3 device, telemetry integration, and upgrade to PHP 8.1' or a similar phrase that captures the primary objectives.
Docstring Coverage	⚠️ Warning	Docstring coverage is 44.57% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ 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 0.18.x

---

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments

src/Storage/Device/Local.php (1)

100-119: Chunk idempotency logic looks correct, but double file read and inconsistency with uploadData.

Two observations:

1. Lines 110 and 115 both read the chunk log file. $chunkLogs on line 110 is only used for the falsy check — reuse it for the count instead of reading the file again.

2. uploadData (line 146) does not have the same idempotency guard (file_exists check before writing the chunk log). If idempotent re-upload handling is needed for upload, it's likely needed for uploadData as well.

Fix double read

         $chunkLogs = file($tmp);
         if (! $chunkLogs) {
             throw new Exception('Unable to read chunk log '.$tmp);
         }
 
-        $chunksReceived = count(file($tmp));
+        $chunksReceived = count($chunkLogs);

src/Storage/Device/S3.php (3)

51-54: Minor inconsistency: setRetryAttempts is missing : void return type.

setRetryDelay (line 206) declares : void but setRetryAttempts (line 195) does not.

Fix

-    public static function setRetryAttempts(int $attempts)
+    public static function setRetryAttempts(int $attempts): void

Also applies to: 195-209

---

940-959: parseAndThrowS3Error swallows XML parse errors silently.

If simplexml_load_string fails (e.g., malformed XML that starts with <?xml), the catch on line 953 silently discards the error and falls through to throw a generic Exception with the raw body. This is acceptable behavior, but consider logging the parse failure for debuggability.

Also, the method is private but could be protected to allow subclasses (AWS, Backblaze, etc.) to override error parsing if their providers return different error formats.

---

810-929: Telemetry records attempts: 0 on success without retries — consider recording attempts: 1 for clarity.

When the first request succeeds (no retries), $attempt remains 0, which is recorded in telemetry. This could be misleading — typically "attempts" means total attempts including the initial one. A value of 0 might suggest the request was never attempted.

src/Storage/Device/Telemetry.php (2)

21-35: measure() uses dynamic dispatch — consider the implications.

The variadic by-ref &...$args pattern is needed for methods like upload/uploadData that accept array &$metadata, so this is correct. However, dynamic method dispatch ($this->underlying->{$method}(...)) bypasses static analysis and IDE support entirely.

Also note that when the underlying device is S3, operations will be recorded twice under different names: once as device:<method> by this wrapper and once as s3:<operation> inside S3::call(). This is presumably intentional for different granularity levels, but worth confirming.

---

37-55: Accessor methods (getName, getType, getDescription, getRoot) bypass measure() — intentional?

These are pure proxies without telemetry recording. This seems intentional since they're lightweight getters, but it breaks the pattern — getPath (line 57) does go through measure(). If telemetry isn't needed for simple getters, getPath arguably shouldn't be measured either.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 4

🧹 Nitpick comments (9)

src/Storage/Device/Wasabi.php (1)

44-45: Simplify the host string concatenation.

The host construction is functionally correct but uses excessive concatenation operators. Consider simplifying for better readability.

🔎 Proposed simplification

-        $host = $bucket.'.'.'s3'.'.'.$region.'.'.'wasabisys'.'.'.'com';
+        $host = $bucket.'.s3.'.$region.'.wasabisys.com';
         parent::__construct($root, $accessKey, $secretKey, $host, $region, $acl);

src/Storage/Device/Linode.php (1)

32-33: Simplify the host string concatenation.

The host construction uses unnecessary concatenation. Consider simplifying for consistency and readability.

🔎 Proposed simplification

-        $host = $bucket.'.'.$region.'.'.'linodeobjects.com';
+        $host = $bucket.'.'.$region.'.linodeobjects.com';
         parent::__construct($root, $accessKey, $secretKey, $host, $region, $acl);

src/Storage/Device/Backblaze.php (1)

37-38: Consider simplifying string concatenation.

The host construction works correctly but has unnecessary concatenation with separate dot strings. This can be simplified for readability.

🔎 Proposed simplification

-        $host = $bucket.'.'.'s3'.'.'.$region.'.backblazeb2.com';
+        $host = $bucket.'.s3.'.$region.'.backblazeb2.com';

tests/Storage/Device/LocalTest.php (2)

188-189: Comment references AWS S3 in a Local storage test.

This comment about AWS S3's 5MB chunk requirement is copied from S3 tests but may be misleading here since this tests Local storage which doesn't have such requirements. Consider updating the comment or using a different chunk size rationale.

---

196-210: Redundant variable assignments.

The variable $op is assigned at line 196, then reassigned identically inside the loop at line 199. This pattern is repeated in the second loop. Consider removing the redundant assignments inside the loops.

🔎 Proposed cleanup

         $op = __DIR__.'/chunkx.part';
         while ($start < $totalSize) {
             $contents = fread($handle, $chunkSize);
-            $op = __DIR__.'/chunkx.part';
             $cc = fopen($op, 'wb');

src/Storage/Device/AWS.php (1)

64-65: Docblock says "S3 Constructor" but this is the AWS class.

Minor documentation inconsistency - the docblock refers to "S3 Constructor" but this is the AWS class constructor.

🔎 Proposed fix

     /**
-     * S3 Constructor
+     * AWS Constructor
      *

src/Storage/Device/S3.php (2)

195-198: Missing return type annotation.

setRetryAttempts lacks an explicit : void return type, while setRetryDelay has one. Add the return type for consistency.

🔎 Proposed fix

-    public static function setRetryAttempts(int $attempts)
+    public static function setRetryAttempts(int $attempts): void
     {
         self::$retryAttempts = $attempts;
     }

---

920-927: Consider renaming attempts to retries for clarity.

The attempts attribute tracks the number of retries (0 = no retries, 1 = one retry). Renaming to retries would make this semantically clearer in telemetry dashboards.

src/Storage/Device/Telemetry.php (1)

57-60: Use explicit nullable type syntax.

The parameter types should use explicit nullable syntax (?string, ?int) to match the base class signatures and follow PHP 8.1+ best practices.

🔎 Proposed fix

-    public function getPath(string $filename, string $prefix = null): string
+    public function getPath(string $filename, ?string $prefix = null): string
     {
         return $this->measure(__FUNCTION__, $filename, $prefix);
     }

-    public function read(string $path, int $offset = 0, int $length = null): string
+    public function read(string $path, int $offset = 0, ?int $length = null): string
     {
         return $this->measure(__FUNCTION__, $path, $offset, $length);
     }

Also applies to: 77-80

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 32b6259 and 9c3f9a4.

⛔ Files ignored due to path filters (1)

• composer.lock is excluded by !**/*.lock

📒 Files selected for processing (21)

• .github/workflows/tests.yml
• Dockerfile
• composer.json
• src/Storage/Device.php
• src/Storage/Device/AWS.php
• src/Storage/Device/Backblaze.php
• src/Storage/Device/DOSpaces.php
• src/Storage/Device/Linode.php
• src/Storage/Device/Local.php
• src/Storage/Device/S3.php
• src/Storage/Device/Telemetry.php
• src/Storage/Device/Wasabi.php
• src/Storage/Exception/NotFoundException.php
• src/Storage/Storage.php
• src/Storage/Validator/FileExt.php
• src/Storage/Validator/FileName.php
• tests/Storage/Device/AWSTest.php
• tests/Storage/Device/LocalTest.php
• tests/Storage/S3Base.php
• tests/Storage/Validator/FileExtTest.php
• tests/Storage/Validator/FileNameTest.php

🧰 Additional context used

🧬 Code graph analysis (13)

src/Storage/Exception/NotFoundException.php (1)

src/Storage/Storage.php (1)

• Storage (7-126)

tests/Storage/S3Base.php (1)

src/Storage/Exception/NotFoundException.php (1)

• NotFoundException (7-9)

src/Storage/Device/Linode.php (4)

src/Storage/Device/S3.php (1)

• __construct (113-131)

src/Storage/Device/Backblaze.php (1)

• __construct (35-39)

src/Storage/Device/DOSpaces.php (1)

• __construct (34-38)

src/Storage/Device/Wasabi.php (1)

• __construct (42-46)

src/Storage/Device/Wasabi.php (2)

src/Storage/Device/S3.php (1)

• __construct (113-131)

src/Storage/Device/AWS.php (1)

• __construct (74-81)

src/Storage/Device/AWS.php (2)

src/Storage/Storage.php (1)

• Storage (7-126)

src/Storage/Device/S3.php (4)

• S3 (10-986)
• getName (136-139)
• getType (144-147)
• getDescription (152-155)

src/Storage/Device/DOSpaces.php (1)

src/Storage/Device/S3.php (1)

• __construct (113-131)

src/Storage/Device/Local.php (1)

src/Storage/Exception/NotFoundException.php (1)

• NotFoundException (7-9)

tests/Storage/Device/LocalTest.php (2)

src/Storage/Device/AWS.php (1)

• AWS (7-106)

src/Storage/Exception/NotFoundException.php (1)

• NotFoundException (7-9)

tests/Storage/Validator/FileNameTest.php (1)

src/Storage/Validator/FileName.php (1)

• isValid (23-38)

src/Storage/Device/Telemetry.php (1)

src/Storage/Device.php (24)

• Device (10-339)
• __construct (24-27)
• setTelemetry (77-84)
• getType (66-66)
• getName (57-57)
• getRoot (93-93)
• getPath (104-104)
• upload (121-121)
• uploadData (139-139)
• abort (148-148)
• read (158-158)
• transfer (169-169)
• write (178-178)
• delete (211-211)
• deletePath (220-220)
• exists (228-228)
• getFileSize (238-238)
• getFileMimeType (248-248)
• getFileHash (258-258)
• createDirectory (268-268)
• getDirectorySize (280-280)
• getPartitionFreeSpace (289-289)
• getPartitionTotalSpace (298-298)
• getFiles (308-308)

src/Storage/Device.php (1)

src/Storage/Device/Telemetry.php (5)

• Telemetry (8-146)
• __construct (10-13)
• setTelemetry (15-19)
• getPath (57-60)
• read (77-80)

src/Storage/Device/Backblaze.php (2)

src/Storage/Device/S3.php (1)

• __construct (113-131)

src/Storage/Device/AWS.php (1)

• __construct (74-81)

src/Storage/Device/S3.php (1)

src/Storage/Exception/NotFoundException.php (1)

• NotFoundException (7-9)

🔇 Additional comments (33)

.github/workflows/tests.yml (1)

61-61: LGTM!

The rename from S3Test to AWSTest aligns with the introduction of the new AWS device class. The S3 environment variables are appropriately retained since AWS uses S3-compatible storage APIs. The AWSTest.php test file exists and is properly referenced in the matrix.

Dockerfile (3)

116-116: Clarify the purpose of the CMD directive.

The addition of CMD [ "tail", "-f", "/dev/null" ] keeps the container running indefinitely without performing any work. This pattern is typically used for development, testing, or CI/CD scenarios where the container needs to remain active for interactive debugging or testing.

If this image is intended for production use where the storage library should be invoked as part of another service, consider whether this CMD is appropriate or if it should be removed to allow the consuming application to define its own entrypoint.

---

107-111: Extension API version correctly updated for PHP 8.1.

The extension paths have been correctly updated from API version 20200930 (PHP 8.0) to 20210902 (PHP 8.1). This change is consistent across all five extensions.

---

18-18: PHP 8.1 compatibility is already in place.

The composer.json already specifies "php": ">=8.1", confirming the project targets PHP 8.1 as minimum. The extension API version (20210902) is correct for PHP 8.1. A scan of the source code found no deprecated patterns (such as strptime, strftime, Serializable interface, or passing null to non-nullable parameters) that would conflict with PHP 8.1. All custom compression extensions compile correctly for the target PHP version.

composer.json (3)

24-24: Verify that the new extension requirements are utilized in the codebase.

The addition of ext-curl and ext-simplexml aligns with the S3/AWS device refactoring mentioned in the PR summary. However, please verify these extensions are actually used in the implementation.

#!/bin/bash
# Description: Verify usage of curl and simplexml extensions in the codebase.

# Search for curl function calls
echo "=== Checking for curl usage ==="
rg -n --type=php '\b(curl_init|curl_exec|curl_setopt|curl_close|curl_multi_)\w*\s*\(' -C2

# Search for simplexml function calls
echo "=== Checking for simplexml usage ==="
rg -n --type=php '\b(simplexml_load_string|simplexml_load_file|SimpleXMLElement)\b' -C2

Also applies to: 26-26

---

31-31: Telemetry dependency properly integrated and actively utilized.

The utopia-php/telemetry: 0.2.* addition is correctly implemented through a Decorator pattern wrapper class (src/Storage/Device/Telemetry.php) that instruments all storage operations. The base Device class accepts the Adapter with a no-op default, and creates a histogram to measure operation latency with appropriate bucket boundaries. All major storage operations (upload, read, write, delete, etc.) are instrumented to record execution time and operation metadata.

---

39-44: This configuration follows Composer security best practices.

Disabling php-http/discovery and tbachert/spi plugin discovery is the correct approach. Best practice is to use a deny-by-default posture: explicitly allow only Composer plugins you trust and actually need. These plugins execute code during composer install/update, so disabling them reduces supply-chain risk. If the project requires automatic HTTP client discovery, it should be handled via explicit pinning in composer.json extra configuration rather than auto-discovery.

src/Storage/Storage.php (1)

16-16: LGTM!

The new DEVICE_AWS_S3 constant follows the established naming pattern and provides a distinct device type for AWS-specific S3 storage.

src/Storage/Validator/FileExt.php (1)

19-19: LGTM!

The TYPE_ZIP constant follows the existing pattern and enables ZIP file validation support.

tests/Storage/Validator/FileExtTest.php (1)

17-40: LGTM!

The test coverage correctly validates the new ZIP extension support:

• file.zip is properly recognized as valid
• file.7zip is correctly rejected (different extension)

tests/Storage/Validator/FileNameTest.php (1)

32-34: LGTM!

The new test cases properly validate the expanded character support in file names (dashes, underscores, and combinations with dots), aligning with the validator implementation.

src/Storage/Exception/NotFoundException.php (1)

1-9: LGTM!

The new NotFoundException provides specific exception handling for not-found scenarios in storage operations, improving error granularity over generic exceptions.

src/Storage/Device/DOSpaces.php (1)

36-37: LGTM!

The refactored host construction aligns with the parent S3 constructor's signature and follows the same pattern as other storage devices in this PR, providing cleaner initialization.

src/Storage/Validator/FileName.php (1)

18-35: LGTM!

The updated validation logic correctly expands the allowed character set while properly rejecting filenames that contain only special characters or other invalid characters.

tests/Storage/Device/LocalTest.php (2)

79-83: LGTM!

Good addition of test coverage for the NotFoundException when attempting to read a non-existent file.

---

323-323: LGTM!

The transfer tests are correctly updated to use the new AWS device class.

Also applies to: 342-342

tests/Storage/S3Base.php (3)

137-141: LGTM!

Good test coverage for NotFoundException on reading non-existent files from S3-compatible storage.

---

304-371: Test validates multipart upload retry semantics.

The test correctly validates that re-uploading chunks from the beginning doesn't cause issues. However, note the same redundant $op assignments as in LocalTest.php (lines 324/327 and 346/349).

---

420-430: LGTM!

Good addition of test coverage for transfer operations on non-existent source files.

src/Storage/Device/AWS.php (1)

76-81: LGTM!

The host construction correctly handles Chinese AWS regions with the .amazonaws.cn domain suffix.

tests/Storage/Device/AWSTest.php (1)

5-36: LGTM!

The test class is correctly updated to use the new AWS device class with appropriate adapter name and description.

src/Storage/Device/Local.php (3)

22-26: LGTM!

The parent::__construct() call ensures proper initialization of the base Device class, including telemetry setup.

---

100-119: Idempotent chunk upload implementation looks correct.

The logic correctly prevents duplicate log entries when a chunk is re-uploaded by checking if the chunk file already exists. This ensures multipart uploads can be safely retried.

---

275-277: LGTM!

Appropriately using NotFoundException for missing file reads, providing more precise error handling.

src/Storage/Device.php (2)

22-27: LGTM!

The telemetry integration is well-structured with a sensible default of NoTelemetry (no-op adapter), ensuring backward compatibility.

---

77-84: Good histogram configuration for storage operation metrics.

The bucket boundaries (5ms to 10s) are appropriate for storage operations, covering both fast local operations and slower remote/S3 operations.

src/Storage/Device/S3.php (4)

51-53: Static retry config affects all S3 instances globally.

These static properties are shared across all S3-derived instances (S3, AWS, Backblaze, DOSpaces, etc.). Consider whether per-instance configuration would be more appropriate for multi-tenant scenarios.

---

113-131: LGTM!

The host-based construction logic correctly handles both protocol-prefixed and bare hostnames, appropriately setting fqdn and the host header.

---

260-271: LGTM!

The re-upload detection using array_key_exists correctly prevents double-counting chunks when retrying failed part uploads.

---

940-959: LGTM!

The error parsing correctly identifies NoSuchKey errors and throws the appropriate NotFoundException, with proper re-throw handling and graceful fallback for XML parsing failures.

src/Storage/Device/Telemetry.php (3)

10-19: LGTM!

The telemetry propagation to both the wrapper and underlying device ensures consistent instrumentation.

---

21-35: LGTM!

The measure helper correctly uses by-reference variadic parameters to support methods like upload() that modify their $metadata argument. The finally block ensures telemetry is recorded regardless of success or failure.

---

132-140: LGTM!

Parameterless methods correctly delegate through measure() with an empty argument list.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Incorrect region value for SA_EAST_1.

The constant SA_EAST_1 is assigned 'eu-north-1' but should be 'sa-east-1'. This appears to be a copy-paste error from line 48.

🔎 Proposed fix

-    const SA_EAST_1 = 'eu-north-1';
+    const SA_EAST_1 = 'sa-east-1';

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	const SA_EAST_1 = 'eu-north-1';
	const SA_EAST_1 = 'sa-east-1';

🤖 Prompt for AI Agents

In src/Storage/Device/AWS.php around line 50, the constant SA_EAST_1 is
incorrectly set to 'eu-north-1'; change its value to the correct AWS region
identifier 'sa-east-1' to fix the copy-paste error and ensure region constants
map to the proper AWS regions.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Response body accumulates across retries.

The retry loop re-executes curl_exec, but $response->body is not reset between attempts. Since CURLOPT_WRITEFUNCTION appends to $response->body, retries will concatenate all response bodies, corrupting the final result.

🔎 Proposed fix

         $attempt = 0;
         while ($attempt < self::$retryAttempts && $response->code >= 500) {
             usleep(self::$retryDelay * 1000);
             $attempt++;
+            $response->body = '';
+            $response->headers = [];
             $result = \curl_exec($curl);
             $response->code = \curl_getinfo($curl, CURLINFO_HTTP_CODE);
         }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	$attempt = 0;
	while ($attempt < self::$retryAttempts && $response->code >= 500) {
	usleep(self::$retryDelay * 1000);
	$attempt++;
	$result = \curl_exec($curl);
	$response->code = \curl_getinfo($curl, CURLINFO_HTTP_CODE);
	}
	$attempt = 0;
	while ($attempt < self::$retryAttempts && $response->code >= 500) {
	usleep(self::$retryDelay * 1000);
	$attempt++;
	$response->body = '';
	$response->headers = [];
	$result = \curl_exec($curl);
	$response->code = \curl_getinfo($curl, CURLINFO_HTTP_CODE);
	}

🤖 Prompt for AI Agents

In src/Storage/Device/S3.php around lines 893 to 899, the retry loop re-calls
curl_exec but never clears the previous response data so CURLOPT_WRITEFUNCTION
keeps appending bodies across attempts; before each retry execution reset
$response->body (and any related header/length state used by the write callback)
to an empty value so each attempt starts with a fresh buffer, then re-run
curl_exec and update the HTTP code as shown.