changes to make query() function return boolean for write-type queries. centralize isWriteType functionality in one place. Change tests impacted by this change

sneakyimp · sneakyimp · commit 094d0032a506 · 2021-02-11T16:13:38.000-08:00
diff --git a/system/BaseModel.php b/system/BaseModel.php
@@ -357,7 +357,7 @@ abstract protected function doFirst();
 	 *
 	 * @param array $data Data
 	 *
-	 * @return object|integer|string|false
+	 * @return integer|string|boolean
 	 */
 	abstract protected function doInsert(array $data);
 
@@ -407,7 +407,7 @@ abstract protected function doUpdateBatch(array $set = null, string $index = nul
 	 * @param integer|string|array|null $id    The rows primary key(s)
 	 * @param boolean                   $purge Allows overriding the soft deletes setting.
 	 *
-	 * @return object|boolean
+	 * @return string|boolean
 	 *
 	 * @throws DatabaseException
 	 */
@@ -666,11 +666,7 @@ public function save($data): bool
 		{
 			$response = $this->insert($data, false);
 
-			if ($response instanceof BaseResult)
-			{
-				$response = $response->resultID !== false;
-			}
-			elseif ($response !== false)
+			if ($response !== false)
 			{
 				$response = true;
 			}
@@ -708,7 +704,7 @@ public function getInsertID()
 	 * @param array|object|null $data     Data
 	 * @param boolean           $returnID Whether insert ID should be returned or not.
 	 *
-	 * @return BaseResult|object|integer|string|false
+	 * @return integer|string|boolean
 	 *
 	 * @throws ReflectionException
 	 */
diff --git a/system/Database/BaseBuilder.php b/system/Database/BaseBuilder.php
@@ -2236,7 +2236,7 @@ public function getCompiledInsert(bool $reset = true)
 	 *
 	 * @throws DatabaseException
 	 *
-	 * @return BaseResult|Query|false
+	 * @return Query|boolean
 	 */
 	public function insert(array $set = null, bool $escape = null)
 	{
@@ -2467,7 +2467,7 @@ public function update(array $set = null, $where = null, int $limit = null): boo
 
 			$result = $this->db->query($sql, $this->binds, false);
 
-			if ($result->resultID !== false)
+			if ($result !== false)
 			{
 				// Clear our binds so we don't eat up memory
 				$this->binds = [];
@@ -2816,7 +2816,7 @@ public function getCompiledDelete(bool $reset = true): string
 	 * @param integer $limit     The limit clause
 	 * @param boolean $resetData
 	 *
-	 * @return mixed
+	 * @return string|boolean
 	 * @throws DatabaseException
 	 */
 	public function delete($where = '', int $limit = null, bool $resetData = true)
diff --git a/system/Database/BaseConnection.php b/system/Database/BaseConnection.php
@@ -605,7 +605,7 @@ abstract protected function execute(string $sql);
 	 * @param boolean $setEscapeFlags
 	 * @param string  $queryClass
 	 *
-	 * @return BaseResult|Query|false
+	 * @return BaseResult|Query|boolean
 	 *
 	 * @todo BC set $queryClass default as null in 4.1
 	 */
@@ -618,7 +618,6 @@ public function query(string $sql, $binds = null, bool $setEscapeFlags = true, s
 			$this->initialize();
 		}
 
-		$resultClass = str_replace('Connection', 'Result', get_class($this));
 		/**
 		 * @var Query $query
 		 */
@@ -689,7 +688,20 @@ public function query(string $sql, $binds = null, bool $setEscapeFlags = true, s
 		// If $pretend is true, then we just want to return
 		// the actual query object here. There won't be
 		// any results to return.
-		return $this->pretend ? $query : new $resultClass($this->connID, $this->resultID);
+		if ($this->pretend)
+		{
+			return $query;
+		}
+
+		// resultID is not false, so it must be successful
+		if (\CodeIgniter\Database\Query::sqlIsWriteType($sql)) {
+			return true;
+		}
+
+		// query is not write-type, so it must be read-type query; return QueryResult
+		$resultClass = str_replace('Connection', 'Result', get_class($this));
+		return new $resultClass($this->connID, $this->resultID);
+
 	}
 
 	//--------------------------------------------------------------------
diff --git a/system/Database/BaseUtils.php b/system/Database/BaseUtils.php
@@ -120,7 +120,7 @@ public function databaseExists(string $databaseName): bool
 	 * Optimize Table
 	 *
 	 * @param  string $tableName
-	 * @return mixed
+	 * @return boolean
 	 * @throws DatabaseException
 	 */
 	public function optimizeTable(string $tableName)
@@ -135,10 +135,10 @@ public function optimizeTable(string $tableName)
 		}
 
 		$query = $this->db->query(sprintf($this->optimizeTable, $this->db->escapeIdentifiers($tableName)));
+
 		if ($query !== false)
 		{
-			$query = $query->getResultArray();
-			return current($query);
+			return true;
 		}
 
 		return false;
diff --git a/system/Database/ConnectionInterface.php b/system/Database/ConnectionInterface.php
@@ -126,7 +126,7 @@ public function getVersion(): string;
 	 * @param string $sql
 	 * @param mixed  ...$binds
 	 *
-	 * @return mixed
+	 * @return BaseResult|Query|boolean
 	 */
 	public function query(string $sql, $binds = null);
 
diff --git a/system/Database/Query.php b/system/Database/Query.php
@@ -297,15 +297,27 @@ public function getErrorMessage(): string
 
 	//--------------------------------------------------------------------
 
+	/**
+	 * Determines if the statement is a write-type query or not.
+	 *
+	 * @param string $sql
+	 * @return boolean
+	 */
+	public static function sqlIsWriteType($sql): bool
+	{
+		return (bool) preg_match('/^\s*"?(SET|INSERT|UPDATE|DELETE|REPLACE|CREATE|DROP|TRUNCATE|LOAD|COPY|ALTER|RENAME|GRANT|REVOKE|LOCK|UNLOCK|REINDEX|OPTIMIZE|MERGE)\s/i', $sql);
+	}
+
+	//--------------------------------------------------------------------
+
 	/**
 	 * Determines if the statement is a write-type query or not.
 	 *
 	 * @return boolean
 	 */
 	public function isWriteType(): bool
 	{
-		return (bool) preg_match(
-						'/^\s*"?(SET|INSERT|UPDATE|DELETE|REPLACE|CREATE|DROP|TRUNCATE|LOAD|COPY|ALTER|RENAME|GRANT|REVOKE|LOCK|UNLOCK|REINDEX)\s/i', $this->originalQueryString);
+		return self::sqlIsWriteType($this->originalQueryString);
 	}
 
 	//--------------------------------------------------------------------
diff --git a/system/Database/SQLSRV/Connection.php b/system/Database/SQLSRV/Connection.php
@@ -11,6 +11,7 @@
 
 namespace CodeIgniter\Database\SQLSRV;
 
+use CodeIgniter\Database\Query;
 use CodeIgniter\Database\BaseConnection;
 use CodeIgniter\Database\Exceptions\DatabaseException;
 use Exception;
@@ -511,7 +512,7 @@ public function execute(string $sql)
 	 */
 	public function isWriteType($sql)
 	{
-		return (bool) preg_match('/^\s*"?(SET|INSERT|UPDATE|DELETE|REPLACE|CREATE|DROP|TRUNCATE|LOAD|COPY|ALTER|RENAME|GRANT|REVOKE|LOCK|UNLOCK|REINDEX|MERGE)\s/i', $sql);
+		return Query::sqlIsWriteType($sql);
 	}
 
 	/**
diff --git a/system/Database/SQLite3/Connection.php b/system/Database/SQLite3/Connection.php
@@ -11,6 +11,7 @@
 
 namespace CodeIgniter\Database\SQLite3;
 
+use CodeIgniter\Database\Query;
 use CodeIgniter\Database\BaseConnection;
 use CodeIgniter\Database\Exceptions\DatabaseException;
 use ErrorException;
@@ -497,9 +498,7 @@ protected function _transRollback(): bool
 	 */
 	public function isWriteType($sql): bool
 	{
-		return (bool) preg_match(
-			'/^\s*"?(SET|INSERT|UPDATE|DELETE|REPLACE|CREATE|DROP|TRUNCATE|LOAD|COPY|ALTER|RENAME|GRANT|REVOKE|LOCK|UNLOCK|REINDEX)\s/i',
-			$sql);
+		return Query::sqlIsWriteType($sql);
 	}
 
 	//--------------------------------------------------------------------
diff --git a/system/Model.php b/system/Model.php
@@ -19,6 +19,7 @@
 use CodeIgniter\Database\ConnectionInterface;
 use CodeIgniter\Database\Exceptions\DatabaseException;
 use CodeIgniter\Database\Exceptions\DataException;
+use CodeIgniter\Database\Query;
 use CodeIgniter\Exceptions\ModelException;
 use CodeIgniter\I18n\Time;
 use CodeIgniter\Validation\ValidationInterface;
@@ -253,7 +254,7 @@ protected function doFirst()
 	 *
 	 * @param array $data Data
 	 *
-	 * @return BaseResult|integer|string|false
+	 * @return Query|boolean
 	 */
 	protected function doInsert(array $data)
 	{
@@ -278,7 +279,7 @@ protected function doInsert(array $data)
 		$result = $builder->insert();
 
 		// If insertion succeeded then save the insert ID
-		if ($result->resultID)
+		if ($result)
 		{
 			if (! $this->useAutoIncrement)
 			{
@@ -379,7 +380,7 @@ protected function doUpdateBatch(array $set = null, string $index = null, int $b
 	 * @param integer|string|array|null $id    The rows primary key(s)
 	 * @param boolean                   $purge Allows overriding the soft deletes setting.
 	 *
-	 * @return BaseResult|boolean
+	 * @return string|boolean
 	 *
 	 * @throws DatabaseException
 	 */
diff --git a/system/Test/Mock/MockConnection.php b/system/Test/Mock/MockConnection.php
@@ -48,7 +48,7 @@ public function shouldReturn(string $method, $return)
 	 * @param boolean $setEscapeFlags
 	 * @param string  $queryClass
 	 *
-	 * @return BaseResult|Query|false
+	 * @return BaseResult|Query|boolean
 	 *
 	 * @todo BC set $queryClass default as null in 4.1
 	 */
@@ -81,8 +81,13 @@ public function query(string $sql, $binds = null, bool $setEscapeFlags = true, s
 
 		$query->setDuration($startTime);
 
-		$resultClass = str_replace('Connection', 'Result', get_class($this));
+		// resultID is not false, so it must be successful
+		if (Query::sqlIsWriteType($sql)) {
+			return true;
+		}
 
+		// query is not write-type, so it must be read-type query; return QueryResult
+		$resultClass = str_replace('Connection', 'Result', get_class($this));
 		return new $resultClass($this->connID, $this->resultID);
 	}
 
diff --git a/tests/system/Database/Live/DbUtilsTest.php b/tests/system/Database/Live/DbUtilsTest.php
@@ -115,8 +115,14 @@ public function testUtilsOptimizeTableFalseOptimizeDatabase()
 
 		$this->setPrivateProperty($util, 'optimizeTable', false);
 
-		$this->expectException(DatabaseException::class);
-		$this->expectExceptionMessage('Unsupported feature of the database platform you are using.');
+		if ($this->db->DBDebug) {
+			$this->expectException(DatabaseException::class);
+			$this->expectExceptionMessage('Unsupported feature of the database platform you are using.');
+			$util->optimizeDatabase();
+		} else {
+			$result = $util->optimizeDatabase();
+			$this->assertFalse($result);
+		}
 
 		$util->optimizeDatabase();
 	}
@@ -128,15 +134,8 @@ public function testUtilsOptimizeTable()
 		$util = (new Database())->loadUtils($this->db);
 
 		$d = $util->optimizeTable('db_job');
-
-		if (in_array($this->db->DBDriver, ['SQLite3', 'Postgre', 'SQLSRV']))
-		{
-			$this->assertFalse((bool) $d);
-		}
-		else
-		{
-			$this->assertTrue((bool) $d);
-		}
+		
+		$this->assertTrue((bool) $d);
 	}
 
 	//--------------------------------------------------------------------
diff --git a/tests/system/Models/DeleteModelTest.php b/tests/system/Models/DeleteModelTest.php
@@ -16,7 +16,7 @@ public function testDeleteBasics(): void
 		$this->seeInDatabase('job', ['name' => 'Developer']);
 
 		$result = $this->model->delete(1);
-		$this->assertTrue($result->resultID !== false);
+		$this->assertTrue($result);
 		$this->dontSeeInDatabase('job', ['name' => 'Developer']);
 	}
 
@@ -27,7 +27,7 @@ public function testDeleteFail(): void
 		$this->seeInDatabase('job', ['name' => 'Developer']);
 
 		$result = $this->model->where('name123', 'Developer')->delete();
-		$this->assertFalse($result->resultID);
+		$this->assertFalse($result);
 		$this->seeInDatabase('job', ['name' => 'Developer']);
 	}
 
diff --git a/tests/system/Models/InsertModelTest.php b/tests/system/Models/InsertModelTest.php
@@ -119,7 +119,7 @@ public function testInsertResult(): void
 		$this->createModel(JobModel::class);
 
 		$result = $this->model->protect(false)->insert($data, false);
-		$this->assertTrue($result->resultID !== false);
+		$this->assertTrue($result);
 
 		$lastInsertId = $this->model->getInsertID();
 		$this->seeInDatabase('job', ['id' => $lastInsertId]);
@@ -136,7 +136,7 @@ public function testInsertResultFail(): void
 
 		$this->createModel(JobModel::class);
 		$result = $this->model->protect(false)->insert($data, false);
-		$this->assertFalse($result->resultID);
+		$this->assertFalse($result);
 
 		$lastInsertId = $this->model->getInsertID();
 		$this->assertSame(0, $lastInsertId);

Original file line number	Diff line number	Diff line change
`@@ -357,7 +357,7 @@ abstract protected function doFirst();`
`357`	`357`	`*`
`358`	`358`	`* @param array $data Data`
`359`	`359`	`*`
`360`		`- * @return object\|integer\|string\|false`
	`360`	`+ * @return integer\|string\|boolean`
`361`	`361`	`*/`
`362`	`362`	`abstract protected function doInsert(array $data);`
`363`	`363`
`@@ -407,7 +407,7 @@ abstract protected function doUpdateBatch(array $set = null, string $index = nul`
`407`	`407`	`* @param integer\|string\|array\|null $id The rows primary key(s)`
`408`	`408`	`* @param boolean $purge Allows overriding the soft deletes setting.`
`409`	`409`	`*`
`410`		`- * @return object\|boolean`
	`410`	`+ * @return string\|boolean`
`411`	`411`	`*`
`412`	`412`	`* @throws DatabaseException`
`413`	`413`	`*/`
`@@ -666,11 +666,7 @@ public function save($data): bool`
`666`	`666`	`{`
`667`	`667`	`$response = $this->insert($data, false);`
`668`	`668`
`669`		`- if ($response instanceof BaseResult)`
`670`		`- {`
`671`		`- $response = $response->resultID !== false;`
`672`		`- }`
`673`		`- elseif ($response !== false)`
	`669`	`+ if ($response !== false)`
`674`	`670`	`{`
`675`	`671`	`$response = true;`
`676`	`672`	`}`
`@@ -708,7 +704,7 @@ public function getInsertID()`
`708`	`704`	`* @param array\|object\|null $data Data`
`709`	`705`	`* @param boolean $returnID Whether insert ID should be returned or not.`
`710`	`706`	`*`
`711`		`- * @return BaseResult\|object\|integer\|string\|false`
	`707`	`+ * @return integer\|string\|boolean`
`712`	`708`	`*`
`713`	`709`	`* @throws ReflectionException`
`714`	`710`	`*/`
Original file line number	Diff line number	Diff line change
`@@ -2236,7 +2236,7 @@ public function getCompiledInsert(bool $reset = true)`
`2236`	`2236`	`*`
`2237`	`2237`	`* @throws DatabaseException`
`2238`	`2238`	`*`
`2239`		`- * @return BaseResult\|Query\|false`
	`2239`	`+ * @return Query\|boolean`
`2240`	`2240`	`*/`
`2241`	`2241`	`public function insert(array $set = null, bool $escape = null)`
`2242`	`2242`	`{`
`@@ -2467,7 +2467,7 @@ public function update(array $set = null, $where = null, int $limit = null): boo`
`2467`	`2467`
`2468`	`2468`	`$result = $this->db->query($sql, $this->binds, false);`
`2469`	`2469`
`2470`		`- if ($result->resultID !== false)`
	`2470`	`+ if ($result !== false)`
`2471`	`2471`	`{`
`2472`	`2472`	`// Clear our binds so we don't eat up memory`
`2473`	`2473`	`$this->binds = [];`
`@@ -2816,7 +2816,7 @@ public function getCompiledDelete(bool $reset = true): string`
`2816`	`2816`	`* @param integer $limit The limit clause`
`2817`	`2817`	`* @param boolean $resetData`
`2818`	`2818`	`*`
`2819`		`- * @return mixed`
	`2819`	`+ * @return string\|boolean`
`2820`	`2820`	`* @throws DatabaseException`
`2821`	`2821`	`*/`
`2822`	`2822`	`public function delete($where = '', int $limit = null, bool $resetData = true)`
Original file line number	Diff line number	Diff line change
`@@ -120,7 +120,7 @@ public function databaseExists(string $databaseName): bool`
`120`	`120`	`* Optimize Table`
`121`	`121`	`*`
`122`	`122`	`* @param string $tableName`
`123`		`- * @return mixed`
	`123`	`+ * @return boolean`
`124`	`124`	`* @throws DatabaseException`
`125`	`125`	`*/`
`126`	`126`	`public function optimizeTable(string $tableName)`
`@@ -135,10 +135,10 @@ public function optimizeTable(string $tableName)`
`135`	`135`	`}`
`136`	`136`
`137`	`137`	`$query = $this->db->query(sprintf($this->optimizeTable, $this->db->escapeIdentifiers($tableName)));`
	`138`	`+`
`138`	`139`	`if ($query !== false)`
`139`	`140`	`{`
`140`		`- $query = $query->getResultArray();`
`141`		`- return current($query);`
	`141`	`+ return true;`
`142`	`142`	`}`
`143`	`143`
`144`	`144`	`return false;`
Original file line number	Diff line number	Diff line change
`@@ -126,7 +126,7 @@ public function getVersion(): string;`
`126`	`126`	`* @param string $sql`
`127`	`127`	`* @param mixed ...$binds`
`128`	`128`	`*`
`129`		`- * @return mixed`
	`129`	`+ * @return BaseResult\|Query\|boolean`
`130`	`130`	`*/`
`131`	`131`	`public function query(string $sql, $binds = null);`
`132`	`132`
Original file line number	Diff line number	Diff line change
`@@ -11,6 +11,7 @@`
`11`	`11`
`12`	`12`	`namespace CodeIgniter\Database\SQLSRV;`
`13`	`13`
	`14`	`+use CodeIgniter\Database\Query;`
`14`	`15`	`use CodeIgniter\Database\BaseConnection;`
`15`	`16`	`use CodeIgniter\Database\Exceptions\DatabaseException;`
`16`	`17`	`use Exception;`
`@@ -511,7 +512,7 @@ public function execute(string $sql)`
`511`	`512`	`*/`
`512`	`513`	`public function isWriteType($sql)`
`513`	`514`	`{`
`514`		`- return (bool) preg_match('/^\s*"?(SET\|INSERT\|UPDATE\|DELETE\|REPLACE\|CREATE\|DROP\|TRUNCATE\|LOAD\|COPY\|ALTER\|RENAME\|GRANT\|REVOKE\|LOCK\|UNLOCK\|REINDEX\|MERGE)\s/i', $sql);`
	`515`	`+ return Query::sqlIsWriteType($sql);`
`515`	`516`	`}`
`516`	`517`
`517`	`518`	`/**`
Original file line number	Diff line number	Diff line change
`@@ -19,6 +19,7 @@`
`19`	`19`	`use CodeIgniter\Database\ConnectionInterface;`
`20`	`20`	`use CodeIgniter\Database\Exceptions\DatabaseException;`
`21`	`21`	`use CodeIgniter\Database\Exceptions\DataException;`
	`22`	`+use CodeIgniter\Database\Query;`
`22`	`23`	`use CodeIgniter\Exceptions\ModelException;`
`23`	`24`	`use CodeIgniter\I18n\Time;`
`24`	`25`	`use CodeIgniter\Validation\ValidationInterface;`
`@@ -253,7 +254,7 @@ protected function doFirst()`
`253`	`254`	`*`
`254`	`255`	`* @param array $data Data`
`255`	`256`	`*`
`256`		`- * @return BaseResult\|integer\|string\|false`
	`257`	`+ * @return Query\|boolean`
`257`	`258`	`*/`
`258`	`259`	`protected function doInsert(array $data)`
`259`	`260`	`{`
`@@ -278,7 +279,7 @@ protected function doInsert(array $data)`
`278`	`279`	`$result = $builder->insert();`
`279`	`280`
`280`	`281`	`// If insertion succeeded then save the insert ID`
`281`		`- if ($result->resultID)`
	`282`	`+ if ($result)`
`282`	`283`	`{`
`283`	`284`	`if (! $this->useAutoIncrement)`
`284`	`285`	`{`
`@@ -379,7 +380,7 @@ protected function doUpdateBatch(array $set = null, string $index = null, int $b`
`379`	`380`	`* @param integer\|string\|array\|null $id The rows primary key(s)`
`380`	`381`	`* @param boolean $purge Allows overriding the soft deletes setting.`
`381`	`382`	`*`
`382`		`- * @return BaseResult\|boolean`
	`383`	`+ * @return string\|boolean`
`383`	`384`	`*`
`384`	`385`	`* @throws DatabaseException`
`385`	`386`	`*/`
Original file line number	Diff line number	Diff line change
`@@ -48,7 +48,7 @@ public function shouldReturn(string $method, $return)`
`48`	`48`	`* @param boolean $setEscapeFlags`
`49`	`49`	`* @param string $queryClass`
`50`	`50`	`*`
`51`		`- * @return BaseResult\|Query\|false`
	`51`	`+ * @return BaseResult\|Query\|boolean`
`52`	`52`	`*`
`53`	`53`	`* @todo BC set $queryClass default as null in 4.1`
`54`	`54`	`*/`
`@@ -81,8 +81,13 @@ public function query(string $sql, $binds = null, bool $setEscapeFlags = true, s`
`81`	`81`
`82`	`82`	`$query->setDuration($startTime);`
`83`	`83`
`84`		`- $resultClass = str_replace('Connection', 'Result', get_class($this));`
	`84`	`+ // resultID is not false, so it must be successful`
	`85`	`+ if (Query::sqlIsWriteType($sql)) {`
	`86`	`+ return true;`
	`87`	`+ }`
`85`	`88`
	`89`	`+ // query is not write-type, so it must be read-type query; return QueryResult`
	`90`	`+ $resultClass = str_replace('Connection', 'Result', get_class($this));`
`86`	`91`	`return new $resultClass($this->connID, $this->resultID);`
`87`	`92`	`}`
`88`	`93`