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

@@ -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
+<?php
+
+namespace App\EventListener;
+
+use Vich\UploaderBundle\Event\ErrorEvent;use Vich\UploaderBundle\Event\Event;
+use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+use Vich\UploaderBundle\Event\Events;
+
+class RemoveErrorEventListener implements EventSubscriberInterface
+{
+    public static function getSubscribedEvents(){
+            return [
+            Events::REMOVE_ERROR => 'onUploadError',
+        ]       
+    }
+    public function onUploadError(ErrorEvent $errorEvent)
+    {
+        $object = $event->getObject();
+        $mapping = $event->getMapping();
+        $exception = $errorEvent->getThrowable();
+        
+        throw $exception;
+    }
+
+}
+```
+
 [Return to the index](../index.md)

src/Event/ErrorEvent.php

@@ -0,0 +1,21 @@
+<?php
+
+namespace Vich\UploaderBundle\Event;
+
+use Vich\UploaderBundle\Mapping\PropertyMapping;
+
+class ErrorEvent extends Event
+{
+    private \Throwable $throwable;
+
+    public function __construct(object $object, PropertyMapping $mapping, \Throwable $throwable)
+    {
+        parent::__construct($object, $mapping);
+        $this->throwable = $throwable;
+    }
+
+    public function getThrowable(): \Throwable
+    {
+        return $this->throwable;
+    }
+}

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';
 }

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));

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) {

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

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

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;
 

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]);

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')

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');