From fc089120eee9064b27618fb5fcca5276f3c8a6e8 Mon Sep 17 00:00:00 2001 From: Lucas Carvalho Date: Fri, 16 Jan 2026 18:24:36 -0300 Subject: [PATCH 1/6] chore: allow directory rename on Stream_Wrapper --- inc/class-stream-wrapper.php | 92 ++++++++++++++++++++++++++++++++---- 1 file changed, 84 insertions(+), 8 deletions(-) diff --git a/inc/class-stream-wrapper.php b/inc/class-stream-wrapper.php index 707eae35..46c5d29a 100644 --- a/inc/class-stream-wrapper.php +++ b/inc/class-stream-wrapper.php @@ -738,13 +738,12 @@ private function formatKey( string $key ) : string { } /** - * Called in response to rename() to rename a file or directory. Currently - * only supports renaming objects. + * Called in response to rename() to rename a file or directory. * - * @param string $path_from the path to the file to rename - * @param string $path_to the new path to the file + * @param string $path_from the path to the file or directory to rename + * @param string $path_to the new path to the file or directory * - * @return bool true if file was successfully renamed + * @return bool true if file or directory was successfully renamed * @link http://www.php.net/manual/en/function.rename.php */ public function rename( $path_from, $path_to ) { @@ -764,10 +763,69 @@ public function rename( $path_from, $path_to ) { } return $this->boolCall( - function () use ( $partsFrom, $partsTo ) { + function () use ( $partsFrom, $partsTo, $path_from, $path_to ) { $options = $this->getOptions( true ); - // Copy the object and allow overriding default parameters if - // desired, but by default copy metadata + $client = $this->getClient(); + + // Normalize keys - ensure trailing slash for directories + $fromKey = rtrim( $partsFrom['Key'], '/' ); + $toKey = rtrim( $partsTo['Key'], '/' ); + + $existsAsFile = $client->doesObjectExistV2( + $partsFrom['Bucket'], + $partsFrom['Key'], + false, + $options + ); + + $isDirectory = ! $existsAsFile && $this->isDirectoryPrefix( $partsFrom['Bucket'], $fromKey ); + + if ( $isDirectory ) { + $fromPrefix = $fromKey . '/'; + $toPrefix = $toKey . '/'; + + $paginator = $client->getPaginator( 'ListObjectsV2', [ + 'Bucket' => $partsFrom['Bucket'], + 'Prefix' => $fromPrefix, + ] ); + + $objectsToDelete = []; + foreach ( $paginator as $result ) { + if ( ! isset( $result['Contents'] ) ) { + continue; + } + + foreach ( $result['Contents'] as $object ) { + $oldKey = $object['Key']; + $newKey = str_replace( $fromPrefix, $toPrefix, $oldKey ); + + $client->copy( + $partsFrom['Bucket'], + $oldKey, + $partsTo['Bucket'], + $newKey, + isset( $options['acl'] ) ? $options['acl'] : 'private', + $options + ); + + $objectsToDelete[] = $oldKey; + + $this->clearCacheKey( "{$this->protocol}://{$partsFrom['Bucket']}/{$oldKey}" ); + $this->clearCacheKey( "{$this->protocol}://{$partsTo['Bucket']}/{$newKey}" ); + } + } + + // Delete all original objects after successful copy + foreach ( $objectsToDelete as $key ) { + $client->deleteObject( [ + 'Bucket' => $partsFrom['Bucket'], + 'Key' => $key, + ] + $options ); + } + + return true; + } + $this->getClient()->copy( $partsFrom['Bucket'], $partsFrom['Key'], @@ -1113,6 +1171,24 @@ private function deleteSubfolder( string $path, array $params ) : bool { : true; } + /** + * Check if a key represents a directory prefix (has objects with that prefix). + * + * @param string $bucket The bucket name + * @param string $key The key to check (without trailing slash) + * @return bool True if the key is a directory prefix + */ + private function isDirectoryPrefix( string $bucket, string $key ) : bool { + $prefix = $key . '/'; + $result = $this->getClient()->listObjectsV2( [ + 'Bucket' => $bucket, + 'Prefix' => $prefix, + 'MaxKeys' => 1, + ] ); + + return ! empty( $result['Contents'] ) || ! empty( $result['CommonPrefixes'] ); + } + /** * Determine the most appropriate ACL based on a file mode. * From 1ebdc8657f8099bd52bb62681f93e8382d674ca6 Mon Sep 17 00:00:00 2001 From: Lucas Carvalho Date: Fri, 16 Jan 2026 20:23:14 -0300 Subject: [PATCH 2/6] feat: implement directory renaming with batch operations in Stream_Wrapper --- inc/class-stream-wrapper.php | 199 ++++++++++++++++------- tests/test-s3-uploads-stream-wrapper.php | 88 ++++++++++ 2 files changed, 231 insertions(+), 56 deletions(-) diff --git a/inc/class-stream-wrapper.php b/inc/class-stream-wrapper.php index 46c5d29a..f479a5a7 100644 --- a/inc/class-stream-wrapper.php +++ b/inc/class-stream-wrapper.php @@ -3,6 +3,7 @@ namespace S3_Uploads; use Aws\CacheInterface; +use Aws\CommandPool; use Aws\LruArrayCache; use Aws\Result; use Aws\S3\Exception\S3Exception; @@ -763,9 +764,10 @@ public function rename( $path_from, $path_to ) { } return $this->boolCall( - function () use ( $partsFrom, $partsTo, $path_from, $path_to ) { + function () use ( $partsFrom, $partsTo ) { $options = $this->getOptions( true ); $client = $this->getClient(); + $acl = isset( $options['acl'] ) ? $options['acl'] : 'private'; // Normalize keys - ensure trailing slash for directories $fromKey = rtrim( $partsFrom['Key'], '/' ); @@ -781,69 +783,154 @@ function () use ( $partsFrom, $partsTo, $path_from, $path_to ) { $isDirectory = ! $existsAsFile && $this->isDirectoryPrefix( $partsFrom['Bucket'], $fromKey ); if ( $isDirectory ) { - $fromPrefix = $fromKey . '/'; - $toPrefix = $toKey . '/'; - - $paginator = $client->getPaginator( 'ListObjectsV2', [ - 'Bucket' => $partsFrom['Bucket'], - 'Prefix' => $fromPrefix, - ] ); - - $objectsToDelete = []; - foreach ( $paginator as $result ) { - if ( ! isset( $result['Contents'] ) ) { - continue; - } + return $this->renameDirectory( $client, $partsFrom, $partsTo, $fromKey, $toKey, $acl, $options ); + } - foreach ( $result['Contents'] as $object ) { - $oldKey = $object['Key']; - $newKey = str_replace( $fromPrefix, $toPrefix, $oldKey ); + return $this->renameFile( $client, $partsFrom, $partsTo, $acl, $options ); + } + ); + } - $client->copy( - $partsFrom['Bucket'], - $oldKey, - $partsTo['Bucket'], - $newKey, - isset( $options['acl'] ) ? $options['acl'] : 'private', - $options - ); + /** + * Rename a directory by copying all objects and then deleting originals. + * Uses batch operations for better S3 performance. + * + * @param S3ClientInterface $client S3 client instance + * @param array{Bucket: string, Key: string} $partsFrom Source path parts + * @param array{Bucket: string, Key: string} $partsTo Destination path parts + * @param string $fromKey Normalized source key + * @param string $toKey Normalized destination key + * @param string $acl ACL for copied objects + * @param array $options Additional S3 options + * @return bool True on success + */ + private function renameDirectory( + S3ClientInterface $client, + array $partsFrom, + array $partsTo, + string $fromKey, + string $toKey, + string $acl, + array $options + ) : bool { + $fromPrefix = $fromKey . '/'; + $toPrefix = $toKey . '/'; + $copyCommands = []; + $objectsToDelete = []; + $cacheKeysToClear = []; + + $paginator = $client->getPaginator( 'ListObjectsV2', [ + 'Bucket' => $partsFrom['Bucket'], + 'Prefix' => $fromPrefix, + ] ); - $objectsToDelete[] = $oldKey; + foreach ( $paginator as $result ) { + if ( ! isset( $result['Contents'] ) || ! is_array( $result['Contents'] ) ) { + continue; + } - $this->clearCacheKey( "{$this->protocol}://{$partsFrom['Bucket']}/{$oldKey}" ); - $this->clearCacheKey( "{$this->protocol}://{$partsTo['Bucket']}/{$newKey}" ); - } - } + /** @var list $contents */ + $contents = $result['Contents']; + foreach ( $contents as $object ) { + /** @var array{Key: string} $object */ + /** @var string $oldKey */ + $oldKey = $object['Key']; + /** @var string $newKey */ + $newKey = str_replace( $fromPrefix, $toPrefix, $oldKey ); + + // Prepare copy command for batch execution + $copyCommands[] = $client->getCommand( 'CopyObject', [ + 'Bucket' => $partsTo['Bucket'], + 'Key' => $newKey, + 'CopySource' => "{$partsFrom['Bucket']}/{$oldKey}", + 'ACL' => $acl, + ] + $options ); + + $objectsToDelete[] = [ 'Key' => $oldKey ]; + $cacheKeysToClear[] = "{$this->protocol}://{$partsFrom['Bucket']}/{$oldKey}"; + $cacheKeysToClear[] = "{$this->protocol}://{$partsTo['Bucket']}/{$newKey}"; + } + } - // Delete all original objects after successful copy - foreach ( $objectsToDelete as $key ) { - $client->deleteObject( [ - 'Bucket' => $partsFrom['Bucket'], - 'Key' => $key, - ] + $options ); - } + // Handle directory marker (empty object with key ending in "/") if it exists + $directoryMarkerKey = $fromPrefix; + if ( $client->doesObjectExistV2( $partsFrom['Bucket'], $directoryMarkerKey, false, $options ) ) { + $directoryMarkerNewKey = $toPrefix; + $copyCommands[] = $client->getCommand( 'CopyObject', [ + 'Bucket' => $partsTo['Bucket'], + 'Key' => $directoryMarkerNewKey, + 'CopySource' => "{$partsFrom['Bucket']}/{$directoryMarkerKey}", + 'ACL' => $acl, + ] + $options ); + + $objectsToDelete[] = [ 'Key' => $directoryMarkerKey ]; + $cacheKeysToClear[] = "{$this->protocol}://{$partsFrom['Bucket']}/{$directoryMarkerKey}"; + $cacheKeysToClear[] = "{$this->protocol}://{$partsTo['Bucket']}/{$directoryMarkerNewKey}"; + } - return true; - } + if ( empty( $copyCommands ) ) { + return true; + } - $this->getClient()->copy( - $partsFrom['Bucket'], - $partsFrom['Key'], - $partsTo['Bucket'], - $partsTo['Key'], - isset( $options['acl'] ) ? $options['acl'] : 'private', - $options - ); - // Delete the original object - $this->getClient()->deleteObject( - [ - 'Bucket' => $partsFrom['Bucket'], - 'Key' => $partsFrom['Key'], - ] + $options - ); - return true; - } + // Execute all copies in parallel using CommandPool + CommandPool::batch( $client, $copyCommands ); + + // Clear cache for all affected keys + foreach ( $cacheKeysToClear as $cacheKey ) { + $this->clearCacheKey( $cacheKey ); + } + + // Delete all original objects using batch deleteObjects (up to 1000 per request) + $maxDeleteBatch = 1000; + $deleteBatches = array_chunk( $objectsToDelete, $maxDeleteBatch ); + + foreach ( $deleteBatches as $deleteBatch ) { + $client->deleteObjects( [ + 'Bucket' => $partsFrom['Bucket'], + 'Delete' => [ + 'Objects' => $deleteBatch, + ], + ] + $options ); + } + + return true; + } + + /** + * Rename a single file by copying and then deleting the original. + * + * @param S3ClientInterface $client S3 client instance + * @param array{Bucket: string, Key: string} $partsFrom Source path parts + * @param array{Bucket: string, Key: string} $partsTo Destination path parts + * @param string $acl ACL for copied object + * @param array $options Additional S3 options + * @return bool True on success + */ + private function renameFile( + S3ClientInterface $client, + array $partsFrom, + array $partsTo, + string $acl, + array $options + ) : bool { + $client->copy( + $partsFrom['Bucket'], + $partsFrom['Key'], + $partsTo['Bucket'], + $partsTo['Key'], + $acl, + $options ); + + $client->deleteObject( [ + 'Bucket' => $partsFrom['Bucket'], + 'Key' => $partsFrom['Key'], + ] + $options ); + + $this->clearCacheKey( "{$this->protocol}://{$partsFrom['Bucket']}/{$partsFrom['Key']}" ); + $this->clearCacheKey( "{$this->protocol}://{$partsTo['Bucket']}/{$partsTo['Key']}" ); + + return true; } public function stream_cast( int $cast_as ) : bool { diff --git a/tests/test-s3-uploads-stream-wrapper.php b/tests/test-s3-uploads-stream-wrapper.php index 6d9c6e86..1be545e4 100644 --- a/tests/test-s3-uploads-stream-wrapper.php +++ b/tests/test-s3-uploads-stream-wrapper.php @@ -154,4 +154,92 @@ public function test_list_directory_with_wildcard() { $files ); } + + public function test_rename_directory_via_stream_wrapper() { + $upload_dir = wp_upload_dir(); + $test_dir = $upload_dir['path'] . '/test-dir'; + $renamed_dir = $upload_dir['path'] . '/renamed-dir'; + + // Create a directory with files + mkdir( $test_dir, 0755, true ); + $test_file1 = $test_dir . '/file1.txt'; + $test_file2 = $test_dir . '/subdir/file2.txt'; + + file_put_contents( $test_file1, 'content1' ); + mkdir( $test_dir . '/subdir', 0755, true ); + file_put_contents( $test_file2, 'content2' ); + + // Rename the directory + $result = rename( $test_dir, $renamed_dir ); + $this->assertTrue( $result, 'Directory rename should succeed' ); + + // Verify original directory files don't exist + $this->assertFalse( file_exists( $test_dir . '/file1.txt' ), 'Original file should not exist' ); + $this->assertFalse( file_exists( $test_dir . '/subdir/file2.txt' ), 'Original subdir file should not exist' ); + + // Verify renamed directory files exist + $this->assertTrue( file_exists( $renamed_dir . '/file1.txt' ), 'Renamed file should exist' ); + $this->assertTrue( file_exists( $renamed_dir . '/subdir/file2.txt' ), 'Renamed subdir file should exist' ); + + // Verify file contents + $this->assertEquals( 'content1', file_get_contents( $renamed_dir . '/file1.txt' ), 'File1 content should match' ); + $this->assertEquals( 'content2', file_get_contents( $renamed_dir . '/subdir/file2.txt' ), 'File2 content should match' ); + } + + public function test_rename_empty_directory_via_stream_wrapper() { + $upload_dir = wp_upload_dir(); + $test_dir = $upload_dir['path'] . '/empty-dir'; + $renamed_dir = $upload_dir['path'] . '/empty-renamed'; + + // Create an empty directory + mkdir( $test_dir, 0755, true ); + + // Rename the empty directory + $result = rename( $test_dir, $renamed_dir ); + $this->assertTrue( $result, 'Empty directory rename should succeed' ); + + // Verify renamed directory exists + $this->assertTrue( is_dir( $renamed_dir ), 'Renamed empty directory should exist' ); + + // Note: For empty directories, file_exists() may still return true because + // S3 checks for objects with that prefix. The directory marker should have been moved. + // We verify the renamed directory exists instead of checking the original doesn't. + } + + public function test_rename_directory_with_multiple_files_via_stream_wrapper() { + $upload_dir = wp_upload_dir(); + $test_dir = $upload_dir['path'] . '/multi-dir'; + $renamed_dir = $upload_dir['path'] . '/multi-renamed'; + + // Create a directory with multiple files + mkdir( $test_dir, 0755, true ); + $files = [ 'file1.txt', 'file2.txt', 'file3.txt', 'nested/deep/file4.txt' ]; + $contents = [ 'content1', 'content2', 'content3', 'content4' ]; + + foreach ( $files as $index => $file ) { + $file_path = $test_dir . '/' . $file; + $dir_path = dirname( $file_path ); + if ( ! is_dir( $dir_path ) ) { + mkdir( $dir_path, 0755, true ); + } + file_put_contents( $file_path, $contents[ $index ] ); + } + + // Rename the directory + $result = rename( $test_dir, $renamed_dir ); + $this->assertTrue( $result, 'Directory with multiple files rename should succeed' ); + + // Verify all files were moved correctly + foreach ( $files as $index => $file ) { + $renamed_file = $renamed_dir . '/' . $file; + $this->assertTrue( file_exists( $renamed_file ), "Renamed file {$file} should exist" ); + $this->assertEquals( $contents[ $index ], file_get_contents( $renamed_file ), "File {$file} content should match" ); + } + + // Verify original directory files don't exist + foreach ( $files as $file ) { + $original_file = $test_dir . '/' . $file; + $this->assertFalse( file_exists( $original_file ), "Original file {$file} should not exist" ); + } + } } From ea5111c2a45788126c7bfcb9e111eb037af8a3fd Mon Sep 17 00:00:00 2001 From: Lucas Carvalho Date: Fri, 16 Jan 2026 21:10:33 -0300 Subject: [PATCH 3/6] refactor: standardize variable naming in Stream_Wrapper for consistency --- inc/class-stream-wrapper.php | 136 +++++++++++++++++------------------ 1 file changed, 68 insertions(+), 68 deletions(-) diff --git a/inc/class-stream-wrapper.php b/inc/class-stream-wrapper.php index f479a5a7..848f30b1 100644 --- a/inc/class-stream-wrapper.php +++ b/inc/class-stream-wrapper.php @@ -751,12 +751,12 @@ public function rename( $path_from, $path_to ) { // PHP will not allow rename across wrapper types, so we can safely // assume $path_from and $path_to have the same protocol $this->initProtocol( $path_from ); - $partsFrom = $this->withPath( $path_from ); - $partsTo = $this->withPath( $path_to ); + $parts_from = $this->withPath( $path_from ); + $parts_to = $this->withPath( $path_to ); $this->clearCacheKey( $path_from ); $this->clearCacheKey( $path_to ); - if ( ! $partsFrom['Key'] || ! $partsTo['Key'] ) { + if ( ! $parts_from['Key'] || ! $parts_to['Key'] ) { return $this->triggerError( 'The Amazon S3 stream wrapper only ' . 'supports copying objects' @@ -764,29 +764,29 @@ public function rename( $path_from, $path_to ) { } return $this->boolCall( - function () use ( $partsFrom, $partsTo ) { + function () use ( $parts_from, $parts_to ) { $options = $this->getOptions( true ); $client = $this->getClient(); $acl = isset( $options['acl'] ) ? $options['acl'] : 'private'; // Normalize keys - ensure trailing slash for directories - $fromKey = rtrim( $partsFrom['Key'], '/' ); - $toKey = rtrim( $partsTo['Key'], '/' ); + $from_key = rtrim( $parts_from['Key'], '/' ); + $to_key = rtrim( $parts_to['Key'], '/' ); $existsAsFile = $client->doesObjectExistV2( - $partsFrom['Bucket'], - $partsFrom['Key'], + $parts_from['Bucket'], + $parts_from['Key'], false, $options ); - $isDirectory = ! $existsAsFile && $this->isDirectoryPrefix( $partsFrom['Bucket'], $fromKey ); + $isDirectory = ! $existsAsFile && $this->isDirectoryPrefix( $parts_from['Bucket'], $from_key ); if ( $isDirectory ) { - return $this->renameDirectory( $client, $partsFrom, $partsTo, $fromKey, $toKey, $acl, $options ); + return $this->renameDirectory( $client, $parts_from, $parts_to, $from_key, $to_key, $acl, $options ); } - return $this->renameFile( $client, $partsFrom, $partsTo, $acl, $options ); + return $this->renameFile( $client, $parts_from, $parts_to, $acl, $options ); } ); } @@ -796,32 +796,32 @@ function () use ( $partsFrom, $partsTo ) { * Uses batch operations for better S3 performance. * * @param S3ClientInterface $client S3 client instance - * @param array{Bucket: string, Key: string} $partsFrom Source path parts - * @param array{Bucket: string, Key: string} $partsTo Destination path parts - * @param string $fromKey Normalized source key - * @param string $toKey Normalized destination key + * @param array{Bucket: string, Key: string} $parts_from Source path parts + * @param array{Bucket: string, Key: string} $parts_to Destination path parts + * @param string $from_key Normalized source key + * @param string $to_key Normalized destination key * @param string $acl ACL for copied objects * @param array $options Additional S3 options * @return bool True on success */ private function renameDirectory( S3ClientInterface $client, - array $partsFrom, - array $partsTo, - string $fromKey, - string $toKey, + array $parts_from, + array $parts_to, + string $from_key, + string $to_key, string $acl, array $options ) : bool { - $fromPrefix = $fromKey . '/'; - $toPrefix = $toKey . '/'; - $copyCommands = []; - $objectsToDelete = []; - $cacheKeysToClear = []; + $from_prefix = $from_key . '/'; + $to_prefix = $to_key . '/'; + $copy_commands = []; + $objects_to_delete = []; + $cache_keys_to_clear = []; $paginator = $client->getPaginator( 'ListObjectsV2', [ - 'Bucket' => $partsFrom['Bucket'], - 'Prefix' => $fromPrefix, + 'Bucket' => $parts_from['Bucket'], + 'Prefix' => $from_prefix, ] ); foreach ( $paginator as $result ) { @@ -833,62 +833,62 @@ private function renameDirectory( $contents = $result['Contents']; foreach ( $contents as $object ) { /** @var array{Key: string} $object */ - /** @var string $oldKey */ - $oldKey = $object['Key']; - /** @var string $newKey */ - $newKey = str_replace( $fromPrefix, $toPrefix, $oldKey ); + /** @var string $old_key */ + $old_key = $object['Key']; + /** @var string $new_key */ + $new_key = str_replace( $from_prefix, $to_prefix, $old_key ); // Prepare copy command for batch execution - $copyCommands[] = $client->getCommand( 'CopyObject', [ - 'Bucket' => $partsTo['Bucket'], - 'Key' => $newKey, - 'CopySource' => "{$partsFrom['Bucket']}/{$oldKey}", + $copy_commands[] = $client->getCommand( 'CopyObject', [ + 'Bucket' => $parts_to['Bucket'], + 'Key' => $new_key, + 'CopySource' => "{$parts_from['Bucket']}/{$old_key}", 'ACL' => $acl, ] + $options ); - $objectsToDelete[] = [ 'Key' => $oldKey ]; - $cacheKeysToClear[] = "{$this->protocol}://{$partsFrom['Bucket']}/{$oldKey}"; - $cacheKeysToClear[] = "{$this->protocol}://{$partsTo['Bucket']}/{$newKey}"; + $objects_to_delete[] = [ 'Key' => $old_key ]; + $cache_keys_to_clear[] = "{$this->protocol}://{$parts_from['Bucket']}/{$old_key}"; + $cache_keys_to_clear[] = "{$this->protocol}://{$parts_to['Bucket']}/{$new_key}"; } } // Handle directory marker (empty object with key ending in "/") if it exists - $directoryMarkerKey = $fromPrefix; - if ( $client->doesObjectExistV2( $partsFrom['Bucket'], $directoryMarkerKey, false, $options ) ) { - $directoryMarkerNewKey = $toPrefix; - $copyCommands[] = $client->getCommand( 'CopyObject', [ - 'Bucket' => $partsTo['Bucket'], - 'Key' => $directoryMarkerNewKey, - 'CopySource' => "{$partsFrom['Bucket']}/{$directoryMarkerKey}", + $directory_marker_key = $from_prefix; + if ( $client->doesObjectExistV2( $parts_from['Bucket'], $directory_marker_key, false, $options ) ) { + $directory_marker_new_key = $to_prefix; + $copy_commands[] = $client->getCommand( 'CopyObject', [ + 'Bucket' => $parts_to['Bucket'], + 'Key' => $directory_marker_new_key, + 'CopySource' => "{$parts_from['Bucket']}/{$directory_marker_key}", 'ACL' => $acl, ] + $options ); - $objectsToDelete[] = [ 'Key' => $directoryMarkerKey ]; - $cacheKeysToClear[] = "{$this->protocol}://{$partsFrom['Bucket']}/{$directoryMarkerKey}"; - $cacheKeysToClear[] = "{$this->protocol}://{$partsTo['Bucket']}/{$directoryMarkerNewKey}"; + $objects_to_delete[] = [ 'Key' => $directory_marker_key ]; + $cache_keys_to_clear[] = "{$this->protocol}://{$parts_from['Bucket']}/{$directory_marker_key}"; + $cache_keys_to_clear[] = "{$this->protocol}://{$parts_to['Bucket']}/{$directory_marker_new_key}"; } - if ( empty( $copyCommands ) ) { + if ( empty( $copy_commands ) ) { return true; } // Execute all copies in parallel using CommandPool - CommandPool::batch( $client, $copyCommands ); + CommandPool::batch( $client, $copy_commands ); // Clear cache for all affected keys - foreach ( $cacheKeysToClear as $cacheKey ) { - $this->clearCacheKey( $cacheKey ); + foreach ( $cache_keys_to_clear as $cache_key ) { + $this->clearCacheKey( $cache_key ); } // Delete all original objects using batch deleteObjects (up to 1000 per request) - $maxDeleteBatch = 1000; - $deleteBatches = array_chunk( $objectsToDelete, $maxDeleteBatch ); + $max_delete_batch = 1000; + $delete_batches = array_chunk( $objects_to_delete, $max_delete_batch ); - foreach ( $deleteBatches as $deleteBatch ) { + foreach ( $delete_batches as $delete_batch ) { $client->deleteObjects( [ - 'Bucket' => $partsFrom['Bucket'], + 'Bucket' => $parts_from['Bucket'], 'Delete' => [ - 'Objects' => $deleteBatch, + 'Objects' => $delete_batch, ], ] + $options ); } @@ -900,35 +900,35 @@ private function renameDirectory( * Rename a single file by copying and then deleting the original. * * @param S3ClientInterface $client S3 client instance - * @param array{Bucket: string, Key: string} $partsFrom Source path parts - * @param array{Bucket: string, Key: string} $partsTo Destination path parts + * @param array{Bucket: string, Key: string} $parts_from Source path parts + * @param array{Bucket: string, Key: string} $parts_to Destination path parts * @param string $acl ACL for copied object * @param array $options Additional S3 options * @return bool True on success */ private function renameFile( S3ClientInterface $client, - array $partsFrom, - array $partsTo, + array $parts_from, + array $parts_to, string $acl, array $options ) : bool { $client->copy( - $partsFrom['Bucket'], - $partsFrom['Key'], - $partsTo['Bucket'], - $partsTo['Key'], + $parts_from['Bucket'], + $parts_from['Key'], + $parts_to['Bucket'], + $parts_to['Key'], $acl, $options ); $client->deleteObject( [ - 'Bucket' => $partsFrom['Bucket'], - 'Key' => $partsFrom['Key'], + 'Bucket' => $parts_from['Bucket'], + 'Key' => $parts_from['Key'], ] + $options ); - $this->clearCacheKey( "{$this->protocol}://{$partsFrom['Bucket']}/{$partsFrom['Key']}" ); - $this->clearCacheKey( "{$this->protocol}://{$partsTo['Bucket']}/{$partsTo['Key']}" ); + $this->clearCacheKey( "{$this->protocol}://{$parts_from['Bucket']}/{$parts_from['Key']}" ); + $this->clearCacheKey( "{$this->protocol}://{$parts_to['Bucket']}/{$parts_to['Key']}" ); return true; } From 23c3fdf565f1215a6f936bac5e67e9dbd7a5af80 Mon Sep 17 00:00:00 2001 From: Lucas Carvalho Date: Mon, 19 Jan 2026 14:48:47 -0300 Subject: [PATCH 4/6] feat: add encodeCopySource method to Stream_Wrapper for proper URL encoding of S3 object keys --- inc/class-stream-wrapper.php | 20 ++++- tests/test-s3-uploads-stream-wrapper.php | 94 ++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 2 deletions(-) diff --git a/inc/class-stream-wrapper.php b/inc/class-stream-wrapper.php index 848f30b1..25cf39d9 100644 --- a/inc/class-stream-wrapper.php +++ b/inc/class-stream-wrapper.php @@ -842,7 +842,7 @@ private function renameDirectory( $copy_commands[] = $client->getCommand( 'CopyObject', [ 'Bucket' => $parts_to['Bucket'], 'Key' => $new_key, - 'CopySource' => "{$parts_from['Bucket']}/{$old_key}", + 'CopySource' => $this->encodeCopySource( $parts_from['Bucket'], $old_key ), 'ACL' => $acl, ] + $options ); @@ -859,7 +859,7 @@ private function renameDirectory( $copy_commands[] = $client->getCommand( 'CopyObject', [ 'Bucket' => $parts_to['Bucket'], 'Key' => $directory_marker_new_key, - 'CopySource' => "{$parts_from['Bucket']}/{$directory_marker_key}", + 'CopySource' => $this->encodeCopySource( $parts_from['Bucket'], $directory_marker_key ), 'ACL' => $acl, ] + $options ); @@ -1384,4 +1384,20 @@ private function getSize() { return $size !== null ? $size : $this->size; } + + /** + * Encode a key for use in CopySource parameter. + * URL-encodes each path segment separately to preserve slashes. + * + * @param string $bucket The bucket name + * @param string $key The object key (may contain spaces, special chars, etc.) + * @return string The encoded CopySource string (bucket/encoded-key) + */ + private function encodeCopySource( string $bucket, string $key ) : string { + $parts = explode( '/', $key ); + $encoded_parts = array_map( 'rawurlencode', $parts ); + $encoded_key = implode( '/', $encoded_parts ); + + return "{$bucket}/{$encoded_key}"; + } } diff --git a/tests/test-s3-uploads-stream-wrapper.php b/tests/test-s3-uploads-stream-wrapper.php index 1be545e4..b9121584 100644 --- a/tests/test-s3-uploads-stream-wrapper.php +++ b/tests/test-s3-uploads-stream-wrapper.php @@ -242,4 +242,98 @@ public function test_rename_directory_with_multiple_files_via_stream_wrapper() { $this->assertFalse( file_exists( $original_file ), "Original file {$file} should not exist" ); } } + + public function test_encode_copy_source_encodes_file_names() { + $wrapper = new S3_Uploads\Stream_Wrapper(); + $reflection = new ReflectionClass( $wrapper ); + $method = $reflection->getMethod( 'encodeCopySource' ); + $method->setAccessible( true ); + + $bucket = 'test-bucket'; + + // Test 1: File with spaces (the main problematic case) + $key1 = 'Vector Strips Module I with expressions 2_Page_1.jpg'; + $result1 = $method->invoke( $wrapper, $bucket, $key1 ); + $this->assertStringContainsString( '%20', $result1, 'Spaces must be URL-encoded as %20' ); + $this->assertStringNotContainsString( ' ', $result1, 'CopySource should not contain unencoded spaces' ); + $expected1 = 'test-bucket/Vector%20Strips%20Module%20I%20with%20expressions%202_Page_1.jpg'; + $this->assertEquals( $expected1, $result1, 'CopySource must have spaces properly encoded' ); + + // Test 2: File with special characters (parentheses) + $key2 = 'file with spaces (1).txt'; + $result2 = $method->invoke( $wrapper, $bucket, $key2 ); + $this->assertStringContainsString( '%28', $result2, 'Opening parenthesis must be encoded' ); + $this->assertStringContainsString( '%29', $result2, 'Closing parenthesis must be encoded' ); + $this->assertStringNotContainsString( '(', $result2, 'CopySource should not contain unencoded opening parenthesis' ); + $this->assertStringNotContainsString( ')', $result2, 'CopySource should not contain unencoded closing parenthesis' ); + + // Test 3: File in subdirectory - slashes must be preserved + $key3 = 'subdir/file with spaces.txt'; + $result3 = $method->invoke( $wrapper, $bucket, $key3 ); + $this->assertStringContainsString( '/', $result3, 'Slashes must be preserved in CopySource' ); + $this->assertStringNotContainsString( '%2F', $result3, 'Slashes must NOT be encoded as %2F' ); + $this->assertStringContainsString( '%20', $result3, 'Spaces in path segments must be encoded' ); + } + + /** + * @note Amazon minio does not replicates the issue with naming files but it happens in AWS S3. + */ + public function test_rename_directory_implementation_uses_encode_copy_source() { + // Read the source code to verify encodeCopySource is used + $source_file = dirname( dirname( __FILE__ ) ) . '/inc/class-stream-wrapper.php'; + $source_code = file_get_contents( $source_file ); + + // Find the renameDirectory method + $rename_directory_start = strpos( $source_code, 'private function renameDirectory' ); + $this->assertNotFalse( $rename_directory_start, 'renameDirectory method must exist' ); + + // Find the end of the method (next private/public function or closing brace at class level) + $method_code = substr( $source_code, $rename_directory_start ); + $next_function = strpos( $method_code, "\n\tprivate function " ); + $next_public = strpos( $method_code, "\n\tpublic function " ); + $end_pos = false; + if ( $next_function !== false ) { + $end_pos = $next_function; + } + if ( $next_public !== false && ( $end_pos === false || $next_public < $end_pos ) ) { + $end_pos = $next_public; + } + if ( $end_pos !== false ) { + $method_code = substr( $method_code, 0, $end_pos ); + } + + // Verify encodeCopySource is called in the method + $this->assertStringContainsString( + 'encodeCopySource', + $method_code, + 'renameDirectory must call encodeCopySource for CopySource parameter. Without this, files with spaces will fail.' + ); + + // Verify it's used for the CopySource parameter, not just mentioned + $this->assertStringContainsString( + "'CopySource' => \$this->encodeCopySource", + $method_code, + 'CopySource parameter must use encodeCopySource method' + ); + } + + /** + * @note Amazon minio does not replicates the issue with naming files but it happens in AWS S3. + */ + public function test_rename_file_implementation_consistency() { + // This test ensures we're aware of how single file renames work + // The copy() method may handle encoding, but directory renames definitely need it + $upload_dir = wp_upload_dir(); + $test_file = $upload_dir['path'] . '/Vector Strips Module I with expressions 2_Page_1.jpg'; + $renamed_file = $upload_dir['path'] . '/Vector Strips Module I with expressions 2_Page_1_renamed.jpg'; + + // Create and rename the file + file_put_contents( $test_file, 'test content' ); + $result = rename( $test_file, $renamed_file ); + + // This should work - if it doesn't, there's a problem + $this->assertTrue( $result, 'File rename with spaces should succeed' ); + $this->assertTrue( file_exists( $renamed_file ), 'Renamed file should exist' ); + $this->assertFalse( file_exists( $test_file ), 'Original file should not exist' ); + } } From 702cf27ed8b932d4bf3c6f4df1425c7a61be8acd Mon Sep 17 00:00:00 2001 From: Lucas Carvalho Date: Mon, 26 Jan 2026 19:03:57 -0300 Subject: [PATCH 5/6] refactor: streamline directory handling in Stream_Wrapper by removing unnecessary checks and normalizing key slashes --- inc/class-stream-wrapper.php | 29 ++++------------------------- 1 file changed, 4 insertions(+), 25 deletions(-) diff --git a/inc/class-stream-wrapper.php b/inc/class-stream-wrapper.php index 25cf39d9..cd186b01 100644 --- a/inc/class-stream-wrapper.php +++ b/inc/class-stream-wrapper.php @@ -769,23 +769,18 @@ function () use ( $parts_from, $parts_to ) { $client = $this->getClient(); $acl = isset( $options['acl'] ) ? $options['acl'] : 'private'; - // Normalize keys - ensure trailing slash for directories + // Normalize keys - remove trailing slashes $from_key = rtrim( $parts_from['Key'], '/' ); $to_key = rtrim( $parts_to['Key'], '/' ); - $existsAsFile = $client->doesObjectExistV2( - $parts_from['Bucket'], - $parts_from['Key'], - false, - $options - ); - - $isDirectory = ! $existsAsFile && $this->isDirectoryPrefix( $parts_from['Bucket'], $from_key ); + $isDirectory = $this->isDirectoryPrefix( $parts_from['Bucket'], $from_key ); if ( $isDirectory ) { return $this->renameDirectory( $client, $parts_from, $parts_to, $from_key, $to_key, $acl, $options ); } + $parts_from['Key'] = $from_key; + $parts_to['Key'] = $to_key; return $this->renameFile( $client, $parts_from, $parts_to, $acl, $options ); } ); @@ -852,22 +847,6 @@ private function renameDirectory( } } - // Handle directory marker (empty object with key ending in "/") if it exists - $directory_marker_key = $from_prefix; - if ( $client->doesObjectExistV2( $parts_from['Bucket'], $directory_marker_key, false, $options ) ) { - $directory_marker_new_key = $to_prefix; - $copy_commands[] = $client->getCommand( 'CopyObject', [ - 'Bucket' => $parts_to['Bucket'], - 'Key' => $directory_marker_new_key, - 'CopySource' => $this->encodeCopySource( $parts_from['Bucket'], $directory_marker_key ), - 'ACL' => $acl, - ] + $options ); - - $objects_to_delete[] = [ 'Key' => $directory_marker_key ]; - $cache_keys_to_clear[] = "{$this->protocol}://{$parts_from['Bucket']}/{$directory_marker_key}"; - $cache_keys_to_clear[] = "{$this->protocol}://{$parts_to['Bucket']}/{$directory_marker_new_key}"; - } - if ( empty( $copy_commands ) ) { return true; } From d09c5a92be397e76d487ae1615e86af80bef2a49 Mon Sep 17 00:00:00 2001 From: Lucas Carvalho Date: Thu, 12 Mar 2026 20:17:52 -0300 Subject: [PATCH 6/6] Clarify error message for unsupported bucket root renaming and update parameter types for source and destination paths in Stream_Wrapper --- inc/class-stream-wrapper.php | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/inc/class-stream-wrapper.php b/inc/class-stream-wrapper.php index 231e7c5e..473a6cb9 100644 --- a/inc/class-stream-wrapper.php +++ b/inc/class-stream-wrapper.php @@ -763,8 +763,8 @@ public function rename( $path_from, $path_to ) { if ( $partsFrom['Key'] === null || $partsFrom['Key'] === '' || $partsTo['Key'] === null || $partsTo['Key'] === '' ) { return $this->triggerError( - 'The Amazon S3 stream wrapper only ' - . 'supports copying objects' + 'Renaming a bucket root is not supported. ' + . 'You must specify a path in the form of s3://bucket/key' ); } @@ -796,8 +796,8 @@ function () use ( $partsFrom, $partsTo ) { * Uses batch operations for better S3 performance. * * @param S3ClientInterface $client S3 client instance - * @param array{Bucket: string, Key: string} $parts_from Source path parts - * @param array{Bucket: string, Key: string} $parts_to Destination path parts + * @param array{Bucket: string, Key: string, ...} $parts_from Source path parts + * @param array{Bucket: string, Key: string, ...} $parts_to Destination path parts * @param string $from_key Normalized source key * @param string $to_key Normalized destination key * @param string $acl ACL for copied objects @@ -884,8 +884,8 @@ private function renameDirectory( * Rename a single file by copying and then deleting the original. * * @param S3ClientInterface $client S3 client instance - * @param array{Bucket: string, Key: string} $parts_from Source path parts - * @param array{Bucket: string, Key: string} $parts_to Destination path parts + * @param array{Bucket: string, Key: string, ...} $parts_from Source path parts + * @param array{Bucket: string, Key: string, ...} $parts_to Destination path parts * @param string $acl ACL for copied object * @param array $options Additional S3 options * @return bool True on success @@ -1257,7 +1257,8 @@ private function isDirectoryPrefix( string $bucket, string $key ) : bool { 'MaxKeys' => 1, ] ); - return ! empty( $result['Contents'] ) || ! empty( $result['CommonPrefixes'] ); + return ( is_array( $result['Contents'] ) && count( $result['Contents'] ) > 0 ) + || ( is_array( $result['CommonPrefixes'] ) && count( $result['CommonPrefixes'] ) > 0 ); } /**