Transaction handling is fundamentally broken due to automatic reconnect behaviour in execute() method

[ChristianEhlscheid]

Steps to reproduce the issue

It's not that simple to reproduce this error since you have to simulate a database disconnect.

The following code illustrates the problem, it targets MySQL (but all database drivers are affected since every database rolls back transactions automatically when the connection gets lost).

$db = JFactory::getDbo(); // set up connection (Joomla CMS)
$db->setQuery('CREATE TABLE IF NOT EXISTS transactiontest (pk_value INT, somecol INT)');
$db->execute();
try {
    $db->transactionStart();
    $db->setQuery('INSERT INTO transactiontest VALUES (1,1)'); 
    $db->execute(); 
    // to simulate a database disconnect set a brakepoint in the debugger on the next line 
    // and when it get's hit restart your database and then continue debugging when the database is running again
    $db->setQuery('INSERT INTO transactiontest VALUES (2,2)'); 
    // the next line should fail but it does not since the automatic reconnect behaviour inside the execute method
    // will kick in and reconnect sucessfully and just execute the query without triggering an error
    $db->execute(); 
    $db->transactionCommit();
}
catch(RuntimeException $e)
{
    // something failed - rollback
    $db->transactionRollback();
}

$db->setQuery('SELECT * FROM transactiontest');
$rows = $db->loadObjectList();
// excpected result is zero rows, but it returns 1 row with the values 2,2
var_dump($rows); 

Expected result

No rows inserted into the database

Actual result

1 row inserted into the database

System information (as much as possible)

Doesn't matter - since the issue is independent of OS, database or driver that's used.
All platforms are affected.

Additional comments

There are two solutions to the issue.

1. remove the automatic reconnect code in the execute() method, this is probably problematic since it
   isn't in general a bad idea to do so, intermittend database outages might occur and if one is not inside a transaction the reconnect behaviour is a good thing

2. only try to reconnect if not inside a transaction - this is actually my proposed solution since
   to implement that just one line needs to be changed in the execute method (of all database classes)

// If an error occurred handle it.
if (!$this->cursor)
{
	// Get the error number and message before we execute any more queries.
	$this->errorNum = $this->getErrorNumber();
	$this->errorMsg = $this->getErrorMessage();

	// Check if the server was disconnected, but only if not inside a transaction
	if ($this->transactionDepth == 0 && !$this->connected())
	{
		try
		{
			// Attempt to reconnect.
			$this->connection = null;
			$this->connect();
		}
...

[PhilETaylor]

This is probably a @mbabker type thing :)

[mbabker]

Could you do a pull request with your change? Transactions aren't a strong point in my SQL knowledge but the fix makes sense to me.

[csthomas]

Does PR should this go to the master or 2.0-dev branch?

[mbabker]

master

[richard67]

Has this been solved meanwhile?