Bug: Entity::hasChanged() is unreliable for casts

[rmilecki]

PHP Version

8.0

CodeIgniter4 Version

4.1.9

CodeIgniter4 Installation Method

Manual (zip or tar.gz)

Which operating systems have you tested for this bug?

Linux

Which server did you use?

cgi-fcgi

Database

No response

What happened?

The problem is that \CodeIgniter\Entity::hasChanged() may return true even if there isn't any data change.

This problem is caused by two facts:

1. Casting is performed only when reading properties
2. hasChanged() ignores all casts (it compares $original with $attributes directly, bypassing castAs())

Steps to Reproduce

/* This matches what \CodeIgniter\Database\BaseResult does */
$data = [
    'id' => '1',
    'name' => 'John',
    'age' => '35',
];
$entity = new class ($data) extends \CodeIgniter\Entity {
    protected $casts = [
        'id'   => 'integer',
        'name' => 'string',
        'age'  => 'integer',
    ];
};
$entity->syncOriginal();

var_dump($entity->age);
var_dump($entity->hasChanged());
$entity->age = 35;
var_dump($entity->age);
var_dump($entity->hasChanged());

Expected Output

int(35)
bool(false)
int(35)
bool(false)

Anything else?

Current output:

int(35)
bool(false)
int(35)
bool(true)

[kenjis]

@rmilecki Thank you for reporting.

#5678 indicates that if you use fill(), you need to pass the same typed value from the database.

Do you think this bug can be fixed?

[kenjis]

Ref #2441 (comment)

[kenjis]

\CodeIgniter\Entity is deprecated. Please use CodeIgniter\Entity\Entity class instead.

[kenjis]

Workaround:

<?php

namespace App\Controllers;

class Home extends BaseController
{
    public function index()
    {
        $data = [
            'id' => '1',
            'name' => 'John',
            'age' => '35',
        ];
        $entity = new class ($data) extends \CodeIgniter\Entity\Entity {
            protected $casts = [
                'id'   => 'integer',
                'name' => 'string',
                'age'  => 'integer',
            ];

            protected function setAge($value)
            {
                $this->attributes['age'] = (string) $value;
            }
        };
        $entity->syncOriginal();

        var_dump($entity->age);
        var_dump($entity->hasChanged());
        $entity->age = 35;
        var_dump($entity->age);
        var_dump($entity->hasChanged());
    }
}

[Patricio-Byte-Solution]

Hello how are you.
I think this problem would be solved if the hasChanged() method were rewritten as follows.

    
    public function hasChanged(?string $key = null) : bool
    {
        // If no parameter was given then check all attributes
        if ($key === null) {
            foreach ($this->attributes as $key => $value) {
                // It's a new element
                if (! array_key_exists($key, $this->original)) {
                    return true;
                }

                if ($this->hasChangedWithCast($key)) {
                    return true;
                }
            }
            return false;
        }
    
        // Key doesn't exist in either
        if (! array_key_exists($key, $this->original) && ! array_key_exists($key, $this->attributes)) {
            return false;
        }

        // It's a new element
        if (! array_key_exists($key, $this->original) && array_key_exists($key, $this->attributes)) {
            return true;
        }

        return $this->hasChangedWithCast($key);
    }

    public function hasChangedWithCast(string $key) : bool
    {
        $original   = $this->castAs($this->original[$key], $key, 'get');
        $actual     = $this->castAs($this->attributes[$key], $key, 'get');
        $changed    = false;

        if ($original !== $actual) {
            $changed = true;            
            // At the moment they are not the same.
            // If a property has a Cast defined.
            if (!empty($this->casts[$key])) {
                $type = $this->casts[$key];
                // If it is a numeric Cast
                if (in_array($type, ['int','float','double'])) {
                    // To compare numbers better convert them to strings.
                    if ((string)$original === (string)$actual) {
                        $changed = false;
                    }
                }
            }
        }
        return $changed;
    }

I created a class called BaseEntity that extends from Entity and in it I added this method.

[kenjis]

@Patricio-Byte-Solution Could you send a PR to review your code?

[iRedds]

Instead of adding a mutator to set a value or extending the InetegerCast class, or adding your own class to cast types, you change the logic of the hasChange() method. Why?

[Patricio-Byte-Solution]

Hi @iRedds.
The problem with that is that when an entity is loaded as a result of a database lookup, the convert handlers are not applied.

[iRedds]

I don't understand you.
From the database, you get all the data as a string.
So you need to convert the new value to a string.
As @kenjis showed.

protected function setAge($value)
{
   $this->attributes['age'] = (string) $value;
}

[kenjis]

@iRedds if we cast both values and then compare them, we can solve the problem of the type difference?

[iRedds]

@kenjis This will only work with primitive types.
For example, new A !== new A will return true even if the data (string) is the same.
Or (bool) 'false' === (bool) 'true'

[rmilecki]

    protected $casts = [
        'id'   => 'integer',
        'name' => 'string',
        'age'  => 'integer',
    ];

    protected function setAge($value)
    {
        $this->attributes['age'] = (string) $value;
    }

