Merge pull request #4288 from tractorcow/pulls/3.2/fix-pdoconnector

BUG Fix major segfault on PDOConnector after any DDL / Issue in PDOQuery::first()

model/connect/DBConnector.php

@@ -10,11 +10,20 @@ abstract class DBConnector {
 
 	/**
 	 * List of operations to treat as write
+	 * Implicitly includes all ddl_operations
 	 *
 	 * @config
 	 * @var array
 	 */
-	private static $write_operations = array('insert', 'update', 'delete', 'replace', 'alter', 'drop');
+	private static $write_operations = array('insert', 'update', 'delete', 'replace');
+	
+	/**
+	 * List of operations to treat as DDL
+	 *
+	 * @config
+	 * @var array
+	 */
+	private static $ddl_operations = array('alter', 'drop', 'create', 'truncate');
 
 	/**
 	 * Error handler for database errors.
@@ -27,6 +36,7 @@ abstract class DBConnector {
 	 * @param integer $errorLevel The level of the error to throw.
 	 * @param string $sql The SQL related to this query
 	 * @param array $parameters Parameters passed to the query
+	 * @throws SS_DatabaseException
 	 */
 	protected function databaseError($msg, $errorLevel = E_USER_ERROR, $sql = null, $parameters = array()) {
 		// Prevent errors when error checking is set at zero level
@@ -49,46 +59,57 @@ abstract class DBConnector {
 	}
 	
 	/**
-	 * Determines if the query should be previewed, and thus interrupted silently.
-	 * If so, this function also displays the query via the debuging system.
-	 * Subclasess should respect the results of this call for each query, and not
-	 * execute any queries that generate a true response.
+	 * Determine if this SQL statement is a destructive operation (write or ddl)
 	 * 
-	 * @param string $sql The query to be executed
-	 * @return boolean Flag indicating that the query was previewed
+	 * @param string $sql
+	 * @return bool
 	 */
-	protected function previewWrite($sql) {
-		// Break if not requested
-		if (!isset($_REQUEST['previewwrite'])) return false;
-
-		// Break if non-write operation
-		$operation = strtolower(substr($sql, 0, strpos($sql, ' ')));
-		$writeOperations = Config::inst()->get(get_class($this), 'write_operations');
-		if (!in_array($operation, $writeOperations)) {
-			return false;
-		}
-
-		// output preview message
-		Debug::message("Will execute: $sql");
-		return true;
+	public function isQueryMutable($sql) {
+		$operations = array_merge(
+			Config::inst()->get(get_class($this), 'write_operations'),
+			Config::inst()->get(get_class($this), 'ddl_operations')
+		);
+		return $this->isQueryType($sql, $operations);
 	}
 	
 	/**
-	 * Allows the display and benchmarking of queries as they are being run
+	 * Determine if this SQL statement is a DDL operation
 	 * 
-	 * @param string $sql Query to run, and single parameter to callback
-	 * @param callable $callback Callback to execute code
-	 * @return mixed Result of query
+	 * @param string $sql
+	 * @return bool
 	 */
-	protected function benchmarkQuery($sql, $callback) {
-		if (isset($_REQUEST['showqueries']) && Director::isDev(true)) {
-			$starttime = microtime(true);
-			$result = $callback($sql);
-			$endtime = round(microtime(true) - $starttime, 4);
-			Debug::message("\n$sql\n{$endtime}s\n", false);
-			return $result;
+	public function isQueryDDL($sql) {
+		$operations = Config::inst()->get(get_class($this), 'ddl_operations');
+		return $this->isQueryType($sql, $operations);
+	}
+	
+	/**
+	 * Determine if this SQL statement is a write operation
+	 * (alters content but not structure)
+	 * 
+	 * @param string $sql
+	 * @return bool
+	 */
+	public function isQueryWrite($sql) {
+		$operations = Config::inst()->get(get_class($this), 'write_operations');
+		return $this->isQueryType($sql, $operations);
+	}
+	
+	/**
+	 * Determine if a query is of the given type
+	 * 
+	 * @param string $sql Raw SQL
+	 * @param string|array $type Type or list of types (first word in the query). Must be lowercase
+	 */
+	protected function isQueryType($sql, $type) {
+		if(!preg_match('/^(?<operation>\w+)\b/', $sql, $matches)) {
+			return false;
+		}
+		$operation = $matches['operation'];
+		if(is_array($type)) {
+			return in_array(strtolower($operation), $type);
 		} else {
-			return $callback($sql);
+			return strcasecmp($sql, $type) === 0;
 		}
 	}
 

model/connect/Database.php

@@ -96,7 +96,19 @@ abstract class SS_Database {
 	 * @return SS_Query
 	 */
 	public function query($sql, $errorLevel = E_USER_ERROR) {
-		return $this->connector->query($sql, $errorLevel);
+		// Check if we should only preview this query
+		if ($this->previewWrite($sql)) {
+			return;
+		}
+
+		// Benchmark query
+		$connector = $this->connector;
+		return $this->benchmarkQuery(
+			$sql,
+			function($sql) use($connector, $errorLevel) {
+				return $connector->query($sql, $errorLevel);
+			}
+		);
 	}
 
 
@@ -109,7 +121,62 @@ abstract class SS_Database {
 	 * @return SS_Query
 	 */
 	public function preparedQuery($sql, $parameters, $errorLevel = E_USER_ERROR) {
-		return $this->connector->preparedQuery($sql, $parameters, $errorLevel);
+		// Check if we should only preview this query
+		if ($this->previewWrite($sql)) {
+			return;
+		}
+
+		// Benchmark query
+		$connector = $this->connector;
+		return $this->benchmarkQuery(
+			$sql,
+			function($sql) use($connector, $parameters, $errorLevel) {
+				return $connector->preparedQuery($sql, $parameters, $errorLevel);
+			}
+		);
+	}
+
+	/**
+	 * Determines if the query should be previewed, and thus interrupted silently.
+	 * If so, this function also displays the query via the debuging system.
+	 * Subclasess should respect the results of this call for each query, and not
+	 * execute any queries that generate a true response.
+	 *
+	 * @param string $sql The query to be executed
+	 * @return boolean Flag indicating that the query was previewed
+	 */
+	protected function previewWrite($sql) {
+		// Only preview if previewWrite is set, we are in dev mode, and
+		// the query is mutable
+		if (isset($_REQUEST['previewwrite'])
+			&& Director::isDev()
+			&& $this->connector->isQueryMutable($sql)
+		) {
+			// output preview message
+			Debug::message("Will execute: $sql");
+			return true;
+		} else {
+			return false;
+		}
+	}
+
+	/**
+	 * Allows the display and benchmarking of queries as they are being run
+	 *
+	 * @param string $sql Query to run, and single parameter to callback
+	 * @param callable $callback Callback to execute code
+	 * @return mixed Result of query
+	 */
+	protected function benchmarkQuery($sql, $callback) {
+		if (isset($_REQUEST['showqueries']) && Director::isDev()) {
+			$starttime = microtime(true);
+			$result = $callback($sql);
+			$endtime = round(microtime(true) - $starttime, 4);
+			Debug::message("\n$sql\n{$endtime}s\n", false);
+			return $result;
+		} else {
+			return $callback($sql);
+		}
 	}
 
 	/**

model/connect/MySQLiConnector.php

@@ -104,21 +104,21 @@ class MySQLiConnector extends DBConnector {
 		return $this->dbConn->server_info;
 	}
 	
-	protected function benchmarkQuery($sql, $callback) {
+	/**
+	 * Invoked before any query is executed
+	 * 
+	 * @param string $sql
+	 */
+	protected function beforeQuery($sql) {
 		// Clear the last statement
 		$this->setLastStatement(null);
-		return parent::benchmarkQuery($sql, $callback);
 	}
 
 	public function query($sql, $errorLevel = E_USER_ERROR) {
-		// Check if we should only preview this query
-		if ($this->previewWrite($sql)) return;
+		$this->beforeQuery($sql);
 
 		// Benchmark query
-		$conn = $this->dbConn;
-		$handle = $this->benchmarkQuery($sql, function($sql) use($conn) {
-			return $conn->query($sql, MYSQLI_STORE_RESULT);
-		});
+		$handle = $this->dbConn->query($sql, MYSQLI_STORE_RESULT);
 
 		if (!$handle || $this->dbConn->error) {
 			$this->databaseError($this->getLastError(), $errorLevel, $sql);
@@ -208,23 +208,20 @@ class MySQLiConnector extends DBConnector {
 
 	public function preparedQuery($sql, $parameters, $errorLevel = E_USER_ERROR) {
 		// Shortcut to basic query when not given parameters
-		if(empty($parameters)) return $this->query($sql, $errorLevel);
+		if(empty($parameters)) {
+			return $this->query($sql, $errorLevel);
+		}
 
-		// Check if we should only preview this query
-		if ($this->previewWrite($sql)) return;
+		$this->beforeQuery($sql);
 
 		// Type check, identify, and prepare parameters for passing to the statement bind function
 		$parsedParameters = $this->parsePreparedParameters($parameters, $blobs);
 
 		// Benchmark query
-		$self = $this;
-		$lastStatement = $this->benchmarkQuery($sql, function($sql) use($parsedParameters, $blobs, $self) {
-
-			$statement = $self->prepareStatement($sql, $success);
-			if(!$success) return null;
-
+		$statement = $this->prepareStatement($sql, $success);
+		if($success) {
 			if($parsedParameters) {
-				$self->bindParameters($statement, $parsedParameters);
+				$this->bindParameters($statement, $parsedParameters);
 			}
 
 			// Bind any blobs given
@@ -234,18 +231,18 @@ class MySQLiConnector extends DBConnector {
 
 			// Safely execute the statement
 			$statement->execute();
-			return $statement;
-		});
+		}
 		
-		if (!$lastStatement || $lastStatement->error) {
+		if (!$success || $statement->error) {
 			$values = $this->parameterValues($parameters);
 			$this->databaseError($this->getLastError(), $errorLevel, $sql, $values);
 			return null;
 		}
 
 		// Non-select queries will have no result data
-		if($lastStatement && ($metaData = $lastStatement->result_metadata())) {
-			return new MySQLStatement($lastStatement, $metaData);
+		$metaData = $statement->result_metadata();
+		if($metaData) {
+			return new MySQLStatement($statement, $metaData);
 		} else {
 			// Replicate normal behaviour of ->query() on non-select calls
 			return new MySQLQuery($this, true);

model/connect/PDOConnector.php

@@ -30,11 +30,18 @@ class PDOConnector extends DBConnector {
 	protected $databaseName = null;
 
 	/**
-	 * The most recent statement returned from PDODatabase->query
+	 * If available, the row count of the last executed statement
 	 *
-	 * @var PDOStatement
+	 * @var int|null
 	 */
-	protected $lastStatement = null;
+	protected $rowCount = null;
+	
+	/**
+	 * Error generated by the errorInfo() method of the last PDOStatement
+	 *
+	 * @var array|null 
+	 */
+	protected $lastStatementError = null;
 
 	/**
 	 * List of prepared statements, cached by SQL string
@@ -58,13 +65,22 @@ class PDOConnector extends DBConnector {
 	 * @return PDOStatement
 	 */
 	public function getOrPrepareStatement($sql) {
-		if(empty($this->cachedStatements[$sql])) {
-			$this->cachedStatements[$sql] = $this->pdoConnection->prepare(
+		// Return cached statements
+		if(isset($this->cachedStatements[$sql])) {
+			return $this->cachedStatements[$sql];
+		}
+		
+		// Generate new statement
+		$statement = $this->pdoConnection->prepare(
 			$sql,
 			array(PDO::ATTR_CURSOR => PDO::CURSOR_FWDONLY)
 		);
+		
+		// Only cache select statements
+		if(preg_match('/^(\s*)select\b/i', $sql)) {
+			$this->cachedStatements[$sql] = $statement;
 		}
-		return $this->cachedStatements[$sql];
+		return $statement;
 	}
 
 	/**
@@ -168,52 +184,52 @@ class PDOConnector extends DBConnector {
 	}
 	
 	/**
-	 * Executes a query that doesn't return a resultset
+	 * Invoked before any query is executed
 	 * 
 	 * @param string $sql
+	 */
+	protected function beforeQuery($sql) {
+		// Reset state
+		$this->rowCount = 0;
+		$this->lastStatementError = null;
+		
+		// Flush if necessary
+		if($this->isQueryDDL($sql)) {
+			$this->flushStatements();
+		}
+	}
+
+	/**
+	 * Executes a query that doesn't return a resultset
+	 *
 	 * @param string $sql The SQL query to execute
 	 * @param integer $errorLevel For errors to this query, raise PHP errors
 	 * using this error level.
+	 * @return int
 	 */
 	public function exec($sql, $errorLevel = E_USER_ERROR) {
-		// Check if we should only preview this query
-		if ($this->previewWrite($sql)) return;
+		$this->beforeQuery($sql);
 		
-		// Reset last statement to prevent interference in case of error
-		$this->lastStatement = null;
-
-		// Benchmark query
-		$pdo = $this->pdoConnection;
-		$result = $this->benchmarkQuery($sql, function($sql) use($pdo) {
-			return $pdo->exec($sql);
-		});
+		// Directly exec this query
+		$result = $this->pdoConnection->exec($sql);
 
 		// Check for errors
-		if ($result === false) {
-			$this->databaseError($this->getLastError(), $errorLevel, $sql);
-			return null;
+		if ($result !== false) {
+			return $this->rowCount = $result;
 		}
 		
-		return $result;
+		$this->databaseError($this->getLastError(), $errorLevel, $sql);
+		return null;
 	}
 
 	public function query($sql, $errorLevel = E_USER_ERROR) {
-		// Check if we should only preview this query
-		if ($this->previewWrite($sql)) return;
+		$this->beforeQuery($sql);
 		
-		// Benchmark query
-		$pdo = $this->pdoConnection;
-		$this->lastStatement = $this->benchmarkQuery($sql, function($sql) use($pdo) {
-			return $pdo->query($sql);
-		});
+		// Directly query against connection
+		$statement = $this->pdoConnection->query($sql);
 		
-		// Check for errors
-		if (!$this->lastStatement || $this->hasError($this->lastStatement)) {
-			$this->databaseError($this->getLastError(), $errorLevel, $sql);
-			return null;
-		}
-
-		return new PDOQuery($this->lastStatement);
+		// Generate results
+		return $this->prepareResults($statement, $errorLevel, $sql);
 	}
 
 	/**
@@ -272,33 +288,54 @@ class PDOConnector extends DBConnector {
 	}
 
 	public function preparedQuery($sql, $parameters, $errorLevel = E_USER_ERROR) {
-		// Check if we should only preview this query
-		if ($this->previewWrite($sql)) return;
-
-		// Benchmark query
-		$self = $this;
-		$this->lastStatement = $this->benchmarkQuery($sql, function($sql) use($parameters, $self) {
+		$this->beforeQuery($sql);
 		
 		// Prepare statement
-			$statement = $self->getOrPrepareStatement($sql);
-			if(!$statement) return null;
+		$statement = $this->getOrPrepareStatement($sql);
 		
-			// Inject parameters
-			$self->bindParameters($statement, $parameters);
-
-			// Safely execute the statement
+		// Bind and invoke statement safely
+		if($statement) {
+			$this->bindParameters($statement, $parameters);
 			$statement->execute($parameters);
-			return $statement;
-		});
-
-		// Check for errors
-		if (!$this->lastStatement || $this->hasError($this->lastStatement)) {
-			$values = $this->parameterValues($parameters);
-			$this->databaseError($this->getLastError(), $errorLevel, $sql, $values);
-			return null;
 		}
 		
-		return new PDOQuery($this->lastStatement);
+		// Generate results
+		return $this->prepareResults($statement, $errorLevel, $sql);
+	}
+	
+	/**
+	 * Given a PDOStatement that has just been executed, generate results
+	 * and report any errors
+	 * 
+	 * @param PDOStatement $statement
+	 * @param int $errorLevel
+	 * @param string $sql
+	 * @param array $parameters
+	 * @return \PDOQuery
+	 */
+	protected function prepareResults($statement, $errorLevel, $sql, $parameters = array()) {
+		
+		// Record row-count and errors of last statement
+		if($this->hasError($statement)) {
+			$this->lastStatementError = $statement->errorInfo();
+		} elseif($statement) {
+			// Count and return results
+			$this->rowCount = $statement->rowCount();
+			return new PDOQuery($statement);
+		}
+
+		// Ensure statement is closed
+		if($statement) {
+			$statement->closeCursor();
+			unset($statement);
+		}
+		
+		// Report any errors
+		if($parameters) {
+			$parameters = $this->parameterValues($parameters);
+		}
+		$this->databaseError($this->getLastError(), $errorLevel, $sql, $parameters);
+		return null;
 	}
 
 	/**
@@ -309,11 +346,15 @@ class PDOConnector extends DBConnector {
 	 */
 	protected function hasError($resource) {
 		// No error if no resource
-		if(empty($resource)) return false;
+		if(empty($resource)) {
+			return false;
+		}
 
 		// If the error code is empty the statement / connection has not been run yet
 		$code = $resource->errorCode();
-		if(empty($code)) return false;
+		if(empty($code)) {
+			return false;
+		}
 
 		// Skip 'ok' and undefined 'warning' types.
 		// @see http://docstore.mik.ua/orelly/java-ent/jenut/ch08_06.htm
@@ -321,12 +362,13 @@ class PDOConnector extends DBConnector {
 	}
 	
 	public function getLastError() {
-		if ($this->hasError($this->lastStatement)) {
-			$error = $this->lastStatement->errorInfo();
+		$error = null;
+		if ($this->lastStatementError) {
+			$error = $this->lastStatementError;
 		} elseif($this->hasError($this->pdoConnection)) {
 			$error = $this->pdoConnection->errorInfo();
 		}
-		if (isset($error)) {
+		if ($error) {
 			return sprintf("%s-%s: %s", $error[0], $error[1], $error[2]);
 		}
 	}
@@ -336,8 +378,7 @@ class PDOConnector extends DBConnector {
 	}
 
 	public function affectedRows() {
-		if (empty($this->lastStatement)) return 0;
-		return $this->lastStatement->rowCount();
+		return $this->rowCount;
 	}
 
 	public function selectDatabase($name) {

model/connect/Query.php

@@ -146,6 +146,7 @@ abstract class SS_Query implements Iterator {
 	public function rewind() {
 		if ($this->queryHasBegun && $this->numRecords() > 0) {
 			$this->queryHasBegun = false;
+			$this->currentRecord = null;
 			return $this->seek(0);
 		}
 	}

tests/model/PDODatabaseTest.php

@@ -0,0 +1,113 @@
+<?php
+/**
+ * @package framework
+ * @subpackage testing
+ */
+
+class PDODatabaseTest extends SapphireTest {
+	
+	protected static $fixture_file = 'MySQLDatabaseTest.yml';
+
+	protected $extraDataObjects = array(
+		'MySQLDatabaseTest_Data'
+	);
+
+	public function testPreparedStatements() {
+		if(!(DB::get_connector() instanceof PDOConnector)) {
+			$this->markTestSkipped('This test requires the current DB connector is PDO');
+		}
+
+		// Test preparation of equivalent statemetns
+		$result1 = DB::get_connector()->preparedQuery(
+			'SELECT "Sort", "Title" FROM "MySQLDatabaseTest_Data" WHERE "Sort" > ? ORDER BY "Sort"',
+			array(0)
+		);
+
+		$result2 = DB::get_connector()->preparedQuery(
+			'SELECT "Sort", "Title" FROM "MySQLDatabaseTest_Data" WHERE "Sort" > ? ORDER BY "Sort"',
+			array(2)
+		);
+		$this->assertInstanceOf('PDOQuery', $result1);
+		$this->assertInstanceOf('PDOQuery', $result2);
+
+		// Also select non-prepared statement
+		$result3 = DB::get_connector()->query('SELECT "Sort", "Title" FROM "MySQLDatabaseTest_Data" ORDER BY "Sort"');
+		$this->assertInstanceOf('PDOQuery', $result3);
+
+		// Iterating one level should not buffer, but return the right result
+		$this->assertEquals(
+			array(
+				'Sort' => 1,
+				'Title' => 'First Item'
+			),
+			$result1->next()
+		);
+		$this->assertEquals(
+			array(
+				'Sort' => 2,
+				'Title' => 'Second Item'
+			),
+			$result1->next()
+		);
+
+		// Test first
+		$this->assertEquals(
+			array(
+				'Sort' => 1,
+				'Title' => 'First Item'
+			),
+			$result1->first()
+		);
+
+		// Test seek
+		$this->assertEquals(
+			array(
+				'Sort' => 2,
+				'Title' => 'Second Item'
+			),
+			$result1->seek(1)
+		);
+
+		// Test count
+		$this->assertEquals(4, $result1->numRecords());
+
+		// Test second statement
+		$this->assertEquals(
+			array(
+				'Sort' => 3,
+				'Title' => 'Third Item'
+			),
+			$result2->next()
+		);
+
+		// Test non-prepared query
+		$this->assertEquals(
+			array(
+				'Sort' => 1,
+				'Title' => 'First Item'
+			),
+			$result3->next()
+		);
+	}
+
+	public function testAffectedRows() {
+		if(!(DB::get_connector() instanceof PDOConnector)) {
+			$this->markTestSkipped('This test requires the current DB connector is PDO');
+		}
+
+		$query = new SQLUpdate('MySQLDatabaseTest_Data');
+		$query->setAssignments(array('Title' => 'New Title'));
+
+		// Test update which affects no rows
+		$query->setWhere(array('Title' => 'Bob'));
+		$result = $query->execute();
+		$this->assertInstanceOf('PDOQuery', $result);
+		$this->assertEquals(0, DB::affected_rows());
+
+		// Test update which affects some rows
+		$query->setWhere(array('Title' => 'First Item'));
+		$result = $query->execute();
+		$this->assertInstanceOf('PDOQuery', $result);
+		$this->assertEquals(1, DB::affected_rows());
+	}
+}