From c0851265b4edeeab80c7316b1cdfdb55f74c9635 Mon Sep 17 00:00:00 2001 From: Frank de Jonge Date: Thu, 19 Oct 2023 19:51:35 +0200 Subject: [PATCH 1/5] Prevent copy and move to same destinations as the source. --- src/Filesystem.php | 27 +++++++++++++++++---------- src/FilesystemTest.php | 20 ++++++++++++++++++++ src/UnableToCopyFile.php | 14 ++++++++++++++ src/UnableToMoveFile.php | 18 ++++++++++++++++++ 4 files changed, 69 insertions(+), 10 deletions(-) diff --git a/src/Filesystem.php b/src/Filesystem.php index 94fae15ef..9bac9809a 100644 --- a/src/Filesystem.php +++ b/src/Filesystem.php @@ -119,20 +119,27 @@ private function pipeListing(string $location, bool $deep, iterable $listing): G public function move(string $source, string $destination, array $config = []): void { - $this->adapter->move( - $this->pathNormalizer->normalizePath($source), - $this->pathNormalizer->normalizePath($destination), - $this->config->extend($config) - ); + $from = $this->pathNormalizer->normalizePath($source); + $to = $this->pathNormalizer->normalizePath($destination); + + if ($from === $to) { + throw UnableToMoveFile::sourceAndDestinationAreTheSame($source, $destination); + throw UnableToMoveFile::because('Source and destination are the same', $source, $destination); + } + + $this->adapter->move($from, $to, $this->config->extend($config)); } public function copy(string $source, string $destination, array $config = []): void { - $this->adapter->copy( - $this->pathNormalizer->normalizePath($source), - $this->pathNormalizer->normalizePath($destination), - $this->config->extend($config) - ); + $from = $this->pathNormalizer->normalizePath($source); + $to = $this->pathNormalizer->normalizePath($destination); + + if ($from === $to) { + throw UnableToCopyFile::sourceAndDestinationAreTheSame($source, $destination); + } + + $this->adapter->copy($from, $to, $this->config->extend($config)); } public function lastModified(string $path): int diff --git a/src/FilesystemTest.php b/src/FilesystemTest.php index e7ae891ed..46a7fd6e9 100644 --- a/src/FilesystemTest.php +++ b/src/FilesystemTest.php @@ -480,6 +480,26 @@ public function publicUrl(string $path, Config $config): string self::assertSame('custom/file.txt', $filesystem->publicUrl('file.txt')); } + /** + * @test + */ + public function copying_from_and_to_the_same_location_fails(): void + { + $this->expectExceptionObject(UnableToMoveFile::sourceAndDestinationAreTheSame('from.txt', 'from.txt')); + + $this->filesystem->move('from.txt', 'from.txt'); + } + + /** + * @test + */ + public function moving_from_and_to_the_same_location_fails(): void + { + $this->expectExceptionObject(UnableToCopyFile::sourceAndDestinationAreTheSame('from.txt', 'from.txt')); + + $this->filesystem->copy('from.txt', 'from.txt'); + } + /** * @test */ diff --git a/src/UnableToCopyFile.php b/src/UnableToCopyFile.php index 2180c1e36..a27d66e48 100644 --- a/src/UnableToCopyFile.php +++ b/src/UnableToCopyFile.php @@ -41,6 +41,20 @@ public static function fromLocationTo( return $e; } + public static function sourceAndDestinationAreTheSame(string $source, string $destination): UnableToCopyFile + { + return UnableToCopyFile::because('Source and destination are the same', $source, $destination); + } + + public static function because(string $reason, string $sourcePath, string $destinationPath): UnableToCopyFile + { + $e = new static("Unable to copy file from $sourcePath to $destinationPath, because $reason"); + $e->source = $sourcePath; + $e->destination = $destinationPath; + + return $e; + } + public function operation(): string { return FilesystemOperationFailed::OPERATION_COPY; diff --git a/src/UnableToMoveFile.php b/src/UnableToMoveFile.php index 72792f4ab..f5c68097c 100644 --- a/src/UnableToMoveFile.php +++ b/src/UnableToMoveFile.php @@ -19,6 +19,11 @@ final class UnableToMoveFile extends RuntimeException implements FilesystemOpera */ private $destination; + public static function sourceAndDestinationAreTheSame(string $source, string $destination): UnableToMoveFile + { + return UnableToMoveFile::because('Source and destination are the same', $source, $destination); + } + public function source(): string { return $this->source; @@ -42,6 +47,19 @@ public static function fromLocationTo( return $e; } + public static function because( + string $reason, + string $sourcePath, + string $destinationPath, + ): UnableToMoveFile { + $message = "Unable to move file from $sourcePath to $destinationPath, because $reason"; + $e = new static($message); + $e->source = $sourcePath; + $e->destination = $destinationPath; + + return $e; + } + public function operation(): string { return FilesystemOperationFailed::OPERATION_MOVE; From 01e7c1707cf0bd27784320dd379c78671b33e186 Mon Sep 17 00:00:00 2001 From: Frank de Jonge Date: Thu, 19 Oct 2023 21:19:10 +0200 Subject: [PATCH 2/5] Remove redundant throw statement. --- src/Filesystem.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Filesystem.php b/src/Filesystem.php index 9bac9809a..da4fef9ea 100644 --- a/src/Filesystem.php +++ b/src/Filesystem.php @@ -124,7 +124,6 @@ public function move(string $source, string $destination, array $config = []): v if ($from === $to) { throw UnableToMoveFile::sourceAndDestinationAreTheSame($source, $destination); - throw UnableToMoveFile::because('Source and destination are the same', $source, $destination); } $this->adapter->move($from, $to, $this->config->extend($config)); From 8e5ac91cdb8a95ce5d268e775625b1614f6f9ecd Mon Sep 17 00:00:00 2001 From: Frank de Jonge Date: Thu, 19 Oct 2023 22:07:13 +0200 Subject: [PATCH 3/5] Added ability to set resolution strategy for copy and move conflicts. --- src/Config.php | 2 + src/Filesystem.php | 23 +++++++- src/FilesystemTest.php | 59 +++++++++++++++++-- src/InMemory/InMemoryFilesystemAdapter.php | 8 +-- src/Local/LocalFilesystemAdapter.php | 4 +- ...esolveSameSourceAndDestinationConflict.php | 11 ++++ 6 files changed, 94 insertions(+), 13 deletions(-) create mode 100644 src/ResolveSameSourceAndDestinationConflict.php diff --git a/src/Config.php b/src/Config.php index ace07099b..3c8765757 100644 --- a/src/Config.php +++ b/src/Config.php @@ -8,6 +8,8 @@ class Config { + public const OPTION_COPY_DESTINATION_SAME_AS_SOURCE = 'copy_destination_same_as_source'; + public const OPTION_MOVE_DESTINATION_SAME_AS_SOURCE = 'move_destination_same_as_source'; public const OPTION_VISIBILITY = 'visibility'; public const OPTION_DIRECTORY_VISIBILITY = 'directory_visibility'; diff --git a/src/Filesystem.php b/src/Filesystem.php index da4fef9ea..0ed17a32b 100644 --- a/src/Filesystem.php +++ b/src/Filesystem.php @@ -123,9 +123,17 @@ public function move(string $source, string $destination, array $config = []): v $to = $this->pathNormalizer->normalizePath($destination); if ($from === $to) { - throw UnableToMoveFile::sourceAndDestinationAreTheSame($source, $destination); - } + $resolutionStrategy = $this->config->get( + Config::OPTION_MOVE_DESTINATION_SAME_AS_SOURCE, + ResolveSameSourceAndDestinationConflict::TRY, + ); + if ($resolutionStrategy === ResolveSameSourceAndDestinationConflict::FAIL) { + throw UnableToMoveFile::sourceAndDestinationAreTheSame($source, $destination); + } elseif ($resolutionStrategy === ResolveSameSourceAndDestinationConflict::IGNORE) { + return; + } + } $this->adapter->move($from, $to, $this->config->extend($config)); } @@ -135,7 +143,16 @@ public function copy(string $source, string $destination, array $config = []): v $to = $this->pathNormalizer->normalizePath($destination); if ($from === $to) { - throw UnableToCopyFile::sourceAndDestinationAreTheSame($source, $destination); + $resolutionStrategy = $this->config->get( + Config::OPTION_COPY_DESTINATION_SAME_AS_SOURCE, + ResolveSameSourceAndDestinationConflict::TRY, + ); + + if ($resolutionStrategy === ResolveSameSourceAndDestinationConflict::FAIL) { + throw UnableToCopyFile::sourceAndDestinationAreTheSame($source, $destination); + } elseif ($resolutionStrategy === ResolveSameSourceAndDestinationConflict::IGNORE) { + return; + } } $this->adapter->copy($from, $to, $this->config->extend($config)); diff --git a/src/FilesystemTest.php b/src/FilesystemTest.php index 46a7fd6e9..f9cb0bb02 100644 --- a/src/FilesystemTest.php +++ b/src/FilesystemTest.php @@ -485,9 +485,9 @@ public function publicUrl(string $path, Config $config): string */ public function copying_from_and_to_the_same_location_fails(): void { - $this->expectExceptionObject(UnableToMoveFile::sourceAndDestinationAreTheSame('from.txt', 'from.txt')); + $this->expectExceptionObject(UnableToCopyFile::fromLocationTo('from.txt', 'from.txt')); - $this->filesystem->move('from.txt', 'from.txt'); + $this->filesystem->copy('from.txt', 'from.txt'); } /** @@ -495,9 +495,9 @@ public function copying_from_and_to_the_same_location_fails(): void */ public function moving_from_and_to_the_same_location_fails(): void { - $this->expectExceptionObject(UnableToCopyFile::sourceAndDestinationAreTheSame('from.txt', 'from.txt')); + $this->expectExceptionObject(UnableToMoveFile::fromLocationTo('from.txt', 'from.txt')); - $this->filesystem->copy('from.txt', 'from.txt'); + $this->filesystem->move('from.txt', 'from.txt'); } /** @@ -611,6 +611,57 @@ public function not_being_able_to_generate_temporary_urls(): void $filesystem->temporaryUrl('some/file.txt', new DateTimeImmutable()); } + /** + * @test + */ + public function ignoring_same_paths_for_move_and_copy(): void + { + $this->expectNotToPerformAssertions(); + + $filesystem = new Filesystem( + new InMemoryFilesystemAdapter(), + [ + Config::OPTION_COPY_DESTINATION_SAME_AS_SOURCE => ResolveSameSourceAndDestinationConflict::IGNORE, + Config::OPTION_MOVE_DESTINATION_SAME_AS_SOURCE => ResolveSameSourceAndDestinationConflict::IGNORE, + ] + ); + + $filesystem->move('from.txt', 'from.txt'); + $filesystem->copy('from.txt', 'from.txt'); + } + + /** + * @test + */ + public function failing_same_paths_for_move(): void + { + $filesystem = new Filesystem( + new InMemoryFilesystemAdapter(), + [ + Config::OPTION_MOVE_DESTINATION_SAME_AS_SOURCE => ResolveSameSourceAndDestinationConflict::FAIL, + ] + ); + + $this->expectExceptionObject(UnableToMoveFile::fromLocationTo('from.txt', 'from.txt')); + $filesystem->move('from.txt', 'from.txt'); + } + + /** + * @test + */ + public function failing_same_paths_for_copy(): void + { + $filesystem = new Filesystem( + new InMemoryFilesystemAdapter(), + [ + Config::OPTION_COPY_DESTINATION_SAME_AS_SOURCE => ResolveSameSourceAndDestinationConflict::FAIL, + ] + ); + + $this->expectExceptionObject(UnableToCopyFile::fromLocationTo('from.txt', 'from.txt')); + $filesystem->copy('from.txt', 'from.txt'); + } + /** * @test */ diff --git a/src/InMemory/InMemoryFilesystemAdapter.php b/src/InMemory/InMemoryFilesystemAdapter.php index fb6d18d23..3dddd315c 100644 --- a/src/InMemory/InMemoryFilesystemAdapter.php +++ b/src/InMemory/InMemoryFilesystemAdapter.php @@ -221,15 +221,15 @@ public function listContents(string $path, bool $deep): iterable public function move(string $source, string $destination, Config $config): void { - $source = $this->preparePath($source); - $destination = $this->preparePath($destination); + $sourcePath = $this->preparePath($source); + $destinationPath = $this->preparePath($destination); if ( ! $this->fileExists($source) || $this->fileExists($destination)) { throw UnableToMoveFile::fromLocationTo($source, $destination); } - $this->files[$destination] = $this->files[$source]; - unset($this->files[$source]); + $this->files[$destinationPath] = $this->files[$sourcePath]; + unset($this->files[$sourcePath]); } public function copy(string $source, string $destination, Config $config): void diff --git a/src/Local/LocalFilesystemAdapter.php b/src/Local/LocalFilesystemAdapter.php index aeb1eabf5..c91a9bfa5 100644 --- a/src/Local/LocalFilesystemAdapter.php +++ b/src/Local/LocalFilesystemAdapter.php @@ -248,7 +248,7 @@ public function move(string $source, string $destination, Config $config): void ); if ( ! @rename($sourcePath, $destinationPath)) { - throw UnableToMoveFile::fromLocationTo($sourcePath, $destinationPath); + throw UnableToMoveFile::because(error_get_last()['message'] ?? 'unknown reason', $source, $destination); } } @@ -263,7 +263,7 @@ public function copy(string $source, string $destination, Config $config): void ); if ( ! @copy($sourcePath, $destinationPath)) { - throw UnableToCopyFile::fromLocationTo($sourcePath, $destinationPath); + throw UnableToCopyFile::because(error_get_last()['message'] ?? 'unknown', $source, $destination); } } diff --git a/src/ResolveSameSourceAndDestinationConflict.php b/src/ResolveSameSourceAndDestinationConflict.php new file mode 100644 index 000000000..84de79fd4 --- /dev/null +++ b/src/ResolveSameSourceAndDestinationConflict.php @@ -0,0 +1,11 @@ + Date: Thu, 19 Oct 2023 23:08:21 +0200 Subject: [PATCH 4/5] Allow method based config overwrites. --- src/Filesystem.php | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/Filesystem.php b/src/Filesystem.php index 0ed17a32b..0d429960a 100644 --- a/src/Filesystem.php +++ b/src/Filesystem.php @@ -119,11 +119,12 @@ private function pipeListing(string $location, bool $deep, iterable $listing): G public function move(string $source, string $destination, array $config = []): void { + $config = $this->config->extend($config); $from = $this->pathNormalizer->normalizePath($source); $to = $this->pathNormalizer->normalizePath($destination); if ($from === $to) { - $resolutionStrategy = $this->config->get( + $resolutionStrategy = $config->get( Config::OPTION_MOVE_DESTINATION_SAME_AS_SOURCE, ResolveSameSourceAndDestinationConflict::TRY, ); @@ -134,16 +135,18 @@ public function move(string $source, string $destination, array $config = []): v return; } } - $this->adapter->move($from, $to, $this->config->extend($config)); + + $this->adapter->move($from, $to, $config); } public function copy(string $source, string $destination, array $config = []): void { + $config = $this->config->extend($config); $from = $this->pathNormalizer->normalizePath($source); $to = $this->pathNormalizer->normalizePath($destination); if ($from === $to) { - $resolutionStrategy = $this->config->get( + $resolutionStrategy = $config->get( Config::OPTION_COPY_DESTINATION_SAME_AS_SOURCE, ResolveSameSourceAndDestinationConflict::TRY, ); @@ -155,7 +158,7 @@ public function copy(string $source, string $destination, array $config = []): v } } - $this->adapter->copy($from, $to, $this->config->extend($config)); + $this->adapter->copy($from, $to, $config); } public function lastModified(string $path): int From 532bb63356627b62607a821bc9c06f996b5baedc Mon Sep 17 00:00:00 2001 From: Frank de Jonge Date: Thu, 19 Oct 2023 23:10:45 +0200 Subject: [PATCH 5/5] More consice naming. --- src/Config.php | 4 ++-- src/Filesystem.php | 18 ++++++------------ src/FilesystemTest.php | 8 ++++---- ...ct.php => ResolveIdenticalPathConflict.php} | 2 +- 4 files changed, 13 insertions(+), 19 deletions(-) rename src/{ResolveSameSourceAndDestinationConflict.php => ResolveIdenticalPathConflict.php} (77%) diff --git a/src/Config.php b/src/Config.php index 3c8765757..a416c67e0 100644 --- a/src/Config.php +++ b/src/Config.php @@ -8,8 +8,8 @@ class Config { - public const OPTION_COPY_DESTINATION_SAME_AS_SOURCE = 'copy_destination_same_as_source'; - public const OPTION_MOVE_DESTINATION_SAME_AS_SOURCE = 'move_destination_same_as_source'; + public const OPTION_COPY_IDENTICAL_PATH = 'copy_destination_same_as_source'; + public const OPTION_MOVE_IDENTICAL_PATH = 'move_destination_same_as_source'; public const OPTION_VISIBILITY = 'visibility'; public const OPTION_DIRECTORY_VISIBILITY = 'directory_visibility'; diff --git a/src/Filesystem.php b/src/Filesystem.php index 0d429960a..0a8ef09ba 100644 --- a/src/Filesystem.php +++ b/src/Filesystem.php @@ -124,14 +124,11 @@ public function move(string $source, string $destination, array $config = []): v $to = $this->pathNormalizer->normalizePath($destination); if ($from === $to) { - $resolutionStrategy = $config->get( - Config::OPTION_MOVE_DESTINATION_SAME_AS_SOURCE, - ResolveSameSourceAndDestinationConflict::TRY, - ); + $resolutionStrategy = $config->get(Config::OPTION_MOVE_IDENTICAL_PATH, ResolveIdenticalPathConflict::TRY); - if ($resolutionStrategy === ResolveSameSourceAndDestinationConflict::FAIL) { + if ($resolutionStrategy === ResolveIdenticalPathConflict::FAIL) { throw UnableToMoveFile::sourceAndDestinationAreTheSame($source, $destination); - } elseif ($resolutionStrategy === ResolveSameSourceAndDestinationConflict::IGNORE) { + } elseif ($resolutionStrategy === ResolveIdenticalPathConflict::IGNORE) { return; } } @@ -146,14 +143,11 @@ public function copy(string $source, string $destination, array $config = []): v $to = $this->pathNormalizer->normalizePath($destination); if ($from === $to) { - $resolutionStrategy = $config->get( - Config::OPTION_COPY_DESTINATION_SAME_AS_SOURCE, - ResolveSameSourceAndDestinationConflict::TRY, - ); + $resolutionStrategy = $config->get(Config::OPTION_COPY_IDENTICAL_PATH, ResolveIdenticalPathConflict::TRY); - if ($resolutionStrategy === ResolveSameSourceAndDestinationConflict::FAIL) { + if ($resolutionStrategy === ResolveIdenticalPathConflict::FAIL) { throw UnableToCopyFile::sourceAndDestinationAreTheSame($source, $destination); - } elseif ($resolutionStrategy === ResolveSameSourceAndDestinationConflict::IGNORE) { + } elseif ($resolutionStrategy === ResolveIdenticalPathConflict::IGNORE) { return; } } diff --git a/src/FilesystemTest.php b/src/FilesystemTest.php index f9cb0bb02..b00705b14 100644 --- a/src/FilesystemTest.php +++ b/src/FilesystemTest.php @@ -621,8 +621,8 @@ public function ignoring_same_paths_for_move_and_copy(): void $filesystem = new Filesystem( new InMemoryFilesystemAdapter(), [ - Config::OPTION_COPY_DESTINATION_SAME_AS_SOURCE => ResolveSameSourceAndDestinationConflict::IGNORE, - Config::OPTION_MOVE_DESTINATION_SAME_AS_SOURCE => ResolveSameSourceAndDestinationConflict::IGNORE, + Config::OPTION_COPY_IDENTICAL_PATH => ResolveIdenticalPathConflict::IGNORE, + Config::OPTION_MOVE_IDENTICAL_PATH => ResolveIdenticalPathConflict::IGNORE, ] ); @@ -638,7 +638,7 @@ public function failing_same_paths_for_move(): void $filesystem = new Filesystem( new InMemoryFilesystemAdapter(), [ - Config::OPTION_MOVE_DESTINATION_SAME_AS_SOURCE => ResolveSameSourceAndDestinationConflict::FAIL, + Config::OPTION_MOVE_IDENTICAL_PATH => ResolveIdenticalPathConflict::FAIL, ] ); @@ -654,7 +654,7 @@ public function failing_same_paths_for_copy(): void $filesystem = new Filesystem( new InMemoryFilesystemAdapter(), [ - Config::OPTION_COPY_DESTINATION_SAME_AS_SOURCE => ResolveSameSourceAndDestinationConflict::FAIL, + Config::OPTION_COPY_IDENTICAL_PATH => ResolveIdenticalPathConflict::FAIL, ] ); diff --git a/src/ResolveSameSourceAndDestinationConflict.php b/src/ResolveIdenticalPathConflict.php similarity index 77% rename from src/ResolveSameSourceAndDestinationConflict.php rename to src/ResolveIdenticalPathConflict.php index 84de79fd4..849659b06 100644 --- a/src/ResolveSameSourceAndDestinationConflict.php +++ b/src/ResolveIdenticalPathConflict.php @@ -3,7 +3,7 @@ namespace League\Flysystem; -class ResolveSameSourceAndDestinationConflict +class ResolveIdenticalPathConflict { public const IGNORE = 'ignore'; public const FAIL = 'fail';