From add4e187fc847f8ba868ecea9f93ac2e933e9d97 Mon Sep 17 00:00:00 2001 From: Tarjei Huse Date: Fri, 10 May 2024 09:01:02 +0200 Subject: [PATCH] If the underlying storage fails, emit an event If the underlying storage fails, we emit an event that can be handled as the user wants. The behaviour is to throw exceptions on write but not on remove as this seems to be the semantics as they are in the library prior to this change. --- docs/events/events.md | 56 ++++++++++++++++--- src/Event/ErrorEvent.php | 21 +++++++ src/Event/Events.php | 10 ++++ src/Handler/UploadHandler.php | 16 ++++-- src/Storage/FileSystemStorage.php | 14 ++++- src/Storage/FlysystemStorage.php | 8 +-- src/Storage/GaufretteStorage.php | 7 +-- src/Storage/StorageInterface.php | 2 + tests/Handler/UploadHandlerTest.php | 74 +++++++++++++++++++++++++ tests/Storage/FileSystemStorageTest.php | 2 + tests/Storage/GaufretteStorageTest.php | 2 + 11 files changed, 185 insertions(+), 27 deletions(-) create mode 100644 src/Event/ErrorEvent.php diff --git a/docs/events/events.md b/docs/events/events.md index 3d7fbc1e..711195d1 100644 --- a/docs/events/events.md +++ b/docs/events/events.md @@ -5,14 +5,16 @@ the flux of other code, in this case into the flux of VichUploaderBundle. The following is a list of events you can listen to: -| Event name | Event constant | Trigger point| -|------------|----------------|--------------| -|`vich_uploader.pre_upload`|`Events::PRE_UPLOAD`|before a file upload is handled| -|`vich_uploader.post_upload`|`Events::POST_UPLOAD`|right after a file upload is handled| -|`vich_uploader.pre_inject`|`Events::PRE_INJECT`|before a file is injected into an entity| -|`vich_uploader.post_inject`|`Events::POST_INJECT`|after a file is injected into an entity| -|`vich_uploader.pre_remove`|`Events::PRE_REMOVE`|before a file is removed| -|`vich_uploader.post_remove`|`Events::POST_REMOVE`|after a file is removed| +| Event name | Event constant | Trigger point | +|------------------------------|------------------------|------------------------------------------| +| `vich_uploader.pre_upload` | `Events::PRE_UPLOAD` | before a file upload is handled | +| `vich_uploader.post_upload` | `Events::POST_UPLOAD` | right after a file upload is handled | +| `vich_uploader.pre_inject` | `Events::PRE_INJECT` | before a file is injected into an entity | +| `vich_uploader.post_inject` | `Events::POST_INJECT` | after a file is injected into an entity | +| `vich_uploader.pre_remove` | `Events::PRE_REMOVE` | before a file is removed | +| `vich_uploader.post_remove` | `Events::POST_REMOVE` | after a file is removed | +| `vich_uploader.upload_error` | `Events::UPLOAD_ERROR` | If file upload failed. | +| `vich_uploader.remove_error` | `Events::REMOVE_ERROR` | If file remove failed. | The `vich_uploader.pre_remove` event is cancelable, that means that the actual remove request will not take place, and you have to take action. @@ -51,4 +53,42 @@ services: - { name: kernel.event_listener, event: vich_uploader.pre_upload } ``` +## Error events + +The `UPLOAD_ERROR` event happens when writing a file fails, and before an exception is thrown. + +If an error occurs while removing a file, then the `REMOVE_ERROR` event gets fired. Failures in removing +files do not trigger any exceptions unless you choose to throw an error in the event handler for the `REMOVE_ERROR` +event. + +You can use the event to throw an error when the error occurs: + +```php + 'onUploadError', + ] + } + public function onUploadError(ErrorEvent $errorEvent) + { + $object = $event->getObject(); + $mapping = $event->getMapping(); + $exception = $errorEvent->getThrowable(); + + throw $exception; + } + +} +``` + [Return to the index](../index.md) diff --git a/src/Event/ErrorEvent.php b/src/Event/ErrorEvent.php new file mode 100644 index 00000000..a8473588 --- /dev/null +++ b/src/Event/ErrorEvent.php @@ -0,0 +1,21 @@ +throwable = $throwable; + } + + public function getThrowable(): \Throwable + { + return $this->throwable; + } +} diff --git a/src/Event/Events.php b/src/Event/Events.php index 4bba080b..09c82878 100644 --- a/src/Event/Events.php +++ b/src/Event/Events.php @@ -54,4 +54,14 @@ final class Events * @Event("Vich\UploaderBundle\Event\Event") */ public const POST_REMOVE = 'vich_uploader.post_remove'; + + /** + * Triggered if writing to storage fails. + */ + public const UPLOAD_ERROR = 'vich_uploader.upload_error'; + + /** + * Triggered if removing the file from storage fails. + */ + public const REMOVE_ERROR = 'vich_uploader.remove_error'; } diff --git a/src/Handler/UploadHandler.php b/src/Handler/UploadHandler.php index d0563f2b..01c47cea 100644 --- a/src/Handler/UploadHandler.php +++ b/src/Handler/UploadHandler.php @@ -4,6 +4,7 @@ use Symfony\Component\HttpFoundation\File\UploadedFile; use Symfony\Contracts\EventDispatcher\EventDispatcherInterface; +use Vich\UploaderBundle\Event\ErrorEvent; use Vich\UploaderBundle\Event\Event; use Vich\UploaderBundle\Event\Events; use Vich\UploaderBundle\FileAbstraction\ReplacingFile; @@ -46,8 +47,12 @@ public function upload(object $obj, string $fieldName): void } $this->dispatch(Events::PRE_UPLOAD, new Event($obj, $mapping)); - - $this->storage->upload($obj, $mapping); + try { + $this->storage->upload($obj, $mapping); + } catch (\Throwable $exception) { + $this->dispatch(Events::UPLOAD_ERROR, new ErrorEvent($obj, $mapping, $exception)); + throw $exception; + } $this->injector->injectFile($obj, $mapping); $this->dispatch(Events::POST_UPLOAD, new Event($obj, $mapping)); @@ -93,8 +98,11 @@ public function remove(object $obj, string $fieldName): void if ($preEvent->isCanceled()) { return; } - - $this->storage->remove($obj, $mapping); + try { + $this->storage->remove($obj, $mapping); + } catch (\Throwable $exception) { + $this->dispatch(Events::REMOVE_ERROR, new ErrorEvent($obj, $mapping, $exception)); + } $mapping->erase($obj); $this->dispatch(Events::POST_REMOVE, new Event($obj, $mapping)); diff --git a/src/Storage/FileSystemStorage.php b/src/Storage/FileSystemStorage.php index 228373b9..79d8d89f 100644 --- a/src/Storage/FileSystemStorage.php +++ b/src/Storage/FileSystemStorage.php @@ -41,11 +41,19 @@ protected function doRemove(PropertyMapping $mapping, ?string $dir, string $name { $file = $this->doResolvePath($mapping, $dir, $name); - return \file_exists($file) && \unlink($file); + if (!\file_exists($file) || !unlink($file)) { + throw new \Exception('Cannot remove file '.$file); + } + + return true; } - protected function doResolvePath(PropertyMapping $mapping, ?string $dir, string $name, ?bool $relative = false): string - { + protected function doResolvePath( + PropertyMapping $mapping, + ?string $dir, + string $name, + ?bool $relative = false + ): string { $path = !empty($dir) ? $dir.\DIRECTORY_SEPARATOR.$name : $name; if ($relative) { diff --git a/src/Storage/FlysystemStorage.php b/src/Storage/FlysystemStorage.php index 6316a3b9..3d894afc 100644 --- a/src/Storage/FlysystemStorage.php +++ b/src/Storage/FlysystemStorage.php @@ -56,13 +56,9 @@ protected function doRemove(PropertyMapping $mapping, ?string $dir, string $name $fs = $this->getFilesystem($mapping); $path = !empty($dir) ? $dir.'/'.$name : $name; - try { - $fs->delete($path); + $fs->delete($path); - return true; - } catch (FilesystemException) { - return false; - } + return true; } protected function doResolvePath(PropertyMapping $mapping, ?string $dir, string $name, ?bool $relative = false): string diff --git a/src/Storage/GaufretteStorage.php b/src/Storage/GaufretteStorage.php index b4868b98..849b1da6 100644 --- a/src/Storage/GaufretteStorage.php +++ b/src/Storage/GaufretteStorage.php @@ -3,7 +3,6 @@ namespace Vich\UploaderBundle\Storage; use Gaufrette\Adapter\MetadataSupporter; -use Gaufrette\Exception\FileNotFound; use Gaufrette\FilesystemInterface; use Gaufrette\FilesystemMapInterface; use Symfony\Component\HttpFoundation\File\File; @@ -46,11 +45,7 @@ protected function doRemove(PropertyMapping $mapping, ?string $dir, string $name $filesystem = $this->getFilesystem($mapping); $path = !empty($dir) ? $dir.'/'.$name : $name; - try { - return $filesystem->delete($path); - } catch (FileNotFound) { - return false; - } + return $filesystem->delete($path); } protected function doResolvePath(PropertyMapping $mapping, ?string $dir, string $name, ?bool $relative = false): string diff --git a/src/Storage/StorageInterface.php b/src/Storage/StorageInterface.php index 6489b31a..d605d961 100644 --- a/src/Storage/StorageInterface.php +++ b/src/Storage/StorageInterface.php @@ -23,6 +23,8 @@ public function upload(object $obj, PropertyMapping $mapping): void; * * @param object $obj The object * @param PropertyMapping $mapping The mapping representing the field to remove + * + * @throw \Exception Throws an exception */ public function remove(object $obj, PropertyMapping $mapping): ?bool; diff --git a/tests/Handler/UploadHandlerTest.php b/tests/Handler/UploadHandlerTest.php index c8ad7ee7..ca32b6a4 100644 --- a/tests/Handler/UploadHandlerTest.php +++ b/tests/Handler/UploadHandlerTest.php @@ -4,6 +4,7 @@ use PHPUnit\Framework\MockObject\MockObject; use Symfony\Component\EventDispatcher\EventDispatcherInterface; +use Symfony\Component\HttpFoundation\File\Exception\CannotWriteFileException; use Vich\TestBundle\Entity\Article; use Vich\UploaderBundle\Event\Event; use Vich\UploaderBundle\Event\Events; @@ -191,6 +192,79 @@ public function testRemove(): void $this->handler->remove($this->object, self::FILE_FIELD); } + public function testRemoveFailsInStorageDriverEmitsEvent(): void + { + $this->expectEvents([Events::PRE_REMOVE, Events::REMOVE_ERROR, Events::POST_REMOVE]); + + $this->mapping + ->expects(self::once()) + ->method('getFileName') + ->with($this->object) + ->willReturn('something not null'); + + $this->mapping + ->expects(self::once()) + ->method('erase') + ->with($this->object); + + $this->storage + ->expects(self::once()) + ->method('remove') + ->with($this->object, $this->mapping) + ->willThrowException(new \RuntimeException('Test exception')) + ; + + $this->handler->remove($this->object, self::FILE_FIELD); + } + + public function testUploadFailsEmitsEventAndException(): void + { + $this->expectException(\RuntimeException::class); + + $this->expectEvents([Events::PRE_UPLOAD, Events::UPLOAD_ERROR]); + + $this->mapping + ->expects(self::once()) + ->method('getFile') + ->with($this->object) + ->willReturn($this->getUploadedFileMock()); + + $this->storage + ->expects(self::once()) + ->method('upload') + ->with($this->object, $this->mapping) + ->willThrowException(new \RuntimeException('This is a test')); + + $this->injector + ->expects(self::never()) + ->method('injectFile') + ->with($this->object, $this->mapping); + + $this->handler->upload($this->object, self::FILE_FIELD); + } + + public function testremoveFailsWithCannotWriteException(): void + { + $this->mapping + ->expects(self::once()) + ->method('getFileName') + ->with($this->object) + ->willReturn('something not null'); + + $this->mapping + ->expects(self::once()) + ->method('erase') + ->with($this->object); + + $this->storage + ->expects(self::once()) + ->method('remove') + ->with($this->object, $this->mapping) + ->willThrowException(new CannotWriteFileException('this is a test')); + + $this->handler->remove($this->object, self::FILE_FIELD); + } + public function testRemoveIfEventIsCanceled(): void { $this->expectEvents([Events::PRE_REMOVE]); diff --git a/tests/Storage/FileSystemStorageTest.php b/tests/Storage/FileSystemStorageTest.php index 9856c2ac..33465593 100644 --- a/tests/Storage/FileSystemStorageTest.php +++ b/tests/Storage/FileSystemStorageTest.php @@ -42,6 +42,8 @@ public function testRemoveSkipsEmptyFilenameProperties(?string $propertyValue): */ public function testRemoveSkipsNonExistingFile(): void { + $this->expectException(\Exception::class); + $this->mapping ->expects(self::once()) ->method('getUploadDir') diff --git a/tests/Storage/GaufretteStorageTest.php b/tests/Storage/GaufretteStorageTest.php index 8d8bfe21..3b34a19a 100644 --- a/tests/Storage/GaufretteStorageTest.php +++ b/tests/Storage/GaufretteStorageTest.php @@ -172,6 +172,8 @@ public function testThatRemoveMethodDoesDeleteFile(): void */ public function testRemoveNotFoundFile(): void { + // the exception is caught in the UploadHandler. + $this->expectException(FileNotFound::class); $this->mapping ->method('getUploadDestination') ->willReturn('filesystemKey');