Fixes the consequences when saving empty file data.

When the file field is allowed to be empty and the storage entity is saved without a file present it has still saved successfully even in the case no file data was present. It also created an empty file in the storage backend (using the local adapter when I discovered this).

This commit should resolve these issues and improve the storage event system a little more.

src/Model/Table/FileStorageTable.php

@@ -88,14 +88,13 @@ public function beforeMarshal(Event $event, ArrayAccess $data) {
  * @return boolean true on success
  */
 	public function beforeSave(Event $event, EntityInterface $entity, $options) {
-		$Event = $this->dispatchEvent('FileStorage.beforeSave', array(
+		$storageEvent = $this->dispatchEvent('FileStorage.beforeSave', array(
 			'record' => $entity,
 			'storage' => $this->storageAdapter($entity['adapter'])
 		));
-		if ($Event->isStopped()) {
-			return false;
+		if ($storageEvent->isStopped()) {
+			$event->stopPropagation();
 		}
-		return true;
 	}
 
 /**
@@ -135,7 +134,7 @@ public function getFileInfoFromUpload(&$upload, $field = 'file') {
  * @param \Cake\Event\Event $event
  * @param \Cake\Datasource\EntityInterface $entity
  * @param array $options
- * @return boolean
+ * @return void
  */
 	public function afterSave(Event $event, EntityInterface $entity, $options) {
 		$this->dispatchEvent('FileStorage.afterSave', [
@@ -144,15 +143,14 @@ public function afterSave(Event $event, EntityInterface $entity, $options) {
 			'storage' => $this->storageAdapter($entity['adapter'])
 		]);
 		$this->deleteOldFileOnSave($entity);
-		return true;
 	}
 
 /**
  * Get a copy of the actual record before we delete it to have it present in afterDelete
  *
  * @param \Cake\Event\Event $event
  * @param \Cake\Datasource\EntityInterface $entity
- * @return boolean
+ * @return void
  */
 	public function beforeDelete(Event $event, EntityInterface $entity) {
 		$this->record = $this->find()
@@ -163,10 +161,8 @@ public function beforeDelete(Event $event, EntityInterface $entity) {
 			->first();
 
 		if (empty($this->record)) {
-			return false;
+			$event->stopPropagation();
 		}
-
-		return true;
 	}
 
 /**
@@ -189,7 +185,7 @@ public function afterDelete(Event $event, EntityInterface $entity, $options) {
  * Deletes an old file to replace it with the new one if an old id was passed.
  *
  * Thought to be called in Model::afterSave() but can be used from any other
- * place as well like Model::beforeSave() as long as the field data is present.
+ * place as well like Table::beforeSave() as long as the field data is present.
  *
  * The old id has to be the UUID of the file_storage record that should be deleted.
  *

src/Model/Table/ImageStorageTable.php

@@ -50,19 +50,18 @@ public function initialize(array $config) {
  * @param \Cake\Event\Event $event
  * @param \Cake\Datasource\EntityInterface $entity
  * @param array $options
- * @return boolean true on success
+ * @return void
  */
 	public function beforeSave(Event $event, EntityInterface $entity, $options) {
-		if (!parent::beforeSave($event, $entity, $options)) {
-			return false;
+		if ($event->isStopped()) {
+			return;
 		}
 		$imageEvent = $this->dispatchEvent('ImageStorage.beforeSave', [
 			'record' => $entity
 		]);
 		if ($imageEvent->isStopped()) {
-			return false;
-}
-		return true;
+			$event->stopPropagation();
+		}
 	}
 
 /**
@@ -73,7 +72,7 @@ public function beforeSave(Event $event, EntityInterface $entity, $options) {
  * @param \Cake\Event\Event $event
  * @param \Cake\Datasource\EntityInterface $entity
  * @param array $options
- * @return boolean
+ * @return void
  */
 	public function afterSave(Event $event, EntityInterface $entity, $options) {
 		if ($entity->isNew()) {
@@ -83,19 +82,19 @@ public function afterSave(Event $event, EntityInterface $entity, $options) {
 			]);
 			$this->deleteOldFileOnSave($entity);
 		}
-		return true;
 	}
 
 /**
  * Get a copy of the actual record before we delete it to have it present in afterDelete
  *
  * @param \Cake\Event\Event $event
  * @param \Cake\Datasource\EntityInterface $entity
- * @return boolean
+ * @return void
  */
 	public function beforeDelete(Event $event, EntityInterface $entity) {
-		if (!parent::beforeDelete($event, $entity)) {
-			return false;
+		parent::beforeDelete($event, $entity);
+		if ($event->isStopped()) {
+			return;
 		}
 
 		$imageEvent = $this->dispatchEvent('ImageStorage.beforeDelete', [
@@ -104,10 +103,8 @@ public function beforeDelete(Event $event, EntityInterface $entity) {
 		]);
 
 		if ($imageEvent->isStopped()) {
-			return false;
+			$event->stopPropagation();
 		}
-
-		return true;
 	}
 
 /**

src/Storage/Listener/AbstractListener.php

@@ -7,6 +7,7 @@
 namespace Burzum\FileStorage\Storage\Listener;
 
 use Burzum\FileStorage\Storage\PathBuilder\PathBuilderTrait;
+use Burzum\FileStorage\Storage\StorageException;
 use Burzum\FileStorage\Storage\StorageTrait;
 use Burzum\FileStorage\Storage\StorageUtils;
 use Cake\Core\InstanceConfigTrait;
@@ -114,9 +115,16 @@ protected function _mergeListenerVars() {
  */
 	public function initialize($config) {}
 
+/**
+ * Implemented events
+ *
+ * @return array
+ */
 	public function implementedEvents() {
 		return [
-			'FileStorage.path' => 'getPath'
+			'FileStorage.path' => 'getPath',
+			'FileStorage.beforeSave' => 'beforeSaveCheckFileField',
+			'ImageStorage.beforeSave' => 'beforeSaveCheckFileField',
 		];
 	}
 
@@ -275,13 +283,70 @@ public function createTmpFile($folder = null, $checkAndCreatePath = true) {
 	}
 
 /**
- * @param \Cake\Datasource\EntityInterface
+ * Gets a type of a path.
+ *
+ * @param \Cake\Event\Event
  * @return string
  */
 	public function getPath(Event $event) {
 		return $this->pathBuilder()->{$event->data['method']}($event->subject(), $event->data);
 	}
 
+/**
+ *
+ */
+	public function beforeSaveCheckFileField(Event $event, EntityInterface $entity, $options) {
+		if ($this->_checkEvent($event) && !$this->_checkFileField($entity)) {
+			$event->stopPropagation();
+		}
+	}
+
+/**
+ * Stores the file in the configured storage backend.
+ *
+ * @param \Cake\Event\Event $event
+ * @throws \Burzum\Filestorage\Storage\StorageException
+ * @return boolean
+ */
+	protected function _storeFile(Event $event) {
+		try {
+			$fileField = $this->config('fileField');
+			$entity = $event->data['record'];
+			$Storage = $this->storageAdapter($entity['adapter']);
+			$Storage->write($entity['path'], file_get_contents($entity[$fileField]['tmp_name']), true);
+			$event->result = $event->data['table']->save($entity, array(
+				'checkRules' => false
+			));
+			return true;
+		} catch (\Exception $e) {
+			$this->log($e->getMessage(), LogLevel::ERROR, ['scope' => ['storage']]);
+			throw new StorageException($e->getMessage());
+		}
+	}
+
+/**
+ * Avoid saving of empty files at any chance.
+ *
+ * Use this check BEFORE saving a storage entity!
+ *
+ * We don't want to save "empty files", dead records with no real file. If this
+ * is not checked the system will still store an empty file in the storage
+ * backend and a record is created any way.
+ *
+ * @param \Cake\Datasource\EntityInterface
+ * @return boolean
+ */
+	protected function _checkFileField(EntityInterface $entity) {
+		$fileField = $this->config('fileField');
+		if (!$entity->has($fileField)) {
+			return false;
+		}
+		if (!is_array($entity->{$fileField}) || (isset($entity->{$fileField}['error']) && $entity->{$fileField}['error'] === UPLOAD_ERR_NO_FILE)) {
+			return false;
+		}
+		return true;
+	}
+
 /**
  * Constructs a path builder instance.
  *

src/Storage/Listener/LocalListener.php

@@ -57,8 +57,10 @@ class LocalListener extends AbstractListener {
  */
 	public function implementedEvents() {
 		return array_merge(parent::implementedEvents(), [
+			'FileStorage.beforeSave' => 'beforeSaveCheckFileField',
 			'FileStorage.afterSave' => 'afterSave',
 			'FileStorage.afterDelete' => 'afterDelete',
+			'ImageStorage.beforeSave' => 'beforeSaveCheckFileField',
 			'ImageStorage.afterSave' => 'afterSave',
 			'ImageStorage.afterDelete' => 'afterDelete',
 			'ImageVersion.removeVersion' => 'removeImageVersion',
@@ -160,29 +162,6 @@ public function imagePath(Event $event) {
 		$event->stopPropagation();
 	}
 
-/**
- * Stores the file in the configured storage backend.
- *
- * @param \Cake\Event\Event $event
- * @throws \Burzum\Filestorage\Storage\StorageException
- * @return boolean
- */
-	protected function _storeFile(Event $event) {
-		try {
-			$fileField = $this->config('fileField');
-			$entity = $event->data['record'];
-			$Storage = $this->storageAdapter($entity['adapter']);
-			$Storage->write($entity['path'], file_get_contents($entity[$fileField]['tmp_name']), true);
-			$event->result = $event->data['table']->save($entity, array(
-				'checkRules' => false
-			));
-			return true;
-		} catch (\Exception $e) {
-			$this->log($e->getMessage(), LogLevel::ERROR, ['scope' => ['storage']]);
-			throw new StorageException($e->getMessage());
-		}
-	}
-
 /**
  * Removes a specific image version.
  *

src/View/Helper/ImageHelper.php

@@ -74,15 +74,16 @@ public function imageUrl($image, $version = null, $options = []) {
 		];
 
 		$event1 = new Event('ImageVersion.getVersions', $this, $eventOptions);
-		$event2 = new Event('FileStorage.ImageHelper.imagePath', $this, $eventOptions);
-
 		EventManager::instance()->dispatch($event1);
-		EventManager::instance()->dispatch($event2);
 
 		if ($event1->isStopped()) {
 			return $this->normalizePath($event1->data['path']);
-		} elseif ($event2->isStopped()) {
-			return $this->normalizePath($event2->data['path']);
+		} else {
+			$event2 = new Event('FileStorage.ImageHelper.imagePath', $this, $eventOptions);
+			EventManager::instance()->dispatch($event2);
+			if ($event2->isStopped()) {
+				return $this->normalizePath($event2->data['path']);
+			}
 		}
 		return false;
 	}

tests/TestCase/Storage/Listener/LocalListenerTest.php

@@ -48,6 +48,8 @@ public function setUp() {
 	public function testimplementedEvents() {
 		$expected = [
 			'FileStorage.path' => 'getPath',
+			'FileStorage.beforeSave' => 'beforeSaveCheckFileField',
+			'ImageStorage.beforeSave' => 'beforeSaveCheckFileField',
 			'FileStorage.afterSave' => 'afterSave',
 			'FileStorage.afterDelete' => 'afterDelete',
 			'ImageStorage.afterSave' => 'afterSave',