That would result in having to write a lot of code that seems redundant. If I specify casting in $cast why do I need to handle casting on my own (with setters)?

Would applying specified casts on setting properties break something? That would solve reported problem. Another solution is modifying hasChanged() (or adding its variant) as suggested by @Patricio-Byte-Solution

[rmilecki]

My fix attempt #5944

[kenjis]

@iRedds All values from the database are primitive types.
Entities stores the values from database.
If 'false' should be interpreted false, the CastHander should convert like that.
Am I wrong?

[iRedds]

All values from the database are strings.
I think that in the case of 'false' vs false CastHander, it should process boolean values like pgsql.
https://www.postgresql.org/docs/14/datatype-boolean.html

[rmilecki]

Hey guys, since my attempt #5944 can't be accepted, can you suggest any other solution?

Right now I just can't use CodeIgniter4 for the simplest task: editing database entries (rows).

Maybe I should describe my original problem. My site allows editing existing database entries by displaying their values in a HTML form. User can edit / submit form to update database entry. The problem is that:

1. I shouldn't call $this->db->update() if nothing was edited to avoid useless DB queries
2. If I call update() nevertheless then I'll get DataException "There is no data to update."

I wanted to use hasChanged() to make sure there is actually anything to update however it just doesn't work (as reported in this issue).

[kenjis]

Ref #5944 (comment)

[kenjis]

Unfortunately the current Entity is designed as a loosely typed Entity. See #2441 (comment)

But now year 2022, we need a strictly typed Entity.
The issue is how to implement it.

[kenjis]

I sent a PR #6284
Review, please.

[crustamet]

Entity Class still not works has multiple problems, saving data to the database trough a Model triggers "No data to update" despite hasChanged() returns TRUE

Another thing is now i cannot array_column($object, 'key');
anymore don`t know what is the problem i keep copy paste after update with my Entity Class that was not touched along time ago :(

And maybe fix these issues where you want to update a row in a database, by changing only one value of a column that is mapped ?

Can we reopen this please ?

[kenjis]

@crustamet
This issue is still open.

If you have another issue, please create a new issue with the details.

array_column() does not accept objects.
https://www.php.net/manual/en/function.array-column.php#refsect1-function.array-column-description

[crustamet]

@kenjis array_column() worked before I don`t know why but maybe the magic functions converted into array or something...

[crustamet]

Well i finnaly found why it is not working after making a new project and putting piece by piece a model and another with entities and everything

$model = new Model();
$dbred = $model->findAll();
I found out that when i
array_column($dbred, 'id');
It retrieves everything good.

And then i wanted to have it as an entity
All good array column still works.

But after then i changed a db column lets say id_foreign_key to id_foreign
So i had to make the entity map with the columns so it still works even when the db column change, you just modify the entity and thats it.

So i did that and then my array_column stopped working.

Conclusion. DataMap

    protected $datamap = [
            //property_name => db_column_name
                 'id' 	=> 'id',
    ];

Does something with the object by not allowing the array_column to function as before
This line here
https://github.com/codeigniter4/CodeIgniter4/blob/develop/system/Entity/Entity.php#L520

if i change this to return TRUE instead of FALSE the array_columns works again even with datamap enabled

[crustamet]

@kenjis array_column does not accept objects, but array_column converts the object into an array as it states in the definition function, so even if you pass an object array_column still works even with an object

array_column(array $array, int|string|null $column_key, int|string|null $index_key = null): array

[kenjis]

If you add declare(strict_types=1), would it cause TypeError?

[crustamet]

No it does not

it does not even with declare(strict_types=0)

just the array_column returns empty instead of an array of ids

But please tell me about this line here https://github.com/codeigniter4/CodeIgniter4/blob/develop/system/Entity/Entity.php#L520
why does it returns false I just don't understand the reason

[kenjis]

<?php
declare(strict_types=1);

namespace App\Controllers;

class Home extends BaseController
{
    public function index()
    {
        $obj = (object) ['a' => 1];
        array_column($obj, 'a');
    }
}

TypeError

array_column(): Argument #1 ($array) must be of type array, stdClass given

[kenjis]

@crustamet
If we remove it, EntityTest::testDataMappingIsset() fails.

[crustamet]

@kenjis True but array column does not work on a single object entity
it only works where there is not a singleton element
Try getting items from an model that returns an entity

it does return an array of objects
that return works with array column

try the following

use stdClass;

	$std1 = new stdClass();
	$std1->a = '1';
	
	$std2 = new stdClass();
	$std2->a = '1';
	
	$res = [];
	$res[] = $std1;
	$res[] = $std2;
	
    $RES = array_column($res, 'a');
	print_r($RES);die();

returns Array ( [0] => 1 [1] => 1 )

and from your answer, i do not want to remove DataMappingIsset
does the tests pass if we have the code like this ?

public function __isset(string $key): bool
{
    if ($this->isMappedDbColumn($key)) {
        return true; // THIS IS THE LINE CHANGED
    }

    $key = $this->mapProperty($key);

    $method = 'get' . str_replace(' ', '', ucwords(str_replace(['-', '_'], ' ', $key)));

    if (method_exists($this, $method)) {
        return true;
    }

    return isset($this->attributes[$key]);
}

