refactor: Rework Entity class

[neznaika0]

Description
I keep track of entity changes. At the moment, I don't see any conflicts when using 'attributes' as the column name.
There was also a bug with the restore of $_cast.

Question: Do I need to get rid of the DataCaster object? For example, I completely disabled casting in the Entity and transform it into a Model. But the object is always being created.

I could, under the condition $_cast = false, not create it or destroy it when calling $this->cast(false). We can simplify by calling getDataCaster() "on the fly", although it's no better to create an object every time. Suggestions?

It is also planned to update the DateCast to use the interface, since it does not always set DateTime, but also DateTimeImmutable.

PR should be taken after #9877 But we can discuss it now.

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value (without duplication)
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[neznaika0]

An actual note? We can already have the 'attributes' field

https://github.com/neznaika0/codeigniter-dev/blob/9ac23b4c207c6bea12b4ebb1a085d3ab57942baf/user_guide_src/source/models/entities.rst?plain=1#L43

[michalsn]

Some initial comments added.

If you fix a bug, it should be mentioned in the changelog as well.

[neznaika0]

If you fix a bug, it should be mentioned in the changelog as well.

I've lost the changes. I'll fix it

[neznaika0]

• Now we rely on a flag and an empty array. It would be wrong if any of these values are true and the DataCaster is created.
• The guide has been updated. The question remains refactor: Rework Entity class #9878 (review)

[neznaika0]

Additionally:

• I would return the array check and fix fill() -null is never needed there.

    public function __construct(array $data = [])
    {
       // ...

        $this->syncOriginal()->fill($data);
    }

    public function fill(array $data)
    {
        // ...
    }

[michalsn]

• The question remains refactor: Rework Entity class #9878 (review)

I think we could already with _setAttributes(). Since we dropped setAttributes(), I see no problem with removing this note.

I would return the array check and fix fill() -null is never needed there.

Users are allowed to create the Entity and fill it later. This change could break people's apps.

[neznaika0]

Users are allowed to create the Entity and fill it later. This change could break people's apps.

How? Does the default constructor have an empty array, and fill() without an array doesn't matter?

[michalsn]

How? Does the default constructor have an empty array, and fill() without an array doesn't matter?

This is a BC break, which has no real benefit.

[neznaika0]

Good. Let's leave it. But we're breaking in 4.7. In my opinion, using null is the wrong developer code. php8.5 has already been released, and we still support the old standards. Of course, this is a concern for the developers...

[michalsn]

Good. Let's leave it. But we're breaking in 4.7. In my opinion, using null is the wrong developer code. php8.5 has already been released, and we still support the old standards. Of course, this is a concern for the developers...

We do allow BC breaks, but only when they provide clear value or affect minimally exposed internals. This change does neither and would introduce unnecessary risk for users.

That said, I'm not stubborn... if other maintainers think that this is the right direction, then I won't be opposed.

[michalsn]

Other than this, looks good to me. Thanks.

[michalsn]

Final round from me, cleanup only.

[neznaika0]

I have another suggestion:

1. Do we need the feature to restrict access if $datamap is configured?
   What for? We get double access to one field. It is more correct to work with class properties (if $datamap is specified).

2. Do I need to restrict the creation of dynamic fields\properties? Now, at any given time, we can have a different number of properties, so the object is "incomplete". I suggest introducing key checks in $attributes and explicitly specifying fields when creating an object.

I haven't fully checked the tests, these are my thoughts...

public function testCIDatamap(): void
{
    $entity = new class () extends Entity
        {
            protected $attributes = [
                'id'         => null,
                'full_name'  => null, // In the $attributes, the key is the db column name
                'email'      => null,
             ];
            protected $datamap = [
                // property_name => db_column_name
                'name' => 'full_name',
            ];
            
        // For __get() similar checks
        public function __set(string $key, $value = null)
        {
            $dbColumn = $this->mapProperty($key);

           // feat 2
            if (! $this->hasAttribute($dbColumn)) {
                throw new DomainException(
                    sprintf('Entity: The attribute "%s" was not found.', esc($dbColumn))
                );
            }

            // feat 1
            $mappedProperty = $key === $dbColumn ? array_search($key, $this->datamap, true) : false;

            if ($mappedProperty !== false) {
                throw new DomainException(
                     sprintf('Entity: Access to the "%s" property is possible via "%s".', esc($dbColumn), esc($key))
                );
            }

            parent::__set($key, $value);
        }
        };

    // Set via DB field
    // Now it work, after that it should cause an error (1)
    $entity->full_name = 'Ivan S.';
    
    // Set via property class
    // It works in both cases
    $entity->name = 'Petr М.';
    $entity->email = '[email protected]';
    
     // Now it work, after that it should cause an error (2)
    $entity->undefined = 'Some string.';
}

[michalsn]

I don't think this qualifies as refactoring. While the change itself may be reasonable, it introduces a significant BC break.

1. The current behavior is documented, and users may rely on it. Also, what about getting values back to DB fields?
2. This is a complete behavior change - at the moment, users are not required to define attributes.

[neznaika0]

It is more difficult to discuss on the forum, but there are no discussions in the repository. We have slack, but it's not available to me. I'm sorry to write here.

You're right, this is a "feature request". The methods for DB are not affected at first glance:

        $entity = new class ([]) extends Entity {
            protected $attributes = [
                'name'    => null,
                'role'    => 'user',
                'active'  => true,
                'profile' => null,
            ];
            protected $datamap = [
                'Name'   => 'name',
                'Role'   => 'role',
                'Active' => 'active',
            ];
        };

        $profile = new \stdClass();
        $profile->city = 'Moscow';

        // Access via field names from the DB, errors
        // $entity->name    = 'Ivan';
        // $entity->role    = 'admin';
        // $entity->active  = false;

        $entity->Name    = 'Ivan';
        $entity->Role    = 'admin';
        $entity->Active  = false;
        $entity->profile = $profile;
        self::assertSame(
            [
                'profile' => $profile,
                'Name'    => 'Ivan',
                'Role'    => 'admin',
                'Active'  => false,
            ],
            $entity->toArray()
        );
        self::assertSame(
            [
                'name'    => 'Ivan',
                'role'    => 'admin',
                'active'  => false,
                'profile' => $profile,
            ],
            $entity->toRawArray()
        );
        self::assertTrue($entity->hasChanged());

[michalsn]

Fair enough, but as I mentioned, that's only half of the problem.

While this would be a nice feature, BC is a priority in this case.