diff --git a/inc/class-stream-wrapper.php b/inc/class-stream-wrapper.php index e6a2499c..473a6cb9 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; @@ -743,13 +744,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 ) { @@ -763,34 +763,158 @@ 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' ); } return $this->boolCall( function () use ( $partsFrom, $partsTo ) { $options = $this->getOptions( true ); - // Copy the object and allow overriding default parameters if - // desired, but by default copy metadata - $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; + $client = $this->getClient(); + $acl = isset( $options['acl'] ) ? $options['acl'] : 'private'; + + // Normalize keys - remove trailing slashes + $from_key = rtrim( $partsFrom['Key'], '/' ); + $to_key = rtrim( $partsTo['Key'], '/' ); + + $isDirectory = $this->isDirectoryPrefix( $partsFrom['Bucket'], $from_key ); + + if ( $isDirectory ) { + return $this->renameDirectory( $client, $partsFrom, $partsTo, $from_key, $to_key, $acl, $options ); + } + + $partsFrom['Key'] = $from_key; + $partsTo['Key'] = $to_key; + return $this->renameFile( $client, $partsFrom, $partsTo, $acl, $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, ...} $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 $parts_from, + array $parts_to, + string $from_key, + string $to_key, + string $acl, + array $options + ) : bool { + $from_prefix = $from_key . '/'; + $to_prefix = $to_key . '/'; + $copy_commands = []; + $objects_to_delete = []; + $cache_keys_to_clear = []; + + $paginator = $client->getPaginator( 'ListObjectsV2', [ + 'Bucket' => $parts_from['Bucket'], + 'Prefix' => $from_prefix, + ] ); + + foreach ( $paginator as $result ) { + if ( ! isset( $result['Contents'] ) || ! is_array( $result['Contents'] ) ) { + continue; } + + /** @var list $contents */ + $contents = $result['Contents']; + foreach ( $contents as $object ) { + /** @var array{Key: string} $object */ + /** @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 + $copy_commands[] = $client->getCommand( 'CopyObject', [ + 'Bucket' => $parts_to['Bucket'], + 'Key' => $new_key, + 'CopySource' => $this->encodeCopySource( $parts_from['Bucket'], $old_key ), + 'ACL' => $acl, + ] + $options ); + + $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}"; + } + } + + if ( empty( $copy_commands ) ) { + return true; + } + + // Execute all copies in parallel using CommandPool + CommandPool::batch( $client, $copy_commands ); + + // Clear cache for all affected keys + foreach ( $cache_keys_to_clear as $cache_key ) { + $this->clearCacheKey( $cache_key ); + } + + // Delete all original objects using batch deleteObjects (up to 1000 per request) + $max_delete_batch = 1000; + $delete_batches = array_chunk( $objects_to_delete, $max_delete_batch ); + + foreach ( $delete_batches as $delete_batch ) { + $client->deleteObjects( [ + 'Bucket' => $parts_from['Bucket'], + 'Delete' => [ + 'Objects' => $delete_batch, + ], + ] + $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, ...} $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 $parts_from, + array $parts_to, + string $acl, + array $options + ) : bool { + $client->copy( + $parts_from['Bucket'], + $parts_from['Key'], + $parts_to['Bucket'], + $parts_to['Key'], + $acl, + $options ); + + $client->deleteObject( [ + 'Bucket' => $parts_from['Bucket'], + 'Key' => $parts_from['Key'], + ] + $options ); + + $this->clearCacheKey( "{$this->protocol}://{$parts_from['Bucket']}/{$parts_from['Key']}" ); + $this->clearCacheKey( "{$this->protocol}://{$parts_to['Bucket']}/{$parts_to['Key']}" ); + + return true; } public function stream_cast( int $cast_as ) : bool { @@ -1118,6 +1242,25 @@ 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 ( is_array( $result['Contents'] ) && count( $result['Contents'] ) > 0 ) + || ( is_array( $result['CommonPrefixes'] ) && count( $result['CommonPrefixes'] ) > 0 ); + } + /** * Determine the most appropriate ACL based on a file mode. * @@ -1226,4 +1369,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 172ff33a..1189925f 100644 --- a/tests/test-s3-uploads-stream-wrapper.php +++ b/tests/test-s3-uploads-stream-wrapper.php @@ -155,4 +155,186 @@ 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" ); + } + } + + 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' ); + } }