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 https://github.com/codeigniter4/CodeIgniter4/issues/2441#issuecomment-562604358

[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 https://github.com/codeigniter4/CodeIgniter4/pull/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 https://github.com/codeigniter4/CodeIgniter4/pull/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 https://github.com/codeigniter4/CodeIgniter4/pull/5944#issuecomment-1186486644

[kenjis]

Unfortunately the current Entity is designed as a loosely typed Entity. See https://github.com/codeigniter4/CodeIgniter4/issues/2441#issuecomment-562627170

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);
    }
}