Add commit result bool

[otazniksk]

Add commit result bool from connection commit

[jwage]

@morozov Is this ok to be done in master or should we target develop (3.0)?

[morozov]

I think master is fine. However, it must have a cross-platform integration test to pass. Right now, I believe this area is not well tested and there are no existing expectations of the proper behavior.

[morozov]

Please also see a previous discussion on the topic in #3175.

[otazniksk]

@morozov @jwage Added tests and return value bool to rollback

[morozov]

@otazniksk what is the point in returning a boolean if the boolean can be only true?

[pulzarraider]

@morozov lib/Doctrine/DBAL/Connection.php implements the interface /lib/Doctrine/DBAL/Driver/Connection.php.

For methods commit() and rollback() it says: @return bool TRUE on success or FALSE on failure.

This PR tries to solve the problem, that Doctrine DBAL is not implementing it's own interface correctly and it's hiding the real result of these methods from PDO.

All this is already discussed in #3175. We are experienced the same issues with Percona cluster as it's described in the related issue. The PDO is returning false but we have no option how to handle this problem because DBAL is not sending the result outside.

Together with @otazniksk we would like to create also another PR which will throws exception if the false occurs in commit() or rollback(). But that PR will be more controversial because the exception IMHO should be thrown inside PDO if the ERRMODE_EXCEPTION is set. So we will try to fix the php/PDO bug https://bugs.php.net/bug.php?id=66528 inside dbal.

[morozov]

This PR tries to solve the problem, that Doctrine DBAL is not implementing its own interface correctly and it's hiding the real result of these methods from PDO.

Please show me the code which didn't work before these changes and now will work. Ideally, in the form of an integration test. So far, I can only see the assertions that the methods return true but I don't see the false. This way, the interface is still not implemented.

[pulzarraider]

@morozov I have added test for commit() returning false. Seems that the rollback() method is returning true even the real command cannot be send to mysql so I can't add real functional test for this method.

Is it enough? Do you prefer replacing the false return value with exception in this PR?

[morozov]

The new test is exactly what I wanted to see 👍 Some cleanup is still needed.

[pulzarraider]

@morozov thank you for your review. I fixed all issues expect one.

[morozov]

[…] Seems that the rollback() method is returning true even the real command cannot be send to mysql so I can't add real functional test for this method. […]

Then we need to revert this part. Otherwise, what's the point?

[…] Do you prefer replacing the false return value with exception in this PR?

Not in 2.x. We already throw exceptions in 3.0-dev (#3480).

[pulzarraider]

[…] Seems that the rollback() method is returning true even the real command cannot be send to mysql so I can't add real functional test for this method. […]

Then we need to revert this part. Otherwise, what's the point?

@morozov rollback() changes reverted.

Commits squshed. Tests pass.
Ready to be reviewed and merged if everything is ok.

[morozov]

Thank you @otazniksk and @pulzarraider!

[pulzarraider]

Thank you @morozov.

[beberlei]

@morozov The commit testing for return false is restricted to MySQL only. As such it doesn't actually verify that this works cross platform and not some other platform now returns false from Connection::commit().

[morozov]

@beberlei I must have overseen this or there was another reason for that. I usually require that functional tests are run across all supported platforms/drivers.