Merge pull request #1450 from tarjei/I-1449-exceptionhandling

If the underlying storage fails, emit an event

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