feat: S3 transfer manager improvements by yenfryherrerafeliz · Pull Request #3247 · aws/aws-sdk-php

yenfryherrerafeliz · 2026-02-12T15:49:37Z

Description of changes:

Includes resumable capabilities to resume downloads and uploads.
Concurrent downloads to enhance download speed.
Directory Transfer Improvements.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Until phpunit10 support is release we must use annotations for covers and dataProvider within tests.

src/S3/S3Transfer/AbstractMultipartDownloader.php

src/S3/S3Transfer/Models/DownloadDirectoryResult.php

stobrien89 · 2026-03-11T15:46:35Z

src/S3/S3Transfer/AbstractMultipartDownloader.php

+     */
+    public static function computeObjectSizeFromContentRange(
+        string $contentRange
+    ): int


does this return int or string?

Integer. It basically returns the object size from the content range.

what I was getting at is you might need a cast to int because the size is being extracted from preg_match()

src/S3/S3Transfer/AbstractMultipartDownloader.php

stobrien89 · 2026-03-11T15:48:46Z

src/S3/CalculatesChecksumTrait.php

+     *
+     * @return string|null
+     */
+    public static function filterChecksum(array $parameters):? string


nit: no space after nullable for return types

this one is still out of position- should be : ?string

src/S3/S3Transfer/Progress/TransferProgressSnapshot.php

src/S3/S3Transfer/Models/ResumableDownload.php

src/S3/S3Transfer/Progress/DirectoryTransferProgressAggregator.php

src/S3/S3Transfer/Progress/DirectoryProgressTracker.php

src/S3/S3Transfer/Progress/DirectoryTransferProgressSnapshot.php

- Address some formatting issues. - Fixed arguments passed to ResumableDownload - Add test coverage for: - DirectoryProgressTracker - DirectoryTransferProgressAggregator - DirectoryTransferProgressSnapshot - DirectoryDownloader - DirectoryUploader

yenfryherrerafeliz · 2026-03-17T04:03:25Z

vendor/bin/behat --format=progress --suite=smoke --tags='~@noassumerole'
...................................................................... 70
...................................................................... 140
...................................................................... 210
............

111 scenarios (111 passed)
222 steps (222 passed)
0m35.66s (96.23Mb)

vendor/bin/behat --format=progress --tags=integ
...................................................................... 70
...................................................................... 140
...................................................................... 210
...................................................................... 280
...................................................................... 350
...................................................................... 420
......................................................

136 scenarios (136 passed)
474 steps (474 passed)
27m28.00s (720.09Mb)

stobrien89

Just a few more questions/comments

stobrien89 · 2026-03-19T16:31:22Z

src/S3/S3Transfer/Utils/FileDownloadHandler.php

+     *
+     * @return bool
+     */
+    private function writePartToDestinationHandle(array $response): bool


Question: do part numbers start at 1 or 0? If zero, I think this might be exposed to an off-by-one error

Part numbers start at 1 actually.

stobrien89 · 2026-03-19T16:37:14Z

src/S3/S3Transfer/MultipartUploader.php

+            $this->body->rewind();
+        }
+
+        $partNo = 0;


Does resume data store 1-based part numbers? Might be a mismatch during resume skip logic

$partNo is incremented at line 388, which will unlikely cause a resumable
persist mismatch. Is there any other mismatch you are referring to here, that I
may be missing?

stobrien89 · 2026-03-19T16:46:29Z

src/S3/S3Transfer/Utils/FileDownloadHandler.php

