Fix loss of escape value and data in the model

codeigniter4 · Jan 10, 2021 · 3bcb5a4 · 3bcb5a4
1 parent fad7376
commit 3bcb5a4
Show file tree

Hide file tree

Showing 5 changed files with 169 additions and 74 deletions.
diff --git a/system/BaseModel.php b/system/BaseModel.php
@@ -20,6 +20,7 @@
 use CodeIgniter\Pager\Pager;
 use CodeIgniter\Validation\ValidationInterface;
 use Config\Services;
+use InvalidArgumentException;
 use ReflectionClass;
 use ReflectionException;
 use ReflectionProperty;
@@ -715,31 +716,7 @@ public function insert($data = null, bool $returnID = true)
 	{
 		$this->insertID = 0;
 
-		if (empty($data))
-		{
-			throw DataException::forEmptyDataset('insert');
-		}
-
-		// If $data is using a custom class with public or protected
-		// properties representing the collection elements, we need to grab
-		// them as an array.
-		if (is_object($data) && ! $data instanceof stdClass)
-		{
-			$data = $this->objectToArray($data, false, true);
-		}
-
-		// If it's still a stdClass, go ahead and convert to
-		// an array so doProtectFields and other model methods
-		// don't have to do special checks.
-		if (is_object($data))
-		{
-			$data = (array) $data;
-		}
-
-		if (empty($data))
-		{
-			throw DataException::forEmptyDataset('insert');
-		}
+		$data = $this->transformDataToArray($data, 'insert');
 
 		// Validate data before saving.
 		if (! $this->skipValidation && ! $this->cleanRules()->validate($data))
@@ -877,32 +854,7 @@ public function update($id = null, $data = null): bool
 			$id = [$id];
 		}
 
-		if (empty($data))
-		{
-			throw DataException::forEmptyDataset('update');
-		}
-
-		// If $data is using a custom class with public or protected
-		// properties representing the collection elements, we need to grab
-		// them as an array.
-		if (is_object($data) && ! $data instanceof stdClass)
-		{
-			$data = $this->objectToArray($data, true, true);
-		}
-
-		// If it's still a stdClass, go ahead and convert to
-		// an array so doProtectFields and other model methods
-		// don't have to do special checks.
-		if (is_object($data))
-		{
-			$data = (array) $data;
-		}
-
-		// If it's still empty here, means $data is no change or is empty object
-		if (empty($data))
-		{
-			throw DataException::forEmptyDataset('update');
-		}
+		$data = $this->transformDataToArray($data, 'update');
 
 		// Validate data before saving.
 		if (! $this->skipValidation && ! $this->cleanRules(true)->validate($data))
@@ -1692,6 +1644,55 @@ protected function objectToRawArray($data, bool $onlyChanged = true, bool $recur
 		return $properties;
 	}
 
+	/**
+	 * Transform data to array
+	 *
+	 * @param array|object|null $data Data
+	 * @param string            $type Type of data (insert|update)
+	 *
+	 * @return array
+	 *
+	 * @throws DataException
+	 * @throws InvalidArgumentException
+	 * @throws ReflectionException
+	 */
+	protected function transformDataToArray($data, string $type): array
+	{
+		if (! in_array($type, ['insert', 'update'], true))
+		{
+			throw new InvalidArgumentException(sprintf('Invalid type "%s" used upon transforming data to array.', $type));
+		}
+
+		if (empty($data))
+		{
+			throw DataException::forEmptyDataset($type);
+		}
+
+		// If $data is using a custom class with public or protected
+		// properties representing the collection elements, we need to grab
+		// them as an array.
+		if (is_object($data) && ! $data instanceof stdClass)
+		{
+			$data = $this->objectToArray($data, true, true);
+		}
+
+		// If it's still a stdClass, go ahead and convert to
+		// an array so doProtectFields and other model methods
+		// don't have to do special checks.
+		if (is_object($data))
+		{
+			$data = (array) $data;
+		}
+
+		// If it's still empty here, means $data is no change or is empty object
+		if (empty($data))
+		{
+			throw DataException::forEmptyDataset($type);
+		}
+
+		return $data;
+	}
+
 	// endregion
 
 	// region Magic

diff --git a/system/Model.php b/system/Model.php
@@ -85,11 +85,12 @@ class Model extends BaseModel
 	protected $tempData = [];
 
 	/**
-	 * Escape Parameter to be passed in do methods
+	 * Escape array that maps usage of escape
+	 * flag for every parameter.
 	 *
-	 * @var boolean|null
+	 * @var array
 	 */
-	protected $escape = null;
+	protected $escape = [];
 
 	// endregion
 
@@ -257,7 +258,7 @@ protected function doFirst()
 	protected function doInsert(array $data)
 	{
 		$escape       = $this->escape;
-		$this->escape = null;
+		$this->escape = [];
 
 		// Require non empty primaryKey when
 		// not using auto-increment feature
@@ -266,10 +267,15 @@ protected function doInsert(array $data)
 			throw DataException::forEmptyPrimaryKey('insert');
 		}
 
-		// Must use the set() method to ensure objects get converted to arrays
-		$result = $this->builder()
-			->set($data, '', $escape)
-			->insert();
+		$builder = $this->builder();
+
+		// Must use the set() method to ensure to set the correct escape flag
+		foreach ($data as $key => $val)
+		{
+			$builder->set($key, $val, $escape[$key] ?? null);
+		}
+
+		$result = $builder->insert();
 
 		// If insertion succeeded then save the insert ID
 		if ($result->resultID)
@@ -329,7 +335,7 @@ protected function doInsertBatch(?array $set = null, ?bool $escape = null, int $
 	protected function doUpdate($id = null, $data = null): bool
 	{
 		$escape       = $this->escape;
-		$this->escape = null;
+		$this->escape = [];
 
 		$builder = $this->builder();
 
@@ -338,10 +344,13 @@ protected function doUpdate($id = null, $data = null): bool
 			$builder = $builder->whereIn($this->table . '.' . $this->primaryKey, $id);
 		}
 
-		// Must use the set() method to ensure objects get converted to arrays
-		return $builder
-			->set($data, '', $escape)
-			->update();
+		// Must use the set() method to ensure to set the correct escape flag
+		foreach ($data as $key => $val)
+		{
+			$builder->set($key, $val, $escape[$key] ?? null);
+		}
+
+		return $builder->update();
 	}
 
 	/**
@@ -629,8 +638,12 @@ public function set($key, ?string $value = '', ?bool $escape = null)
 	{
 		$data = is_array($key) ? $key : [$key => $value];
 
-		$this->tempData['escape'] = $escape;
-		$this->tempData['data']   = array_merge($this->tempData['data'] ?? [], $data);
+		foreach ($data as $k => $v)
+		{
+			$this->tempData['escape'][$k] = $escape;
+		}
+
+		$this->tempData['data'] = array_merge($this->tempData['data'] ?? [], $data);
 
 		return $this;
 	}
@@ -673,13 +686,22 @@ protected function shouldUpdate($data) : bool
 	 */
 	public function insert($data = null, bool $returnID = true)
 	{
-		if (empty($data))
+		if (! empty($this->tempData['data']))
 		{
-			$data           = $this->tempData['data'] ?? null;
-			$this->escape   = $this->tempData['escape'] ?? null;
-			$this->tempData = [];
+			if (empty($data))
+			{
+				$data = $this->tempData['data'] ?? null;
+			}
+			else
+			{
+				$data = $this->transformDataToArray($data, 'insert');
+				$data = array_merge($this->tempData['data'], $data);
+			}
 		}
 
+		$this->escape   = $this->tempData['escape'] ?? [];
+		$this->tempData = [];
+
 		return parent::insert($data, $returnID);
 	}
 
