Skip to content

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

Merged
merged 6 commits into from
Oct 3, 2022

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Sep 26, 2022

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 with cleanRules() at runtime.

  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.

Related: #6459

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added bug Verified issues on the current code behavior or pull requests that will fix them database Issues or pull requests that affect the database layer labels Sep 26, 2022
@kenjis kenjis changed the title fix: required_with does not work in Model updates with Entity fix: $cleanValidationRules does not work in Model updates with Entity Sep 26, 2022
@kenjis kenjis changed the title fix: $cleanValidationRules does not work in Model updates with Entity fix: $cleanValidationRules does not work in Model updates Sep 26, 2022
@kenjis kenjis force-pushed the fix-model-cleanValidationRules branch from 8108ef9 to c8cfe1c Compare September 26, 2022 11:20
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.
@kenjis kenjis force-pushed the fix-model-cleanValidationRules branch from c8cfe1c to 4010723 Compare September 29, 2022 03:03
@kenjis
Copy link
Member Author

kenjis commented Sep 29, 2022

Added the docs.

@MGatner
Copy link
Member

MGatner commented Sep 29, 2022

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.

@kenjis
Copy link
Member Author

kenjis commented Sep 29, 2022

Yes, this is not small.
However, I believe this fix is the implementation it should have been originally;
the cleanValidationRules property was provided for that purpose. I can only assume that it is.

Copy link
Member

@MGatner MGatner left a 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()
Copy link
Member

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.

Copy link
Member Author

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.

  1. $cleanValidationRules = true and cleanRules()
  2. $cleanValidationRules = false

Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is:

  1. Model with $cleanValidationRules = true
  2. Model with $cleanValidationRules = false

How do we remove the duplication?

Co-authored-by: MGatner <[email protected]>
@kenjis
Copy link
Member Author

kenjis commented Sep 30, 2022

// 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.
Before: insert() calls $this->cleanRules() inside.
After: insert() sets $this->cleanValidationRules = false inside.

@MGatner
Copy link
Member

MGatner commented Oct 1, 2022

Before: insert() calls $this->cleanRules() inside.

Right, so in my example the rule for name gets cleaned so the insert would succeed.

If I've understood this correctly it could be a breaking change with some impactful side effects.

@kenjis
Copy link
Member Author

kenjis commented Oct 1, 2022

This seems bad default value, but cleanRules() equals cleanRules(false):

public function cleanRules(bool $choice = false)

So this in not a breaking change.

@MGatner
Copy link
Member

MGatner commented Oct 2, 2022

Ohhh gotcha! Yeah that was the piece I was missing! Thanks for explaining. I'm good with this then.

@kenjis kenjis merged commit 1de9173 into codeigniter4:develop Oct 3, 2022
@kenjis kenjis deleted the fix-model-cleanValidationRules branch October 3, 2022 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them database Issues or pull requests that affect the database layer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: required_with validation rule works in insert new record mode but not in update mode
2 participants