and sure we can move it down a little bit ?

public function __isset(string $key): bool
{
    $key = $this->mapProperty($key);

    if ($this->isMappedDbColumn($key)) {
        return true; // THIS IS THE LINE CHANGED
    }

    $method = 'get' . str_replace(' ', '', ucwords(str_replace(['-', '_'], ' ', $key)));

    if (method_exists($this, $method)) {
        return true;
    }

    return isset($this->attributes[$key]);
}

i really dont know why it returns false if the column is mapped for sure the attribute is set trough that mapped item or ?
Don't know how to explain better

    if ($this->isMappedDbColumn($key)) {
        return FALSE; // THIS IS THE ORIGINAL LINE
    }

why this is made to return false if the column is mapped does that mean that the attribute of the entity is not set or what ?

[kenjis]

Okay, the following code works as you say.

<?php

declare(strict_types=1);

namespace App\Controllers;

use stdClass;

class Home extends BaseController
{
    public function index()
    {
        $std1    = new stdClass();
        $std1->a = '1';

        $std2    = new stdClass();
        $std2->a = '1';

        $res   = [];
        $res[] = $std1;
        $res[] = $std2;

        $RES = array_column($res, 'a');
        d($RES);
    }
}

[kenjis]

@ crustamet

does the tests pass if we have the code like this ?

Why don't you run tests by yourself?

[kenjis]

@crustamet When isMappedDbColumn() returns true, the name is a mapped db column name,
which is not a property for the Entity. So the following code should return false, not true:

CodeIgniter4/system/Entity/Entity.php

Lines 520 to 522 in fb0583f

	if ($this->isMappedDbColumn($key)) {
	return false;
	}

I don't get your issue.
Please create another issue with how to reproduce your issue, because this issue is about hasChanged().

[crustamet]

@kenjis i understand now, the code is good
I found my problem, and maybe i found the problem for the system

The only case that isMappedDbColumn not working correctly is when you have the datamap mapping for the same key as the key on the attributes

class Test extends Entity
{
	protected $attributes = [
		'ID_Test'		 		=> null,
		'Username' 			=> null,
	];

	protected $datamap = [
		'ID_Test' => 'ID_Test',
	];
}

So array_column and maybe other things not working is when you have the key equal with the datamap

using this

	$arr = [];
	$Test = new Test();
	$Test->ID_Test = 5;
	
	$arr[] = $Test;
	$Test = new Test();
	$Test->ID_Test = 3;
	
	$arr[] = $Test;
	print_r($Test);
	var_dump(isset($Test->ID_Test));die();
	// returns FALSE; even if is set

certainly this was without intention. the only problem is when you have the same key on the datamap, should warn you maybe ?

So even if I set a value for the mapped column isset() still not working because it is a mapped column but this should return TRUE, because the key from the mappedcolumns is equal with the key of the attribute.

Really don't understand how it says is not set after I've just set it ?

[kenjis]

@crustamet I don't understand the meaning of 'ID_Test' => 'ID_Test'.
If the db column name is the property name, you don't need to set in $datamap.
I also don't understand why someone sets like that by mistake.

[crustamet]

Well the thing is i had implemented this entity and the model, and after i had made some db column rename, and i had to remap all the columns, after i did that i renamed again my db columns.

all my entites had datamap so i thought i could let 'ID_Test' => 'ID_Test'
if its not changed for the future in case i will change it to something like
'ID_Test' => 'ID'
or
'ID' => 'ID_Test'

i never knew it will return the attribute as not set.
But seriously the problem is I set the entity and when i check if is set it tells me is not so what is going on here really ?

It was a mistake because i didn`t know how it worked before, for today this is not a mistake and it is on purpose.
It should work even if the key is the same as the db column

If this will not work in the future it must return an error for the datamap that cannot be the same as the db column or something....

[kenjis]

@crustamet It seems to me that setting 'ID_Test' => 'ID_Test' is just a misuse.
If you believe that it is a bug, please create another GitHub Issue, or send a PR to fix it.

[crustamet]

Thanks @kenjis you where of big help. I would make a PR but I have never did one before ^^
and I never did a test with UnitTesting. Probably my next assignment will be this.

[kenjis]

@crustamet If you send a PR, you need to sign your git commits.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#signing

About unit testing, see https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#unit-testing

I recommend that you first send a very small PR, such as just correcting a typo.

[neznaika0]

Why not create a conditional rule: All values for tables must be string/null?
Then it is really to compare any data as strings. There will be something similar when comparing $original and $attributes: (string)35 === '35' No?

[kenjis]

All values for tables must be string/null?

No. It may be int depending on the database configuration.

After all, it seems to me that Entity now does not hold the values it should as PHP, so it cannot make the correct comparisons.

[kenjis]

I sent a PR #7995

[kenjis]

FYI: When you use MySQL, if you set numberNative to true, you can get int/float type value from the database.
See https://codeigniter4.github.io/CodeIgniter4/database/configuration.html#explanation-of-values

[kenjis]

I sent a draft PR #8243