CodeIgniter4: Bug: Premature empty check in Model->update function.

Describe the bug Not sure if I’m overseeing something here…

In the Model->update() function several checks for empty data are implemented. The last one happens here: https://github.com/codeigniter4/CodeIgniter4/blob/d63020b30977707dac997b8c3505c7636defea11/system/Model.php#L970-L974 Later on, field protection is done: https://github.com/codeigniter4/CodeIgniter4/blob/d63020b30977707dac997b8c3505c7636defea11/system/Model.php#L984 This can be an issue, if we try to update a database row using an entity ($model->save($entity)) and no changes have been submitted. In this case, the entity object will be cast to array here: https://github.com/codeigniter4/CodeIgniter4/blob/d63020b30977707dac997b8c3505c7636defea11/system/Model.php#L959 Hereby, unchanged fields are removed by the Entity->toRawArray() function. However, some fields will remain in the $data array (e.g., the primary key column and the ‘honeypot’).

Therefore, the last empty check is not triggered as $data is not empty. After doProtectFields, the $data array is empty and consequently triggers an error ('You must use the “set” method to update an entry. ').

CodeIgniter 4 version latest develop

Affected module(s) System\Model

Expected behavior, and steps to reproduce if appropriate see description

Context

Windows 10
Apache 2.4
PHP 7.4

About this issue

Original URL
State: closed
Created 4 years ago
Comments: 28 (17 by maintainers)

Most upvoted comments

@includebeer We merged a PR that will make it more clear now.

@iRedds

Michal says entity and model must be independent. But in fact they are already dependent. The entity is not used anywhere except for receiving / passing data from / to the model.

The model has only built-in support for working with Entities and that’s nothing wrong. Requiring to use Entity exclusively with a model would be a bad idea in my opinion. And yes, that’s true that the framework is using Entity only with a model, but that doesn’t stop you from using it in a different way - it’s a really handy class.

michalsn on Jan 31, 2021

@iRedds I think michals proposal of a possible additional option to restrict attributes to allowedFields sounds reasonable. Personally, I find it quite helpful to be able to add non-database attributes to my entities, so I would just keep that option disabled for me. But having the choice will hand the decision over to the app developers so everyone can get what they need.

sfadschm on Jan 31, 2021

I think hasChanged is working well. The thing is that it will label new keys in the $attributes as compared to $original as changes to the entity and therefore will pass through updates empty checks even if no changes have been made to the original field values. Is that what you mean @iRedds? If so, I think the llinked PR will fix this issue by still allowing us to introduce properties to our entities that are not database fields, so we should be fine.

sfadschm on Jan 31, 2021

Okay, this really is the same behaviour then, I just got a little confused re-reading my own issue report 😆 I will try to summon @michalsn, he was involved in the latest big updates and fixes in the Model (and now also BaseModel).

sfadschm on Jan 31, 2021