Skip to content

Always throw exceptions rather than sometimes returning false #24

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rrigby wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Always throw exceptions rather than sometimes returning false #24

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
163039c
Throw exceptions instead of returning false.
rrigby Nov 14, 2025
a6c3dc6
Add tests confirming change and some existing quirks
rrigby Nov 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 33 additions & 29 deletions src/Mapping.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ public function count(string $column = '*')
if ($this->definition->getDeletionTimestamp()) {
$this->isNull($this->prefixTableNameTo($this->definition->getDeletionTimestamp()));
}

return parent::count($column);
}

Expand Down Expand Up @@ -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();
Expand All @@ -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);
}
}

Expand All @@ -177,6 +171,7 @@ public function insert(array $data)
*
* @param array $data
* @return boolean
* @throws \Exception
*/
public function update(array $data = array())
{
Expand All @@ -188,7 +183,7 @@ public function update(array $data = array())

foreach ($primaryKey as $column) {
if (!array_key_exists($column, $data)) {
return false;
throw new \Exception('Failed to update record. Missing primary key column: ' . $column);
}

if (is_null($data[$column])) {
Expand All @@ -199,7 +194,7 @@ public function update(array $data = array())
}

if (!$original = $this->findOne()) {
return false;
throw new \Exception('Failed to update record. Original not found.');
}

$useTransaction = !$this->db->getConnection()->inTransaction();
Expand All @@ -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;
}
}

Expand All @@ -236,14 +231,15 @@ public function update(array $data = array())
*
* @param array $data
* @return bool
* @throws \Exception
*/
public function save(array $data)
{
$primaryKey = $this->definition->getPrimaryKey();

foreach ($primaryKey as $column) {
if (!array_key_exists($column, $data)) {
return false;
throw new \Exception('Failed to save record. Missing primary key column: ' . $column);
}

if (is_null($data[$column])) {
Expand All @@ -261,6 +257,7 @@ public function save(array $data)
* Removes data matching the condition.
*
* @return bool
* @throws \Exception
*/
public function remove()
{
Expand All @@ -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;
}
}

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand All @@ -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;
Expand Down Expand Up @@ -409,7 +412,7 @@ 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);
});
Expand Down Expand Up @@ -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];
});
}
Expand Down Expand Up @@ -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);
}

/**
Expand All @@ -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)) {
Expand Down
134 changes: 131 additions & 3 deletions tests/MappingTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ public function testRemove()
);
}

public function testReadOnlyInsert()
public function testReadOnlyInsert()
{
$customer = [
'id' => 10,
Expand Down Expand Up @@ -288,22 +288,150 @@ public function testInsertRollback()
} catch (SQLException $exception) {
}

$this->assertFalse($this->db->getConnection()->inTransaction());

$inserted = $this->getMapping()->eq('id', 3)->findOne();
$this->assertNull($inserted);
}

public function testInsertDuplicateThrowsException()
{
$this->expectException(SQLException::class);
$this->expectExceptionMessage('UNIQUE constraint failed');

$customer = [
'id' => 1,
'name' => 'Dave Matthews',
'orders' => []
];

$this->getMapping()->insert($customer);
}

public function testInsertDuplicateChildrenThrowsException()
{
$this->expectException(SQLException::class);
$this->expectExceptionMessage('UNIQUE constraint failed');

$customer = [
'id' => 3,
'name' => 'Dave Matthews',
'orders' => [
[
'id' => 4,
'date_created' => '2018-02-01',
'discount' => [
'description' => 'Dessert Discount',
'amount' => 20
],
'items' => [
[
'id' => 7,
'description' => 'Ice Cream',
'amount' => 400
],
[
'id' => 7,
'description' => 'Cookies',
'amount' => '230'
]
]
]
]
];

$this->getMapping()->insert($customer);
}

public function testUpdateRollback()
{
$customer = $this->getMapping()->eq('id', 1)->findOne();
$original = $customer;

$customer['orders'][0]['items'][] = ['id' => '3'];
$this->getMapping()->update($customer);
$customer['orders'][0]['items'] = array_merge(
$customer['orders'][0]['items'],
[
[
'id' => 7,
'description' => 'Ice Cream',
'amount' => 400
],
[
'id' => 7,
'description' => 'Cookies',
'amount' => '230'
]
]
);

try {
$this->getMapping()->update($customer);
} catch (SQLException $exception) {
}

$updated = $this->getMapping()->eq('id', 1)->findOne();
$this->assertEquals($original, $updated);
}

public function testUpdateThrowsExceptionWhenInsertingDuplicateChildrenRecords()
{
$this->expectException(SQLException::class);
$this->expectExceptionMessage('UNIQUE constraint failed');

$customer = $this->getMapping()->eq('id', 1)->findOne();

$customer['orders'][0]['items'] = array_merge(
$customer['orders'][0]['items'],
[
[
'id' => 7,
'description' => 'Ice Cream',
'amount' => 400
],
[
'id' => 7,
'description' => 'Cookies',
'amount' => '230'
]
]
);

$this->getMapping()->update($customer);
}

public function testUpdateDoesNotErrorWhenUpdatingDuplicateChildrenRecords()
Copy link
Contributor Author

@rrigby rrigby Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test simply confirms existing behaviour, that it's fine to include duplicate items in the update array, as just the last entry for a given id will be applied to the item in the db.

{
$customer = $this->getMapping()->eq('id', 1)->findOne();
$original = $customer;

// You can provide the same item multiple times to update.
$customer['orders'][0]['items'] = array_merge(
$customer['orders'][0]['items'],
[
[
'id' => 3,
'description' => 'Ice Cream',
'amount' => 400
],
[
'id' => 3,
'description' => 'Chocolate Bar',
'amount' => 600
]
]
);

$this->getMapping()->update($customer);

$updated = $this->getMapping()->eq('id', 1)->findOne();
$this->assertNotEquals($original, $updated);

// The last array entry for that item will be the one reflected in the database.
$this->assertEquals($updated['orders'][0]['items'][2]['id'], 3);
$this->assertEquals($updated['orders'][0]['items'][2]['description'], 'Chocolate Bar');
$this->assertEquals($updated['orders'][0]['items'][2]['amount'], 600);
}

public function testPrefixAndRemoveTableName()
{
$method = new \ReflectionMethod('PicoMapper\Mapping', 'prefixTableNameTo');
Expand Down