-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix: $cleanValidationRules
does not work in Model updates
#6588
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: $cleanValidationRules
does not work in Model updates
#6588
Conversation
$cleanValidationRules
does not work in Model updates with Entity
$cleanValidationRules
does not work in Model updates with Entity$cleanValidationRules
does not work in Model updates
8108ef9
to
c8cfe1c
Compare
1. Validation in insert()/insertBatch() always uses entire rules (force to set $this->cleanValidationRules = false). 2. Validation in update()/updateBatch()/replace() uses $this->cleanValidationRules value. 3. You can change $this->cleanValidationRules value with cleanRules(). 4. If $this->cleanValidationRules = false, get all data ($onlyChanged = false) from the Entity.
c8cfe1c
to
4010723
Compare
Added the docs. |
This is bigger than I can do right now, but I will come back to it. It looks good, but the situation is difficult to understand. I have mentioned this a few times, but I think our validation has grown over-complicated because it is trying to do too much. |
Yes, this is not small. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made some comments, but just to make sure I have my head around to this correctly... Right now, if I have a model:
protected $validationRules = [
'name' => 'required',
]
This will work:
$model->cleanRules(true)->insert(['color' => 'brown']);
); | ||
} | ||
|
||
public function testUpdateEntityWithCleanValidationRulesFalse() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is redundant and only verified that cleanRules()
sets the property. I recommend removing one or the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is indeed almost the same, but testing two patterns.
$cleanValidationRules = true
andcleanRules()
$cleanValidationRules = false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But my point was: really all you're testing is that cleanRules()
sets the cleanValidationRules
property, so why not either test that or just skip the duplication?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is:
- Model with $cleanValidationRules = true
- Model with $cleanValidationRules = false
How do we remove the duplication?
Co-authored-by: MGatner <[email protected]>
// Model
protected $validationRules = [
'name' => 'required',
]
// Controller
$model->cleanRules(true)->insert(['color' => 'brown']); This does not work, both in before this PR and after this PR. |
$cleanValidationRules removes rules for missing keys.
Right, so in my example the rule for If I've understood this correctly it could be a breaking change with some impactful side effects. |
This seems bad default value, but CodeIgniter4/system/BaseModel.php Line 1314 in ed0cdd0
So this in not a breaking change. |
Ohhh gotcha! Yeah that was the piece I was missing! Thanks for explaining. I'm good with this then. |
Description
Fixes #6577
When you update data, the validation in the model class only validates provided fields.
Model has the property
$this->cleanValidationRules
but it is not able to be used. (Probably a bug)This PR makes that you can use
$this->cleanValidationRules
and you can change it withcleanRules()
at runtime.insert()
/insertBatch()
always uses entire rules (force to set$this->cleanValidationRules = false
).update()
/updateBatch()
/replace()
uses$this->cleanValidationRules
value.$this->cleanValidationRules
value withcleanRules()
.$this->cleanValidationRules = false
, get all data ($onlyChanged = false
) from the Entity.Related: #6459
Checklist: