4

I have some queries that have run perfectly up until now, but I wanted to wrap them in a transaction to decrease the likelihood of data corruption. After running the code, everything APPEARS to work (i.e. no exception is thrown), but the query is never committed to the database. I've looked at other questions on S.O. but I haven't found any that apply to my case.

Here's my code:

    db::$pdo->beginTransaction();  // accesses PDO object method

    try {

        $sql = "INSERT INTO expenses SET date=:date, amount=:amount, accountId=:account; ";

        $sql .= "UPDATE accounts SET balance = balance - :amount WHERE id = :account";


        $s = db::$pdo->prepare($sql);
        $s->bindValue(':date', $date);
        $s->bindValue(':amount', $amount);
        $s->bindValue(':account', $account);

        $s->execute();

        db::$pdo->commit();
        echo 'success';
    }
    catch (PDOException $e) {
        db::$pdo->rollback();
        echo '<p>Failed: ' . $e->getMessage() . '</p>';
    }

When I run the code, it outputs the success message, but like I said, nothing gets committed to the database.

Since it's relevant, I should also note that my PDO error mode is set to ERRMODE_EXCEPTION, so I don't think that's what's causing the problem. I'm running a MySQL database (InnoDB)

Any thoughts?

manwill
  • 457
  • 4
  • 11

4 Answers4

1

I am not completely sure how running multiple queries in a single query statement works (if it works) because I typically do transactions by running each query separately in a prepare/execute statement. The transaction will still queue up all the changes until commit/rollback as expected. According to this SO answer it appears to work, however the example is not binding values and so that might be another issue.

I would suggest just splitting up the queries into multiple prepare/bind/execute statements and it should work as expected.

Community
  • 1
  • 1
Jonathan Kuhn
  • 15,279
  • 3
  • 32
  • 43
  • This did the trick. Running multiple queries in a single statement works fine unless you wrap them in a transaction, in which case the transaction doesn't get committed for some reason. Splitting them into separate prepare/execute statements works perfectly. – manwill Dec 31 '14 at 17:56
  • 1
    Running multiple queries in a single statement only works "fine" if you are using emulated prepared statements instead of proper prepared statements. You should really [disable this](http://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php). – PeeHaa Dec 31 '14 at 18:05
  • @PeeHaa I saw this in the linked answer. According to that though it is on by default. OP isn't disabling them (at least not here) and so I would assume that their code should work, as in the second example in the linked answer. – Jonathan Kuhn Dec 31 '14 at 18:07
  • My point is that OP should change it, because the default is bad. Using emulated prepared statements opens the door for possibly security vulnerabilities. – PeeHaa Dec 31 '14 at 18:08
  • @PeeHaa Agreed, but I was just wondering if/why it wasn't working. But I guess that point is moot if you consider that it is bad and shouldn't be used like that. – Jonathan Kuhn Dec 31 '14 at 18:09
  • The reason it works is because when using emulated prepared statements the statement is actually handled / parsed by PHP instead of the SQL server. – PeeHaa Dec 31 '14 at 18:10
-1

The success message will allways be displayed because it's just there to echo.. if you want to show succes only after the execute you should use if statement.. for the not committing part it could be many things.. are you sure all values $date, $amount, $account are being supplied ? you can also try starting with the simple insert and if that works do the update..

$sql = 'INSERT INTO expenses (date, amount, account)
                    VALUES(:date, :amount, :account)';

    $stmt = $this->pdo->prepare($sql);

    $stmt->bindValue(':date', $);
    $stmt->bindValue(':amount', $amount);
    $stmt->bindValue(':account', $account);

        if($stmt->execute()) {
            db::$pdo->commit();
            echo 'success';
        }    

not sure about the db::$pdo->commit(); part because I use some kind of OOP way, but hope this might help.

  • 2
    An insert and then update would lead to data corruption if the insert succeeded but the update failed. That's the whole purpose of a transaction – manwill Dec 31 '14 at 00:20
-1

I think I had similar problem in one of the projects, please add another catch block just after the one you have so the code will look like:

catch (PDOException $e) {
        db::$pdo->rollback();
        echo '<p>Failed: ' . $e->getMessage() . '</p>';
    }
catch (Exception $exc) {
         db::$pdo->rollback();
         echo '<p>Failed: ' . $exc->getMessage() . '</p>';
}

It will allow you to catch Exceptions of class other than PDOException, so you could ensure more.

UPDATE:

Please consider splitting two queries into 2 PDO statements llike this:

$sql = "INSERT INTO expenses SET date=:date, amount=:amount, accountId=:account; ";

$s = db::$pdo->prepare($sql);
$s->bindValue(':date', $date);
$s->bindValue(':amount', $amount);
$s->bindValue(':account', $account);

$s->execute();

$sql2 = "UPDATE accounts SET balance = balance - :amount WHERE id = :account;";

$s2 = db::$pdo->prepare($sql2);
$s2->bindValue(':amount', $amount);
$s2->bindValue(':account', $account);

$s2->execute();

I use this approach in my entire project with success so I hope it might help.

  • 1
    If there was an exception other than PDO, it should still throw an error (and not echo out the 'success' message). I tried it though, and still no luck. – manwill Dec 31 '14 at 00:23
-2

Don't use db::$pdo->commit();

Use

$sql = "INSERT INTO expenses SET date=:date, amount=:amount, accountId=:account;";

$sql .= "UPDATE accounts SET balance = balance - :amount WHERE id = :account;COMMIT;";  // change here

kind of silly ;(

jsxqf
  • 557
  • 3
  • 13
  • 3
    that'd indicate an error in your database-library - if commit-calls are not translated into database-commits there is something horribly wrong. – specializt Dec 31 '14 at 18:05