@@ -696,13 +718,22 @@ public function insert($data = null, bool $returnID = true)
 	 */
 	public function update($id = null, $data = null): bool
 	{
-		if (empty($data))
+		if (! empty($this->tempData['data']))
 		{
-			$data           = $this->tempData['data'] ?? null;
-			$this->escape   = $this->tempData['escape'] ?? null;
-			$this->tempData = [];
+			if (empty($data))
+			{
+				$data = $this->tempData['data'] ?? null;
+			}
+			else
+			{
+				$data = $this->transformDataToArray($data, 'update');
+				$data = array_merge($this->tempData['data'], $data);
+			}
 		}
 
+		$this->escape   = $this->tempData['escape'] ?? [];
+		$this->tempData = [];
+
 		return parent::update($id, $data);
 	}
 

diff --git a/tests/system/Models/InsertModelTest.php b/tests/system/Models/InsertModelTest.php
@@ -218,4 +218,25 @@ public function testUseAutoIncrementSetToFalseInsert(): void
 		$this->assertSame($insert['key'], $this->model->getInsertID());
 		$this->seeInDatabase('without_auto_increment', $insert);
 	}
+
+	/**
+	 * @see https://github.com/codeigniter4/CodeIgniter4/issues/4087
+	 */
+	public function testInsertWithSetAndEscape(): void
+	{
+		$userData = [
+			'name' => 'Scott',
+		];
+
+		$this->createModel(UserModel::class);
+
+		$this->setPrivateProperty($this->model, 'useTimestamps', true);
+
+		$this->model->set('country', '1+1', false)->set('email', '2+2')->insert($userData);
+
+		$this->assertGreaterThan(0, $this->model->getInsertID());
+
+		$result = $this->model->where('name', 'Scott')->where('country', 2)->where('email', '2+2')->first();
+		$this->assertCloseEnough(time(), strtotime($result->created_at));
+	}
 }
diff --git a/tests/system/Models/MiscellaneousModelTest.php b/tests/system/Models/MiscellaneousModelTest.php
@@ -2,6 +2,8 @@
 
 namespace CodeIgniter\Models;
 
+use CodeIgniter\Database\Exceptions\DataException;
+use InvalidArgumentException;
 use Tests\Support\Models\EntityModel;
 use Tests\Support\Models\JobModel;
 use Tests\Support\Models\SimpleEntity;
@@ -81,4 +83,24 @@ public function testGetValidationMessagesForReplace(): void
 		$error = $this->model->errors();
 		$this->assertTrue(isset($error['description']));
 	}
+
+	public function testUndefinedTypeInTransformDataToArray(): void
+	{
+		$this->expectException(InvalidArgumentException::class);
+		$this->expectExceptionMessage('Invalid type "whatever" used upon transforming data to array.');
+
+		$this->createModel(JobModel::class);
+		$method = $this->getPrivateMethodInvoker($this->model, 'transformDataToArray');
+		$method([], 'whatever');
+	}
+
+	public function testEmptyDataInTransformDataToArray(): void
+	{
+		$this->expectException(DataException::class);
+		$this->expectExceptionMessage('There is no data to insert.');
+
+		$this->createModel(JobModel::class);
+		$method = $this->getPrivateMethodInvoker($this->model, 'transformDataToArray');
+		$method([], 'insert');
+	}
 }
diff --git a/tests/system/Models/UpdateModelTest.php b/tests/system/Models/UpdateModelTest.php
@@ -286,4 +286,24 @@ public function testUseAutoIncrementSetToFalseUpdate(): void
 		$this->createModel(WithoutAutoIncrementModel::class)->update($key, $update);
 		$this->seeInDatabase('without_auto_increment', ['key' => $key, 'value' => $update['value']]);
 	}
+
+	/**
+	 * @see https://github.com/codeigniter4/CodeIgniter4/issues/4087
+	 */
+	public function testUpdateWithSetAndEscape(): void
+	{
+		$userData = [
+			'name' => 'Scott',
+		];
+
+		$this->createModel(UserModel::class);
+
+		$this->assertTrue($this->model->set('country', '2+2', false)->set('email', '1+1')->update(1, $userData));
+
+		$this->seeInDatabase('user', [
+			'name' => 'Scott',
+			'country' => 4,
+			'email' => '1+1',
+		]);
+	}
 }