Skip to content

Commit 1de9173

Browse files
kenjiskenjis
authored
Merge pull request #6588 from kenjis/fix-model-cleanValidationRules
fix: `$cleanValidationRules` does not work in Model updates
2 parents 86d0f3d + d83baa6 commit 1de9173

File tree

3 files changed

+170
-12
lines changed

3 files changed

+170
-12
lines changed

‎system/BaseModel.php

Lines changed: 32 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -189,7 +189,7 @@ abstract class BaseModel
189189

190190
/**
191191
* Whether rules should be removed that do not exist
192-
* in the passed in data. Used between inserts/updates.
192+
* in the passed data. Used in updates.
193193
*
194194
* @var bool
195195
*/
@@ -701,13 +701,23 @@ public function insert($data = null, bool $returnID = true)
701701
{
702702
$this->insertID = 0;
703703

704+
// Set $cleanValidationRules to false temporary.
705+
$cleanValidationRules = $this->cleanValidationRules;
706+
$this->cleanValidationRules = false;
707+
704708
$data = $this->transformDataToArray($data, 'insert');
705709

706710
// Validate data before saving.
707-
if (! $this->skipValidation && ! $this->cleanRules()->validate($data)) {
711+
if (! $this->skipValidation && ! $this->validate($data)) {
712+
// Restore $cleanValidationRules
713+
$this->cleanValidationRules = $cleanValidationRules;
714+
708715
return false;
709716
}
710717

718+
// Restore $cleanValidationRules
719+
$this->cleanValidationRules = $cleanValidationRules;
720+
711721
// Must be called first so we don't
712722
// strip out created_at values.
713723
$data = $this->doProtectFields($data);
@@ -773,6 +783,10 @@ public function insert($data = null, bool $returnID = true)
773783
*/
774784
public function insertBatch(?array $set = null, ?bool $escape = null, int $batchSize = 100, bool $testing = false)
775785
{
786+
// Set $cleanValidationRules to false temporary.
787+
$cleanValidationRules = $this->cleanValidationRules;
788+
$this->cleanValidationRules = false;
789+
776790
if (is_array($set)) {
777791
foreach ($set as &$row) {
778792
// If $data is using a custom class with public or protected
@@ -789,8 +803,11 @@ public function insertBatch(?array $set = null, ?bool $escape = null, int $batch
789803
$row = (array) $row;
790804
}
791805

792-
// Validate every row..
793-
if (! $this->skipValidation && ! $this->cleanRules()->validate($row)) {
806+
// Validate every row.
807+
if (! $this->skipValidation && ! $this->validate($row)) {
808+
// Restore $cleanValidationRules
809+
$this->cleanValidationRules = $cleanValidationRules;
810+
794811
return false;
795812
}
796813

@@ -811,6 +828,9 @@ public function insertBatch(?array $set = null, ?bool $escape = null, int $batch
811828
}
812829
}
813830

831+
// Restore $cleanValidationRules
832+
$this->cleanValidationRules = $cleanValidationRules;
833+
814834
return $this->doInsertBatch($set, $escape, $batchSize, $testing);
815835
}
816836

@@ -832,7 +852,7 @@ public function update($id = null, $data = null): bool
832852
$data = $this->transformDataToArray($data, 'update');
833853

834854
// Validate data before saving.
835-
if (! $this->skipValidation && ! $this->cleanRules(true)->validate($data)) {
855+
if (! $this->skipValidation && ! $this->validate($data)) {
836856
return false;
837857
}
838858

@@ -906,7 +926,7 @@ public function updateBatch(?array $set = null, ?string $index = null, int $batc
906926
}
907927

908928
// Validate data before saving.
909-
if (! $this->skipValidation && ! $this->cleanRules(true)->validate($row)) {
929+
if (! $this->skipValidation && ! $this->validate($row)) {
910930
return false;
911931
}
912932

@@ -1027,7 +1047,7 @@ public function onlyDeleted()
10271047
public function replace(?array $data = null, bool $returnSQL = false)
10281048
{
10291049
// Validate data before saving.
1030-
if ($data && ! $this->skipValidation && ! $this->cleanRules(true)->validate($data)) {
1050+
if ($data && ! $this->skipValidation && ! $this->validate($data)) {
10311051
return false;
10321052
}
10331053

@@ -1581,7 +1601,11 @@ protected function transformDataToArray($data, string $type): array
15811601
// properties representing the collection elements, we need to grab
15821602
// them as an array.
15831603
if (is_object($data) && ! $data instanceof stdClass) {
1584-
$data = $this->objectToArray($data, ($type === 'update'), true);
1604+
// If it validates with entire rules, all fields are needed.
1605+
$onlyChanged = ($this->skipValidation === false && $this->cleanValidationRules === false)
1606+
? false : ($type === 'update');
1607+
1608+
$data = $this->objectToArray($data, $onlyChanged, true);
15851609
}
15861610

15871611
// If it's still a stdClass, go ahead and convert to

‎tests/system/Models/ValidationModelTest.php

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,11 @@
1212
namespace CodeIgniter\Models;
1313

1414
use CodeIgniter\Config\Factories;
15+
use CodeIgniter\Model;
1516
use Config\Validation;
1617
use stdClass;
1718
use Tests\Support\Models\JobModel;
19+
use Tests\Support\Models\SimpleEntity;
1820
use Tests\Support\Models\ValidErrorsModel;
1921
use Tests\Support\Models\ValidModel;
2022

@@ -367,4 +369,118 @@ public function testGetValidationMessages(): void
367369
$error = $this->model->getValidationMessages();
368370
$this->assertSame('Description field is required.', $error['description']);
369371
}
372+
373+
/**
374+
* @see https://github.com/codeigniter4/CodeIgniter4/issues/6577
375+
*/
376+
public function testUpdateEntityWithPropertyCleanValidationRulesTrueAndCallingCleanRulesFalse()
377+
{
378+
$model = new class () extends Model {
379+
protected $table = 'test';
380+
protected $allowedFields = ['field1', 'field2', 'field3', 'field4'];
381+
protected $returnType = SimpleEntity::class;
382+
protected $validationRules = [
383+
'field1' => 'required_with[field2,field3,field4]',
384+
'field2' => 'permit_empty',
385+
'field3' => 'permit_empty',
386+
'field4' => 'permit_empty',
387+
];
388+
};
389+
390+
// Simulate to get the entity from the database.
391+
$entity = new SimpleEntity();
392+
$entity->setAttributes([
393+
'id' => '1',
394+
'field1' => 'value1',
395+
'field2' => 'value2',
396+
'field3' => '',
397+
'field4' => '',
398+
]);
399+
400+
// Change field1 value.
401+
$entity->field1 = '';
402+
403+
// Set $cleanValidationRules to false by cleanRules()
404+
$model->cleanRules(false)->save($entity);
405+
406+
$errors = $model->errors();
407+
$this->assertCount(1, $errors);
408+
$this->assertSame(
409+
$errors['field1'],
410+
'The field1 field is required when field2,field3,field4 is present.'
411+
);
412+
}
413+
414+
public function testUpdateEntityWithPropertyCleanValidationRulesFalse()
415+
{
416+
$model = new class () extends Model {
417+
protected $table = 'test';
418+
protected $allowedFields = ['field1', 'field2', 'field3', 'field4'];
419+
protected $returnType = SimpleEntity::class;
420+
protected $validationRules = [
421+
'field1' => 'required_with[field2,field3,field4]',
422+
'field2' => 'permit_empty',
423+
'field3' => 'permit_empty',
424+
'field4' => 'permit_empty',
425+
];
426+
427+
// Set to false.
428+
protected $cleanValidationRules = false;
429+
};
430+
431+
// Simulate to get the entity from the database.
432+
$entity = new SimpleEntity();
433+
$entity->setAttributes([
434+
'id' => '1',
435+
'field1' => 'value1',
436+
'field2' => 'value2',
437+
'field3' => '',
438+
'field4' => '',
439+
]);
440+
441+
// Change field1 value.
442+
$entity->field1 = '';
443+
444+
$model->save($entity);
445+
446+
$errors = $model->errors();
447+
$this->assertCount(1, $errors);
448+
$this->assertSame(
449+
$errors['field1'],
450+
'The field1 field is required when field2,field3,field4 is present.'
451+
);
452+
}
453+
454+
public function testInsertEntityValidateEntireRules()
455+
{
456+
$model = new class () extends Model {
457+
protected $table = 'test';
458+
protected $allowedFields = ['field1', 'field2', 'field3', 'field4'];
459+
protected $returnType = SimpleEntity::class;
460+
protected $validationRules = [
461+
'field1' => 'required',
462+
'field2' => 'required',
463+
'field3' => 'permit_empty',
464+
'field4' => 'permit_empty',
465+
];
466+
};
467+
468+
$entity = new SimpleEntity();
469+
$entity->setAttributes([
470+
'field1' => 'value1',
471+
// field2 is missing
472+
'field3' => '',
473+
'field4' => '',
474+
]);
475+
476+
// Insert ignores $cleanValidationRules value.
477+
$model->insert($entity);
478+
479+
$errors = $model->errors();
480+
$this->assertCount(1, $errors);
481+
$this->assertSame(
482+
$errors['field2'],
483+
'The field2 field is required.'
484+
);
485+
}
370486
}

‎user_guide_src/source/models/model.rst

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -191,10 +191,25 @@ $skipValidation
191191
---------------
192192

193193
Whether validation should be skipped during all **inserts** and **updates**. The default
194-
value is false, meaning that data will always attempt to be validated. This is
194+
value is ``false``, meaning that data will always attempt to be validated. This is
195195
primarily used by the ``skipValidation()`` method, but may be changed to ``true`` so
196196
this model will never validate.
197197

198+
.. _clean-validation-rules:
199+
200+
$cleanValidationRules
201+
---------------------
202+
203+
Whether validation rules should be removed that do not exist in the passed data.
204+
This is used in **updates**.
205+
The default value is ``true``, meaning that validation rules for the fields
206+
that are not present in the passed data will be (temporarily) removed before the validation.
207+
This is to avoid validation errors when updating only some fields.
208+
209+
You can also change the value by the ``cleanRules()`` method.
210+
211+
.. note:: Prior to v4.2.7, ``$cleanValidationRules`` did not work due to a bug.
212+
198213
$beforeInsert
199214
-------------
200215
$afterInsert
@@ -392,9 +407,12 @@ For many people, validating data in the model is the preferred way to ensure the
392407
standard, without duplicating code. The Model class provides a way to automatically have all data validated
393408
prior to saving to the database with the ``insert()``, ``update()``, or ``save()`` methods.
394409

395-
.. important:: When you update data, the validation in the model class only validate provided fields.
396-
So when you set the rule ``required``, if you don't pass the required field data,
397-
the validation won't fail. This is to avoid validation errors when updating only some fields.
410+
.. important:: When you update data, by default, the validation in the model class only
411+
validates provided fields. This is to avoid validation errors when updating only some fields.
412+
413+
But this means ``required*`` rules do not work as expected when updating.
414+
If you want to check required fields, you can change the behavior by configuration.
415+
See :ref:`clean-validation-rules` for details.
398416

399417
The first step is to fill out the ``$validationRules`` class property with the fields and rules that should
400418
be applied. If you have custom error message that you want to use, place them in the ``$validationMessages`` array:

0 commit comments

Comments
 (0)