Skip to content

Conversation

@rrigby
Copy link
Contributor

@rrigby rrigby commented Nov 14, 2025

This will require a closer look, but I've been using these changes for a while with success.

Reasoning:
In most situations a problem with a query resulted in picodb's SQLException propagating out making for easy handling.
Here's the source of the exception in picodb for reference:

   // StatementHandler.php   

    public function execute()
    {
        try {
            $this->beforeExecute();

            $pdoStatement = $this->db->getConnection()->prepare($this->sql);
            $this->bindParams($pdoStatement);
            $pdoStatement->execute();

            $this->afterExecute();
            return $pdoStatement;
        } catch (PDOException $e) {
            return $this->handleSqlError($e);
        }
    }

    public function handleSqlError(PDOException $e)
    {
        $this->cleanup();
        $this->db->cancelTransaction();
        $this->db->setLogMessage($e->getMessage());

        throw new SQLException('SQL Error: '.$e->getMessage());
    }

However in some situations, this exception is caught in picomapper and instead false is returned. This makes handling errors more difficult. The most common situation I encountered this, was when I had a bug and was attempting to update an aggregate with new duplicate child records. It would attempt to insert a duplicate child and that resulting exception would be caught and false would be returned. I was confused why my request was succeeding but my record wasn't updating. After diving in I figured out why, and thus this PR 😅
before:

    } catch (\Exception $e) {
        if ($useTransaction) {
            $this->db->cancelTransaction();
        }

        return false;
    }

after:

    } catch (\Exception $exception) {
        if ($useTransaction) {
            $this->db->cancelTransaction();
        }

        throw $exception;
    }

I've also opted to simply remove the returning false. Such as this example:

    if (!parent::insert($base)) {
            // Transaction already cancelled by the statement handler
            return false;
    }

In the above example false is never returned, as the SQLException from picodb is thrown already. So I've simply replaced it with:

parent::insert($base);

I've also escalated to exception any errors in the picomapper library that previously returned false, for example:
before:

    if (!$original = $this->findOne()) {
        return false;
    }

after:

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

Maybe these should use a specific exception class from picomapper, I'm happy to add that if you have thoughts. I'd just kept it simple for now.

This fixes the previous behaviour where most issues resulted in an SQLError exception, however a handful of things were handled and simply returned false, which wasn't clear when using the library and lead to bugs.
$this->getMapping()->update($customer);
}

public function testUpdateDoesNotErrorWhenUpdatingDuplicateChildrenRecords()
Copy link
Contributor Author

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.

@rrigby
Copy link
Contributor Author

rrigby commented Jan 21, 2026

@joshmcrae any chance for a look at this one 🙏

Copy link
Member

@joshmcrae joshmcrae left a comment

Choose a reason for hiding this comment

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

Sorry @rrigby, it's been hard to find time to prioritise this one.

I agree that throwing an exception in every case is ideal, and no matter what we do it should be consistent across error cases.

A dedicated PicoMapper exception is a must since we want to use exceptions. Could you please update your branch with that change?

We'll have to release this as a breaking change, which could be quite a lot of work for projects already using this library. That said, it's probably going to uncover unknown bugs in those codebases as it has for you so that's desirable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants