Make SQLQuery strict semver for 3.2

docs/en/04_Changelogs/3.2.0.md

@@ -55,7 +55,7 @@
 * Implementation of a parameterised query framework eliminating the need to manually escape variables for
   use in SQL queries. This has been integrated into nearly every level of the database ORM.
 * Refactor of database connectivity classes into separate components linked together through dependency injection
-* Refactor of `SQLQuery` into separate objects for each query type: `SQLQuery`, `SQLDelete`, `SQLUpdate` and `SQLInsert`
+* Refactor of `SQLQuery` into separate objects for each query type: `SQLSelect`, `SQLDelete`, `SQLUpdate` and `SQLInsert`
 * PDO is now a standard connector, and is available for all database interfaces
 * `DataObject::doValidate()` method visibility added to access `DataObject::validate` externally
 * `NumericField` now uses HTML5 "number" type instead of "text"
@@ -455,6 +455,26 @@ After:
 	$query->execute();
 
 
+When working with SQLQuery passed into user code, it is advisable to strictly
+cast it into either a SQLSelect or SQLDelete. This can be done by using the new
+`SQLQuery::toAppropriateExpression()` method, which will automatically convert
+to the correct type based on whether the SQLQuery is set to delete or not.
+
+If a SQLQuery is not converted, then the result of `getWhere` will not be parameterised.
+This is because user code written for 3.1 expects this list to be a flat array
+of strings. This format is inherently unsafe, and should be avoided where possible.
+
+
+	:::php
+	<?php
+	public function augmentSQL(SQLQuery &$query) {
+		$query->getWhere(); // Will be flattened (unsafe 3.1 compatible format)
+		$expression = $query->toAppropriateExpression(); // Either SQLSelect or SQLDelete
+		$expression->getWhere(); // Will be parameterised (preferred 3.2 compatible format)
+	}
+
+
+
 Alternatively:
 
 
@@ -463,7 +483,8 @@ Alternatively:
 	$query = SQLQuery::create()
 		->setFrom('"SiteTree"')
 		->setWhere(array('"SiteTree"."ShowInMenus"' => 0))
-		->toDelete();
+		->setDelete(true)
+		->toAppropriateExpression();
 	$query->execute();
 
 
@@ -616,12 +637,14 @@ E.g.
 	DB::prepared_query('DELETE FROM "MyObject" WHERE ParentID = ? OR IsValid = ?', $parameters);
 
 
-#### 4. Interaction with `SQLQuery::getWhere()` method
+#### 4. Interaction with `SQLSelect::getWhere()` method
 
-As all where conditions are now parameterised, the format of the results returned by `SQLQuery::getWhere()`
-will not always equate with that in FrameWork 3.1. Once this would be a list of strings, what will
-now be returned is a list of conditions, each of which is an associative array mapping the
-condition string to a list of parameters provided.
+The `SQLSelect` class supercedes the old `SQLQuery` object for performing select queries. Although
+both implement the `getWhere()` method, the results returned by `SQLSelect::getWhere()` will be
+parameterised while `SQLQuery::getWhere()` will be a flattened array of strings.
+
+`SQLSelect::getWhere()` returns a list of conditions, each of which is an
+associative array mapping the condition string to a list of parameters provided.
 
 Before:
 
@@ -630,6 +653,7 @@ Before:
 	<?php
 
 	// Increment value of a single condition
+	$query = new SQLQuery(/*...*/);
 	$conditions = $query->getWhere();
 	$new = array();
 	foreach($conditions as $condition) {
@@ -646,6 +670,7 @@ After:
 
 	:::php
 	// Increment value of a single condition
+	$query = new SQLSelect(/*...*/);
 	$conditions = $query->getWhere();
 	$new = array();
 	foreach($conditions as $condition) {
@@ -665,7 +690,7 @@ replace this method call with the new `getWhereParameterised($parameters)` metho
 applicable.
 
 This method returns a manipulated form of the where conditions stored by the query, so
-that it matches the list of strings consistent with the old 3.1 `SQLQuery::getWhere()` behaviour.
+that it matches the list of strings similar to the old 3.1 `SQLQuery::getWhere()` behaviour.
 Additionally, the list of parameters is safely extracted, flattened, and can be passed out
 through the `$parameters` argument which is passed by reference.
 

model/DataQuery.php

@@ -90,7 +90,7 @@ class DataQuery {
 			$fieldExpression = key($fieldExpression);
 		}
 
-		$where = $this->query->getWhere();
+		$where = $this->query->toAppropriateExpression()->getWhere();
 		// Iterate through each condition
 		foreach($where as $i => $condition) {
 
@@ -836,6 +836,10 @@ class DataQuery {
  */
 class DataQuery_SubGroup extends DataQuery implements SQLConditionGroup {
 
+	/**
+	 *
+	 * @var SQLQuery
+	 */
 	protected $whereQuery;
 
 	public function __construct(DataQuery $base, $connective) {
@@ -867,11 +871,14 @@ class DataQuery_SubGroup extends DataQuery implements SQLConditionGroup {
 		$parameters = array();
 
 		// Ignore empty conditions
-		$where = $this->whereQuery->getWhere();
-		if(empty($where)) return null;
+		$query = $this->whereQuery->toAppropriateExpression();
+		$where = $query->getWhere();
+		if(empty($where)) {
+			return null;
+		}
 
 		// Allow database to manage joining of conditions
-		$sql = DB::get_conn()->getQueryBuilder()->buildWhereFragment($this->whereQuery, $parameters);
+		$sql = DB::get_conn()->getQueryBuilder()->buildWhereFragment($query, $parameters);
 		return preg_replace('/^\s*WHERE\s*/i', '', $sql);
 	}
 }

model/queries/SQLConditionalExpression.php

@@ -371,7 +371,7 @@ abstract class SQLConditionalExpression extends SQLExpression {
 	/**
 	 * Set a WHERE clause.
 	 *
-	 * @see SQLQuery::addWhere() for syntax examples
+	 * @see SQLConditionalExpression::addWhere() for syntax examples
 	 *
 	 * @param mixed $where Predicate(s) to set, as escaped SQL statements or paramaterised queries
 	 * @param mixed $where,... Unlimited additional predicates
@@ -473,7 +473,7 @@ abstract class SQLConditionalExpression extends SQLExpression {
 	}
 
 	/**
-	 * @see SQLQuery::addWhere()
+	 * @see SQLConditionalExpression::addWhere()
 	 *
 	 * @param mixed $filters Predicate(s) to set, as escaped SQL statements or paramaterised queries
 	 * @param mixed $filters,... Unlimited additional predicates
@@ -487,7 +487,7 @@ abstract class SQLConditionalExpression extends SQLExpression {
 	}
 
 	/**
-	 * @see SQLQuery::addWhere()
+	 * @see SQLConditionalExpression::addWhere()
 	 *
 	 * @param mixed $filters Predicate(s) to set, as escaped SQL statements or paramaterised queries
 	 * @param mixed $filters,... Unlimited additional predicates
@@ -694,12 +694,12 @@ abstract class SQLConditionalExpression extends SQLExpression {
 	}
 
 	/**
-	 * Generates an SQLQuery object using the currently specified parameters.
+	 * Generates an SQLSelect object using the currently specified parameters.
 	 *
-	 * @return SQLQuery
+	 * @return SQLSelect
 	 */
 	public function toSelect() {
-		$select = new SQLQuery();
+		$select = new SQLSelect();
 		$this->copyTo($select);
 		return $select;
 	}

model/queries/SQLQuery.php

@@ -10,6 +10,14 @@
  */
 class SQLQuery extends SQLSelect {
 
+	/**
+	 * If this is true, this statement will delete rather than select.
+	 * 
+	 * @deprecated since version 4.0
+	 * @var boolean
+	 */
+	protected $isDelete = false;
+
 	/**
 	 * @deprecated since version 4.0
 	 */
@@ -21,19 +29,180 @@ class SQLQuery extends SQLSelect {
 	}
 
 	/**
-	 * @deprecated since version 3.2
+	 * @deprecated since version 4.0
 	 */
 	public function setDelete($value) {
-		$message = 'SQLQuery->setDelete no longer works. Create a SQLDelete object instead, or use toDelete()';
-		Deprecation::notice('3.2', $message);
-		throw new BadMethodCallException($message);
+		Deprecation::notice('4.0', 'SQLQuery::setDelete is deprecated. Use toDelete instead');
+		$this->isDelete = $value;
 	}
 
 	/**
-	 * @deprecated since version 3.2
+	 * @deprecated since version 4.0
 	 */
 	public function getDelete() {
-		Deprecation::notice('3.2', 'Use SQLDelete object instead');
-		return false;
+		Deprecation::notice('4.0', 'SQLQuery::getDelete is deprecated. Use SQLSelect or SQLDelete instead');
+		return $this->isDelete;
+	}
+
+	public function sql(&$parameters = array()) {
+		return $this->toAppropriateExpression()->sql($parameters);
+	}
+	
+	/**
+	 * Get helper class for flattening parameterised conditions
+	 * 
+	 * @return SQLQuery_ParameterInjector
+	 */
+	protected function getParameterInjector() {
+		return Injector::inst()->get('SQLQuery_ParameterInjector');
+	}
+
+	/**
+	 * Return a list of SQL where conditions (flattened as a list of strings)
+	 * 
+	 * @return array
+	 */
+	public function getWhere() {
+		Deprecation::notice(
+			'4.0',
+			'SQLQuery::getWhere is non-parameterised for backwards compatibility. '.
+			'Use ->toAppropriateExpression()->getWhere() instead'
+		);
+		$conditions = parent::getWhere();
+		
+		// This is where any benefits of parameterised queries die
+		return $this
+			->getParameterInjector()
+			->injectConditions($conditions);
+	}
+
+	/**
+	 * Convert this SQLQuery to a SQLExpression based on its 
+	 * internal $delete state (Normally SQLSelect or SQLDelete)
+	 * 
+	 * @return SQLExpression
+	 */
+	public function toAppropriateExpression() {
+		if($this->isDelete) {
+			return parent::toDelete();
+		} else {
+			return parent::toSelect();
+		}
+	}
+
+	public function toSelect() {
+		if($this->isDelete) {
+			user_error(
+				'SQLQuery::toSelect called when $isDelete is true. Use ' .
+				'toAppropriateExpression() instead',
+				E_USER_WARNING
+			);
+		}
+		return parent::toSelect();
+	}
+
+	public function toDelete() {
+		if(!$this->isDelete) {
+			user_error(
+				'SQLQuery::toDelete called when $isDelete is false. Use ' .
+				'toAppropriateExpression() instead',
+				E_USER_WARNING
+			);
+		}
+		parent::toDelete();
+	}
+}
+
+/**
+ * Provides conversion of parameterised SQL to flattened SQL strings
+ * 
+ * @deprecated since version 4.0
+ */
+class SQLQuery_ParameterInjector {
+	
+	public function __construct() {
+		Deprecation::notice('4.0', "Use SQLSelect / SQLDelete instead of SQLQuery");
+	}
+
+	/**
+	 * Given a list of parameterised conditions, return a flattened
+	 * list of condition strings
+	 * 
+	 * @param array $conditions
+	 * @return array
+	 */
+	public function injectConditions($conditions) {
+		$result = array();
+		foreach($conditions as $condition) {
+			// Evaluate the result of SQLConditionGroup here
+			if($condition instanceof SQLConditionGroup) {
+				$predicate = $condition->conditionSQL($parameters);
+				if(!empty($predicate)) {
+					$result[] = $this->injectValues($predicate, $parameters);
+				}
+			} else {
+				foreach($condition as $predicate => $parameters) {
+					$result[] = $this->injectValues($predicate, $parameters);
+				}
+			}
+		}
+		return $result;
+	}
+
+	/**
+	 * Merge parameters into a SQL prepared condition
+	 * 
+	 * @param string $sql
+	 * @param array $parameters
+	 * @return string
+	 */
+	protected function injectValues($sql, $parameters) {
+		$segments = preg_split('/\?/', $sql);
+		$joined = '';
+		$inString = false;
+		for($i = 0; $i < count($segments); $i++) {
+			// Append next segment
+			$joined .= $segments[$i];
+			// Don't add placeholder after last segment
+			if($i === count($segments) - 1) {
+				break;
+			}
+			// check string escape on previous fragment
+			if($this->checkStringTogglesLiteral($segments[$i])) {
+				$inString = !$inString;
+			}
+			// Append placeholder replacement
+			if($inString) {
+				// Literal questionmark
+				$joined .= '?';
+				continue;
+			}
+			
+			// Encode and insert next parameter
+			$next = array_shift($parameters);
+			if(is_array($next) && isset($next['value'])) {
+				$next = $next['value'];
+			}
+			$joined .= "'".Convert::raw2sql($next)."'";
+		}
+		return $joined;
+	}
+
+	/**
+	 * Determines if the SQL fragment either breaks into or out of a string literal
+	 * by counting single quotes
+	 * 
+	 * Handles double-quote escaped quotes as well as slash escaped quotes
+	 * 
+	 * @param string $input The SQL fragment
+	 * @return boolean True if the string breaks into or out of a string literal
+	 */
+	protected function checkStringTogglesLiteral($input) {
+		// Remove escaped backslashes, count them!
+		$input = preg_replace('/\\\\\\\\/', '', $input);
+		// Count quotes
+		$totalQuotes = substr_count($input, "'"); // Includes double quote escaped quotes
+		$escapedQuotes = substr_count($input, "\\'");
+		return (($totalQuotes - $escapedQuotes) % 2) !== 0;
 	}
 }

model/queries/SQLSelect.php

@@ -8,7 +8,7 @@
  * @subpackage model
  */
 class SQLSelect extends SQLConditionalExpression {
-
+	
 	/**
 	 * An array of SELECT fields, keyed by an optional alias.
 	 *

tests/model/SQLQueryTest.php

@@ -13,6 +13,18 @@ class SQLQueryTest extends SapphireTest {
 		'SQLQueryTestBase',
 		'SQLQueryTestChild'
 	);
+	
+	protected $oldDeprecation = null;
+	
+	public function setUp() {
+		parent::setUp();
+		$this->oldDeprecation = Deprecation::dump_settings();
+	}
+	
+	public function tearDown() {
+		Deprecation::restore_settings($this->oldDeprecation);
+		parent::tearDown();
+	}
 
 	public function testCount() {
 
@@ -681,6 +693,100 @@ class SQLQueryTest extends SapphireTest {
 		$this->assertEquals(array('%MyName%', '2012-08-08 12:00'), $parameters);
 		$query->execute();
 	}
+	
+	/**
+	 * Test deprecation of SQLQuery::getWhere working appropriately
+	 */
+	public function testDeprecatedGetWhere() {
+		// Temporarily disable deprecation
+		Deprecation::notification_version(null);
+		
+		$query = new SQLQuery();
+		$query->setSelect(array('"SQLQueryTest_DO"."Name"'));
+		$query->setFrom('"SQLQueryTest_DO"');
+		$query->addWhere(array(
+			'"SQLQueryTest_DO"."Date" > ?' => '2012-08-08 12:00'
+		));
+		$query->addWhere('"SQLQueryTest_DO"."Name" = \'Richard\'');
+		$query->addWhere(array(
+			'"SQLQueryTest_DO"."Meta" IN (?, \'Who?\', ?)' => array('Left', 'Right')
+		));
+		
+		$expectedSQL = <<<EOS
+SELECT "SQLQueryTest_DO"."Name"
+ FROM "SQLQueryTest_DO"
+ WHERE ("SQLQueryTest_DO"."Date" > ?)
+ AND ("SQLQueryTest_DO"."Name" = 'Richard')
+ AND ("SQLQueryTest_DO"."Meta" IN (?, 'Who?', ?))
+EOS
+			;
+		$expectedParameters = array('2012-08-08 12:00', 'Left', 'Right');
+		
+		
+		// Check sql evaluation of this query maintains the parameters
+		$sql = $query->sql($parameters);
+		$this->assertSQLEquals($expectedSQL, $sql);
+		$this->assertEquals($expectedParameters, $parameters);
+		
+		// Check that ->toAppropriateExpression()->setWhere doesn't modify the query
+		$query->setWhere($query->toAppropriateExpression()->getWhere());
+		$sql = $query->sql($parameters);
+		$this->assertSQLEquals($expectedSQL, $sql);
+		$this->assertEquals($expectedParameters, $parameters);
+		
+		// Check that getWhere are all flattened queries
+		$expectedFlattened = array(
+			'"SQLQueryTest_DO"."Date" > \'2012-08-08 12:00\'',
+			'"SQLQueryTest_DO"."Name" = \'Richard\'',
+			'"SQLQueryTest_DO"."Meta" IN (\'Left\', \'Who?\', \'Right\')'
+		);
+		$this->assertEquals($expectedFlattened, $query->getWhere());
+	}
+	
+	/**
+	 * Test deprecation of SQLQuery::setDelete/getDelete
+	 */
+	public function testDeprecatedSetDelete() {
+		// Temporarily disable deprecation
+		Deprecation::notification_version(null);
+		
+		$query = new SQLQuery();
+		$query->setSelect(array('"SQLQueryTest_DO"."Name"'));
+		$query->setFrom('"SQLQueryTest_DO"');
+		$query->setWhere(array('"SQLQueryTest_DO"."Name"' => 'Andrew'));
+
+		// Check SQL for select
+		$this->assertSQLEquals(<<<EOS
+SELECT "SQLQueryTest_DO"."Name" FROM "SQLQueryTest_DO"
+WHERE ("SQLQueryTest_DO"."Name" = ?)
+EOS
+			,
+			$query->sql($parameters)
+		);
+		$this->assertEquals(array('Andrew'), $parameters);
+		
+		// Check setDelete works
+		$query->setDelete(true);
+	$this->assertSQLEquals(<<<EOS
+DELETE FROM "SQLQueryTest_DO"
+WHERE ("SQLQueryTest_DO"."Name" = ?)
+EOS
+			,
+			$query->sql($parameters)
+		);
+		$this->assertEquals(array('Andrew'), $parameters);
+		
+		// Check that setDelete back to false restores the state
+		$query->setDelete(false);
+		$this->assertSQLEquals(<<<EOS
+SELECT "SQLQueryTest_DO"."Name" FROM "SQLQueryTest_DO"
+WHERE ("SQLQueryTest_DO"."Name" = ?)
+EOS
+			,
+			$query->sql($parameters)
+		);
+		$this->assertEquals(array('Andrew'), $parameters);
+	}
 }
 
 class SQLQueryTest_DO extends DataObject implements TestOnly {