Failures during commit and rollback are not handled

[mariusbalcytis]

Bug Report

Q	A
BC Break	no
Version	2.10.1

Summary

In case PDO::commit returns false, no exception is thrown, failures are not handled in any case.

This could happen, for example, if deadlock is received when using Galera cluster.

Current behaviour

Doctrine\DBAL\Connection returns false, which is totally ignored in all the places where it's called from, including EntityManager and even Connection internal methods.

No exception is thrown.

How to reproduce

<?php

declare(strict_types=1);

class ConnectionTest extends \Mockery\Adapter\Phpunit\MockeryTestCase
{
    public function testTransactionalWithCommitReturningFalse()
    {
        $connectionMock = \Mockery::mock(\Doctrine\DBAL\Driver\Connection::class);

        $driverMock = \Mockery::mock(\Doctrine\DBAL\Driver::class);
        $driverMock->expects('connect')->andReturn($connectionMock);

        $connection = new \Doctrine\DBAL\Connection(
            [],
            $driverMock
        );

        $connectionMock->expects('beginTransaction');
        $connectionMock->expects('exec')->with('SELECT 1')->andReturn(0);
        $connectionMock->expects('commit')->andReturn(false);

        $this->expectException(\Doctrine\DBAL\ConnectionException::class);

        $connection->transactional(function() use ($connection) {
            $connection->exec('SELECT 1');
        });
    }
}

In real-world, one would need:

• to set-up Galera cluster;
• update the same rows using several processes which connects to different Galera nodes.

This bug is really critical from the business side, as application responds with success code – everything seems to be working, but database changes are not really persisted. No errors are sent to the developers about such problems, too.

Expected behaviour

Provided test case should pass – commit method should throw an exception in case commit returns false and not to just return false where it's not handled by any code whatsoever.

The same, but possibly not so critical, behaviour should be applied for rollBack method – PDO::rollBack can also return false.

[l-bujakowski]

Very likely this is the bug report for underlying PDO cause: https://bugs.php.net/bug.php?id=66528

[mariusbalcytis]

Relates to the PDO issue, but not really:

• PDO::commit already says that it returns true on success or false on failure;
• Doctrine\DBAL\Connection::commit can throw ConnectionException, but just returns the boolean, even on failure;
• the return value is not checked anywhere where Doctrine\DBAL\Connection::commit is called, in Doctrine libraries (EntityManager, for instance).

So, this might be not an issue in DBAL, but in ORM. I think it's still best to be fixed in DBAL layer, as there's basically no point in not throwing exceptions in such cases, and it's quite hard and troublesome to check the return value throughout all the code.

[greg0ire]

If you want to contribute this, you will have to target 3.0.x since it will involve a BC-break: the signature should change from bool to void if we always throw in case of failure IMO. Also throwing where false was previously returned is a BC breaking in and of itself.

Please send a PR against that branch with your suggested changes, it will make it easier to discuss this.

[morozov]

This issue will be addressed in 4.0.0 (#3480). It cannot be fixed earlier since it requires an API change.

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.