+                $chunk = $body->read(self::READ_BUFFER_SIZE);
+
+                if (fwrite($this->handle, $chunk) === false) {
+                    throw new FileDownloadException("Failed to write data to temporary file.");


question: where do these exceptions bubble up to?

If part number one fails then it will go through here, otherwise it will go through here.

src/S3/S3Transfer/S3TransferManager.php

src/S3/CalculatesChecksumTrait.php

src/S3/S3Transfer/AbstractMultipartDownloader.php

src/S3/S3Transfer/Models/UploadRequest.php

- Add validation for when resuming an upload and the source file changed - Add test coverage for the behavior when resuming upload and source file changed - Address some styling issues

stobrien89

A couple more comments. Thanks for making the changes to the CBOR tests also

stobrien89 · 2026-03-25T13:28:06Z

src/S3/S3Transfer/AbstractMultipartDownloader.php

+     */
+    public static function computeObjectSizeFromContentRange(
+        string $contentRange
+    ): int


what I was getting at is you might need a cast to int because the size is being extracted from preg_match()

stobrien89 · 2026-03-25T13:30:26Z

src/S3/CalculatesChecksumTrait.php

+     *
+     * @return string|null
+     */
+    public static function filterChecksum(array $parameters):? string


this one is still out of position- should be : ?string

stobrien89 · 2026-03-25T13:36:03Z

src/S3/S3Transfer/S3TransferManager.php

+        $headResult = $this->s3Client->headObject([
+            'Bucket' => $resumableDownload->getBucket(),
+            'Key' => $resumableDownload->getKey(),
+        ]);


Could be wrong, but shouldn't this be headObjectAsync() since its inside an async workflow?

stobrien89 · 2026-03-25T13:40:18Z

src/S3/S3Transfer/S3TransferManager.php

+        $uploads = $this->s3Client->listMultipartUploads([
+            'Bucket' => $resumableUpload->getBucket(),
+            'Prefix' => $resumableUpload->getKey(),
+        ]);


Should this also be paginated like the ListObjectsV2 call?

stobrien89 · 2026-03-25T13:44:18Z

tests/S3/S3Transfer/S3TransferManagerTest.php

-    private const UPLOAD_DIRECTORY_CROSS_PLATFORM_BASE_CASES = __DIR__ . '/test-cases/upload-directory-cross-platform-compatibility.json';
-    private const DOWNLOAD_DIRECTORY_CROSS_PLATFORM_BASE_CASES = __DIR__ . '/test-cases/download-directory-cross-platform-compatibility.json';


Did these tests get moved or removed entirely?

yenfryherrerafeliz added 10 commits February 10, 2026 08:35

chore: merge resumable changes

4c25956

feat: merge and refactor tests

b6a48a0

chore: merge integ tests from resumable

17e7aab

chore: use annotation in tests

4dfbd99

Until phpunit10 support is release we must use annotations for covers and dataProvider within tests.

chore: add filter checksum helper

1325a3f

chore: some unsupported class attributes leftover

82573a0

feat: directory transfer enhancement

31d8335

chore: fix tests to include getHandlerList method

c6b052a

chore: enhance s3 transfer manager tests for windows support

eca52ae

chore: enhance resolvesOutsideTargetDirectory to use dir separator

ae2bf42

yenfryherrerafeliz requested a review from stobrien89 February 23, 2026 20:19

yenfryherrerafeliz changed the title ~~S3 transfer manager improvements~~ feat: S3 transfer manager improvements Feb 24, 2026

chore: make directory transfers implements promisor

a347716

stobrien89 requested changes Mar 11, 2026

View reviewed changes

yenfryherrerafeliz added 5 commits March 15, 2026 19:03

chore: merge 'master' into s3_transfer_manager_improvements

3b819e8

chore: address PR feedback

d58692e

- Address some formatting issues. - Fixed arguments passed to ResumableDownload - Add test coverage for: - DirectoryProgressTracker - DirectoryTransferProgressAggregator - DirectoryTransferProgressSnapshot - DirectoryDownloader - DirectoryUploader

chore: update dataProviders to phpunit10

65712eb

chore: typo in expected file path

c124507

chore: make test valid for windows and linux

eef94ab

yenfryherrerafeliz requested a review from stobrien89 March 17, 2026 02:24

chore: mute warnings

8311ea8

stobrien89 requested changes Mar 19, 2026

View reviewed changes

chore: address PR feedback

bb114dd

- Add validation for when resuming an upload and the source file changed - Add test coverage for the behavior when resuming upload and source file changed - Address some styling issues

yenfryherrerafeliz requested a review from stobrien89 March 20, 2026 14:54

chore: remove space from constructor

eb8d875

stobrien89 requested changes Mar 25, 2026

View reviewed changes

		private const UPLOAD_DIRECTORY_CROSS_PLATFORM_BASE_CASES = __DIR__ . '/test-cases/upload-directory-cross-platform-compatibility.json';
		private const DOWNLOAD_DIRECTORY_CROSS_PLATFORM_BASE_CASES = __DIR__ . '/test-cases/download-directory-cross-platform-compatibility.json';

Conversation

yenfryherrerafeliz commented Feb 12, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yenfryherrerafeliz commented Mar 17, 2026

Uh oh!

stobrien89 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stobrien89 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants