Skip to content
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

Model Changes w.r.t. #1773 #1930

Merged
merged 1 commit into from
Apr 11, 2019
Merged

Model Changes w.r.t. #1773 #1930

merged 1 commit into from
Apr 11, 2019

Conversation

atishhamte
Copy link
Contributor

In the stated issue #1773, he is trying to save the null value in the database which expected to throw an error of required field.
But when data is passed from the insert method to classToArray, it expects to return only changed values.
The previous and current value of the property is null, hence it is not returning the property as changed value.
Hence error is not getting triggered.

So some required changes made here like,

  1. Made allowed null for description column of job table in migration
  2. Removed if condition from save() method as again update() or save() method performs same operation in the classToArray() method.
  3. Introduced $onlyChanged parameter to classToArray() function. So we can escape the only changed value check in case of insert the data but it will return only changed values data in case of the update as the flag value is true by default.
  4. Passing false value to classToArray method for variable $onlyChanged in the insert method as we required all the params, not the changed values only.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • Conforms to style guide

@lonnieezell
Copy link
Member

Much better solution. I'm a little leery about removing the classToArray at the beginning of save() but think it should be fine. Thanks.

@lonnieezell lonnieezell merged commit c51dea3 into codeigniter4:develop Apr 11, 2019
@MGatner
Copy link
Member

MGatner commented May 7, 2019

Point number 4 has introduced a new problem. When a database field is not null and the field on an entity is not set, classToArray will still return the null value which tries to get inserted into the database, throwing an error. IMO save() should never receive all values from classToArray and the solution to #1773 should be fixed in validate() instead.

@MGatner
Copy link
Member

MGatner commented May 7, 2019

After a bit more research I do believe this is an issue and would have failed tests except that the test UserModel.php supplies default values to its fields, a non-mandatory (and probably non-standard) choice:
https://github.com/codeigniter4/CodeIgniter4/blob/develop/tests/_support/Models/UserModel.php

I will open a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants