Bug: Entity can't handle column named attributes

[iRedds]

CodeIgniter4 Version

4.1.9

What happened?

When using attributes, as the property/attribute name of the Entity class, the Entity::setAttributes() method is called as a setter.
Names collision.

Steps to Reproduce

$e = new CodeIgniter\Entity\Entity();
$e->a = 1;
$e->attributes = [1, 2, 3];  // called the setAttributes method as a setter.
dd($e->toRawArray());

//$e->toRawArray() array (3)
//   0 => integer 1
//   1 => integer 2
//   2 => integer 3

Expected Output

//$e->toRawArray() array (2)
//    a => integer 1
//    attributes => array (3)
//        0 => integer 1
//        1 => integer 2
//        2 => integer 3

Anything else?

In Laravel prior to version 9, the custom setter and getter had the following format:
set/get + AttributeName + Attribute.

public function getFullNameAttribute()
{
    return "{$this->first_name} {$this->last_name}";
}

[the-gt99]

Until the update comes out, I offer my temporary solution. Wrap over "Entity"

<?php


namespace App\Helpers;

use CodeIgniter\Entity\Entity;

class EntityModify extends Entity
{
    private $clearFields = ["attributes","casts","datamap"];

    //Фикс метода fill дабы все не ебнулось, когда в данных пришло что-то из списка ;)
    public function fill(?array $data = null)
    {
        if(is_array($data))
        {
            foreach (array_keys($data) as $key) {
                if (in_array($key, $this->clearFields, true)) {
                    unset($data[$key]);
                }
            }
        }

        return parent::fill($data);
    }

    //Добавляем поля которые не должны быть в объекте описывающем таблицу но очень хочется их видеть в ответе  ;)
    public function addMoreInfo($moreInfo)
    {
        foreach ($moreInfo as $key => $value)
        {
            $this->attributes[$key] = $value;
        }
    }
}

[kenjis]

In Laravel prior to version 9, the custom setter and getter had the following format:
set/get + AttributeName + Attribute.

What happened in Laravel 9?

[lonnieezell]

Damn, I wish we could just rename the setAttributes method. That is by far the simplest solution.

I am wondering if that can be considered a bug, though. If others think so then I say we do that. That is preferable to any additional complexity in the logic that we would have to maintain, and it's the only method that would be affected by this.

[kenjis]

Renaming the setAttributes() method seems to be the best solution.

It seems it is undocumented.
https://codeigniter4.github.io/CodeIgniter4/search.html?q=setAttributes&check_keywords=yes&area=default

[iRedds]

@kenjis In version 9, the getter and setter is the method equivalent to the attribute name. (using php8 functionality)

    protected function firstName(): Attribute
    {
        return Attribute::make(
            get: fn ($value) => ucfirst($value),
            set: fn ($value) => strtolower($value),
        );
    }

There is a possibility that there is an application that has its own Entity implementation with the appropriate setAttributes() method.

I do not consider it correct to rename the setAttributes() method, since the name fully reflects the essence of this method. I also think that the toRawArray() method should be called getAttributes().
Otherwise, any internal method that may be added in the future will need to be adapted instead of a friendly name.

It would be more correct to change the format of the names of setters and getters.

[iRedds]

@the-gt99 Your proposal is a special case.
It will not work if the attributes attribute is required.

[lonnieezell]

It is a much larger breaking change to update the format of the getters and setters and that's not something that could happen right now, I don't think. I'm a little leery of changing the name, honestly, but it's a pretty simple fix for people to change and I think it can legitimately be called a bug.

Naming it something like setEntityAttributes still keeps the meaning clear while fixing the issue. While I agree this is not a perfect fix, and I wouldn't be opposed to updating how it is handled in the future, I think the chances that someone will be trying to set an attribute named entityAttributes is slim enough that this can buy us time until we can do a bigger refactor.

I am very interested to from @MGatner @paulbalandan and @samsonasik before we proceed with any changes though.

[kenjis]

The current setX() and getX() are natural to me.
And as @lonnieezell says, changing them to something like setXAttribute() is a much larger breaking change.

[iRedds]

The problem is that no matter how the setAttributes() method is renamed, there will be a chance that the method name will match the table field.
In any case, it will be BC.
But we need to consider all the consequences.

[kenjis]

See my PR: #5763

[iRedds]

For example, we add the Entity::setRelation() method.
It turns out that now the developer will not be able to use the relation attribute normally.
That is, with this approach, internal methods with names of the format set|get + something will be prohibited. Since this format is used for getters and setters.

[kenjis]

@iRedds Probably I got you want to say.
If we want to add setRelation() and use it to set the relationship of entities,
we can't use it like that. It's too restrictive.

[MGatner]

Removing setAttributes() seems a pretty big breaking change over simply stating in the docs that "attributes" is a reserved Entity name. I agree this should be changed in the future, but because of the collision we can't even deprecate this so it will take any dependent projects from functional to crash.

Personally I can't say specifics right now but I'm pretty sure some of my libraries extend setAttributes().

[kenjis]

@MGatner @iRedds I fond another approach.
See #5781

[iRedds]

I still think that for separate setters (mutators) it is necessary to change the name format. Like Laravel.

[lonnieezell]

I think we will have to table this for the future. If this were a personal/company application I'd say we fix it right now, but it's not, and we have a contract with thousands of developers to not break their apps on a non major release. This rules out changing the naming scheme of the setters, or of the setAttributes method itself. As @MGatner suggested updating the docs to mention any currently reserved attribute/methods names is probably the best available option at the moment.

In the future, changing the naming scheme would be appropriate. The two options on the table at the moment are set{field}Attributes like Laravel, or _set{attribute} like Cake. I'm not sure what other systems use but that would be something to explore.

[MGatner]

I'll also point out that this is precisely how Rails sets all model properties in bulk:

user = User.find params[:user_id]
user.attributes = filtered_params
user.save!

I have no need to mimic Rails but I mention it because I don't think it is wildly problematic to "reserve" attributes and then have it call setAttributes() on assignment.

[kenjis]

@MGatner Do you mean RoR can't use attributes as a column name?

[MGatner]

Correct. It is included in the (extensive) reserved word list.

[kenjis]

I thought that the inability to use attributes was too great a limitation.
but Rails is like that, so we might as well make it a reserved word as well.

I sent a PR to update documentation: #5809

[MGatner]

I'm not opposed to "deeper" solutions but I think this is the most straightforward for now.

[MGatner]

Revisiting... @kenjis updated the docs so attributes is now officially a reserved name. For other properties I propose that we require attribute set/get methods to be public. This will allow developers to implement protected function setRelation() without accidentally triggering on $entity->relation = $value, and is arguably the proper scoping already. It is confusing enough that set/get methods can be private or protected but still be called publicly by the magic methods for me to consider it a bug.

[MGatner]

As for collision of methods that need to be public: I would still like to find a better solution in the future. I tend to name my entity methods without "get/set" because of this, which always feels like an undesirable concession: https://github.com/tattersoftware/codeigniter4-firebase/blob/develop/src/Firestore/Entity.php

[kenjis]

Fixed by #7208