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

[sfadschm]

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:

CodeIgniter4/system/Model.php

Lines 970 to 974 in d63020b

	// If it's still empty here, means $data is no change or is empty object
	if (empty($data))
	{
	throw DataException::forEmptyDataset('update');
	}

Later on, field protection is done:

CodeIgniter4/system/Model.php

Line 984 in d63020b

$data = $this->doProtectFields($data);

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:

CodeIgniter4/system/Model.php

Line 959 in d63020b

$data = static::classToArray($data, $this->primaryKey, $this->dateFormat);

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

[sfadschm]

Btw. I think this will also happen if only values of forbidden fields (in the sense of 'not in the $allowedFields array'-forbidden) are changed. These will survive the entity being parsed to array, but will be eliminated by field protection, leaving an empty data array.

[sfadschm]

So I figured that in my specific case, the _method and honeypot values are transfered because I simply fill the entity with all posted data. Unsetting these keys before filling fixes this, as $entity->hasChanged() then returns true.

However, I think it would still be useful to empty check $data after field protection as this step might leave the array empty.

Would it make sense to exclude such system keys explicitly in the Entity->fill() function so we don't have to do this manually?

[includebeer]

I have the same problem. In my case I have a simple table with only 3 text fields and an id for the PK. No date field (createdAt, etc). If I submit my form without any changes, the save() function fail with the error 'You must use the "set" method to update an entry'. If I edit at least one value, it works.

[sfadschm]

Try outputting the entity right before calling save() and exit the script with die() thats how I found which keys were filled automatically.

[includebeer]

I print_r() the Entity in the log file, and all the keys are there. But if I don't change anything in the form, it fails. In the print_r() I see the attributes are all filled and are all equal to the original values. There's something wrong in the validation. I think it should update all the fields, regardless if they have change.

[sfadschm]

Like I mentioned above, I think the operation principle itself is good the way it is.
If no data have been changed at all, there is no point in executing a database query (be it from a performance, security or whatever else point of view).

Can you please post the return value of $entity->toRawArray(true, false); right before the save() call? This should print all updated value in the entity.

[includebeer]

$entity->toRawArray prints an array with only one value that is the submit button and is not in the "allowed fields". So it's empty, nothing to update :

( [form_submit] => Sauvegarder )

But if I edit one value, I have that value in this array and the update works:
( [instructions] => bla bla bla [form_submit] => Sauvegarder )

[sfadschm]

Which version of CI4 are you using?

[includebeer]

4.0.4

[sfadschm]

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).

[michalsn]

The original idea about how this is handled was to perform an insert or update only when data for an entity is changed, so this is basically the correct behavior.

Using the $entity->hasChanged() call before performing an update should solve this problem.

Eventually, you can call $model->save($entity->toArray()) - this way you will always perform an update to the database.

If you guys have any ideas for improvements, please share.

[iRedds]

@michalsn I want to tell about my implementation of Entity in CI3.
Two main properties.
$fields - contains the names of all fields of the database table
$immutable - contains the names of the fields in the database table that cannot be changed manually. For example, id or fields with a timestamp.

Only available fields can be filled in the fill() method. $allowed = array_diff ($this->fields, $this->immutable);

That is, it excludes the addition of fields that are not in the database table.
And the hasChanged() method returns the correct value.

[michalsn]

@iRedds Okay, but is there something that is working wrong when using hasChanged() now? If yes, it would be really helpful to see an example/scenario when it happens.

I believe that entity and model should be independent, so we should be able to recognize if some values have changed in the entity without relying on fields defined in the model. The reason behind it is that you don't have to necessarily use a database with an entity class.

[sfadschm]

@iRedds what you mention is actually the second option I thought about to resolve this specific issue.
I am not sure whether entity->fill is currently working as it is intended.

@michalsn I think the PR I just submitted is important regardless of any changes to the entity. The general problem discussed here can occur when trying to model->update() with an entity as well as with a simple array.
Example case would be trying to update a database row with an array like that:

$model->update($id, [
  'thisFieldIsNotInAllowedFieldsArray' => 'randomValue',
]);

However, as a second point I would think that entity->fill should behave like @iRedds described and basically use doProtectFields to decide which keys should be updated in the entity, such that only database fields will be "filled".

[sfadschm]

I just took a look into entity and from how it is implemented, I assume that one is intentionally allowed to add properties to the $entity that are not defined in the database (or $allowedFields for that matter).

As doProtectFields protects us against trying to send such properties to the database, I think the issue addressed in the PR is the only bug here. @iRedds please elaborate if you think otherwise (or maybe we should discuss the behaviour of entity in a different ticket?).

[michalsn]

If there is any general problem with an Entity class it would be better to start a new issue, to make the discussion clear.

But if you believe it's related to this thread, you can as well continue the conversation here.

[iRedds]

Okay, but is there something that is working wrong when using hasChanged() now? If yes, it would be really helpful to see an example/scenario when it happens.

An example in the first post. (#3896 (comment))

[sfadschm]

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.

[michalsn]

@iRedds In that case, I can't bring myself to think of it as a problem. It's desired behavior. But... I think we can talk about an optional Entity property that will define attributes that can be set. If you believe that this would be a good feature feel free to post a PR.

[iRedds]

Is that what you mean @iRedds?

Yes. You right.

[iRedds]

@michalsn Apparently we have different views on the framework.

[sfadschm]

@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.

[michalsn]

@iRedds I never considered different views on anything as an issue - quite opposite. I proposed the idea of how we can achieve what you want. It's up to you now.

Please, keep in mind that we have to think about breaking changes and different use cases here.

[includebeer]

@michalsn ok I will call hasChanged() before I call save(). Thanks for the suggestion.

Maybe save() should automatically call hasChanged() if the data is an instance of Entity? Because as it is now, it’s not clear the problem occurs because there’s nothing to update. What do you think?

[sfadschm]

@michalsn ok I will call hasChanged() before I call save(). Thanks for the suggestion.

Maybe save() should automatically call hasChanged() if the data is an instance of Entity? Because as it is now, it’s not clear the problem occurs because there’s nothing to update. What do you think?

Actually, hasChanged is already called during save, but it will return true, because the attributes array of your entity contains keys that are not present in $original at that point. Please try to cherry-pick the linked PR, this should resolve the issue.

[iRedds]

@sfadschm I did not evaluate Michal's proposal.
I believe that the framework should contain only basic tools and the developer will write what he needs himself. Or there must be a tough algorithm of actions.

Look at Laravel 5.x.
If you want to use the ActiveRecord template use Eloquent. You do not want? Use a model. Tough and understandable.

The community believes that it is necessary to make the framework universal so that it can solve any given task with minimal effort. But it seems to me that there are no good universal tools.

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.

As for your issue.
Yes, I consider this behavior to be abnormal.
But you can write your Entity class and implement your solution to the problem. And even make your own package so that those who wish can use it through the composer.

[iRedds]

OT:
@michalsn Viva la breaking changes.
Worried about breaking changes? Describe them in the documentation and changelog.
This is normal.

i'm gone

[michalsn]

@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.