Always throw exceptions rather than sometimes returning false (#24)

Author: rrigby

src/Mapping.php

@@ -92,7 +92,7 @@ public function count(string $column = '*')
         if ($this->definition->getDeletionTimestamp()) {
             $this->isNull($this->prefixTableNameTo($this->definition->getDeletionTimestamp()));
         }
-  
+
         return parent::count($column);
     }
 
@@ -124,10 +124,7 @@ public function insert(array $data)
             unset($base[$this->definition->getPrimaryKey()[0]]);
         }
 
-        if (!parent::insert($base)) {
-            // Transaction already cancelled by the statement handler
-            return false;
-        }
+        parent::insert($base);
 
         if ($this->definition->isAutoIncrement()) {
             $this->lastId = $this->db->getLastId();
@@ -154,10 +151,7 @@ public function insert(array $data)
             foreach ($items as $item) {
                 $item[$property->getForeignColumn()] = $data[$property->getLocalColumn()];
 
-                if (!$mapping->insert($item)) {
-                    // Transaction already cancelled by the statement handler
-                    return false;
-                }
+                $mapping->insert($item);
             }
         }
 
@@ -177,6 +171,7 @@ public function insert(array $data)
      *
      * @param array $data
      * @return boolean
+     * @throws MappingException
      */
     public function update(array $data = array())
     {
@@ -188,7 +183,7 @@ public function update(array $data = array())
 
         foreach ($primaryKey as $column) {
             if (!array_key_exists($column, $data)) {
-                return false;
+                throw new MappingException('Failed to update record. Missing primary key column: ' . $column);
             }
 
             if (is_null($data[$column])) {
@@ -199,7 +194,7 @@ public function update(array $data = array())
         }
 
         if (!$original = $this->findOne()) {
-            return false;
+            throw new MappingException('Failed to update record. Original not found.');
         }
 
         $useTransaction = !$this->db->getConnection()->inTransaction();
@@ -222,12 +217,12 @@ public function update(array $data = array())
             ]);
 
             return true;
-        } catch (\Exception $e) {
+        } catch (\Exception $exception) {
             if ($useTransaction) {
                 $this->db->cancelTransaction();
             }
 
-            return false;
+            throw $exception;
         }
     }
 
@@ -236,14 +231,15 @@ public function update(array $data = array())
      *
      * @param array $data
      * @return bool
+     * @throws MappingException
      */
     public function save(array $data)
     {
         $primaryKey = $this->definition->getPrimaryKey();
 
         foreach ($primaryKey as $column) {
             if (!array_key_exists($column, $data)) {
-                return false;
+                throw new MappingException('Failed to save record. Missing primary key column: ' . $column);
             }
 
             if (is_null($data[$column])) {
@@ -261,6 +257,7 @@ public function save(array $data)
      * Removes data matching the condition.
      *
      * @return bool
+     * @throws MappingException
      */
     public function remove()
     {
@@ -271,20 +268,30 @@ public function remove()
             $ids = $this->collectPrimary($original, $ids);
         }
 
-        try {
+        $useTransaction = !$this->db->getConnection()->inTransaction();
+
+        if ($useTransaction) {
             $this->db->startTransaction();
+        }
+
+        try {
             $this->delete($ids);
 
-            $this->db->closeTransaction();
+            if ($useTransaction) {
+                $this->db->closeTransaction();
+            }
 
             foreach ($data as $item) {
                 $this->dispatch('removed', $item);
             }
 
             return true;
         } catch (\Exception $exception) {
-            $this->db->cancelTransaction();
-            return false;
+            if ($useTransaction) {
+                $this->db->cancelTransaction();
+            }
+
+            throw $exception;
         }
     }
 
@@ -296,7 +303,7 @@ public function remove()
      * @param array $original
      * @param array $deleteIds
      * @return array
-     * @throws \Exception
+     * @throws MappingException
      */
     private function replace(array $data, array $original, array $deleteIds = [])
     {
@@ -331,9 +338,7 @@ private function replace(array $data, array $original, array $deleteIds = [])
                 $this->definition->getModificationData()
             );
 
-            if (!$query->update($base)) {
-                throw new \Exception('Failed to update record.');
-            }
+            $query->update($base);
         }
 
         foreach ($this->definition->getProperties() as $property) {
@@ -361,9 +366,7 @@ private function replace(array $data, array $original, array $deleteIds = [])
                     $item[$property->getForeignColumn()] = $data[$property->getLocalColumn()];
                 }
 
-                if (!$mapping->insert($item)) {
-                    throw new \Exception('Failed to insert record.');
-                }
+                $mapping->insert($item);
             }
 
             foreach ($delete as $item) {
@@ -380,7 +383,7 @@ private function replace(array $data, array $original, array $deleteIds = [])
                     continue;
                 }
 
-                $originalItem = Collection::first($propertyOriginal, function($original) use ($item, $propertyPrimary) {
+                $originalItem = Collection::first($propertyOriginal, function ($original) use ($item, $propertyPrimary) {
                     foreach ($propertyPrimary as $column) {
                         if ($original[$column] != $item[$column]) {
                             return false;
@@ -402,14 +405,14 @@ private function replace(array $data, array $original, array $deleteIds = [])
      * mapping tables to primary key values.
      *
      * @param array $ids
-     * @throws \Exception
+     * @throws MappingException
      */
     private function delete($ids = [])
     {
         foreach ($ids as $table => $deleteColumns) {
             foreach ($deleteColumns as $deletion => $deletionContext) {
                 // Arrange values into groups based on all but the last key
-                $grouped = Collection::group($deletionContext['keys'], function(array $keys) {
+                $grouped = Collection::group($deletionContext['keys'], function (array $keys) {
                     array_pop($keys);
                     return implode(':', $keys);
                 });
@@ -465,7 +468,7 @@ private function delete($ids = [])
                     }
 
                     if (!$result) {
-                        throw new \Exception('Failed to delete records.');
+                        throw new MappingException('Failed to delete records.');
                     }
                 }
             }
@@ -549,7 +552,7 @@ private function map(array $data)
             $results = $mapping
                 ->findAll();
 
-            $properties[$property->getName()] = Collection::group($results, function($result) use ($groupColumn) {
+            $properties[$property->getName()] = Collection::group($results, function ($result) use ($groupColumn) {
                 return $result[$groupColumn];
             });
         }
@@ -657,7 +660,7 @@ private function dispatch(string $event, array $data, $args = [])
     private function requiresPrefix(string $column): bool
     {
         $pattern = '/^[a-zA-Z_][a-zA-Z0-9_]*$/';
-        return (bool) preg_match($pattern, $column);
+        return (bool)preg_match($pattern, $column);
     }
 
     /**
@@ -674,7 +677,8 @@ private function requiresPrefix(string $column): bool
      *
      * @return string | array
      */
-    private function prefixTableNameTo($input) {
+    private function prefixTableNameTo($input)
+    {
         $table = $this->definition->getTable();
 
         if (is_string($input)) {

src/MappingException.php

@@ -0,0 +1,7 @@
+<?php
+
+namespace PicoMapper;
+
+class MappingException extends \Exception
+{
+}