Entity: cast scalar types when setting

[rmilecki]

Description

This way Entity stores its attributes using types as specified in the
$casts. This results in:

1. Cleaner logic: data is always stored as expected. Not only read.
2. Fixing reliability of hasChanged()

Without this change what seemed like setting already set (identical to
read) value could actually change type and result in hasChanged()
unexpectedly returning true.

Ref: #5905
Signed-off-by: Rafał Miłecki [email protected]

[kenjis]

Thank you for sending a PR.

We expect all contributions to conform to our style guide, be commented (inside the PHP source files), be documented (in the user guide), and unit tested (in the test folder).

This PR is not unit tested, and an existing test failed.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#unit-testing

Your PRs need to be GPG-signed before they will be considered.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#signing

[kenjis]

Aside from the lack of testing, this appears to be a major design change and requires careful consideration.

[MGatner]

Entities have gotten very confusing, but: this is a mistake. The stored entity value is supposed to represent its "database state", so when Model converts an entity in objectToArray() it can use Entity::toRawArray deliberately to bypass the casts. If we transform the "raw" attributes during set() to match their cast state we are essentially removing the raw state. This might work fine for some types (like boolean) but won't hold consistent, especially for database strict mode.

On a personal note, I've found Entity very frustrating to work with in a NoSQL context specifically because of these assumptions around their "raw" value states. This might be a PR to re-inspect for version 5 when we think about our database layer refactor and if/how NoSQL support is included.

[MGatner]

Follow-up example to my personal note: https://github.com/tattersoftware/codeigniter4-firebase/blob/develop/src/Firestore/Collection.php#L188

Notice how for my Firebase (NoSQL) driver is explicitly use toArray() instead of toRawArray().

[MGatner]

Closing this as stale. @rmilecki if you need entity attributes stored as casts I recommend making your own library. If you really think this belongs somewhere in the framework then start a forum thread to gauge interest, then it could be added as a new feature (instead of changing the current behavior).

[rmilecki]

Thanks for your comments.

If I can share my opinion, the whole casting in Entity is just totally confusing. If:

1. We don't cast database values
2. We cast user-set values

that is inconsistent and results in hard to understand behaviour.

Maybe there are good reasons for that I simply can't understand (I'm not advanced programmer). Please just note that end users can get confused by that. Also it seems to break Entity::hasChanged() as described in the #5905 . So maybe you can think of some simpler & more reliable alternative solution for further releases.

[MGatner]

I'm sorry for the confusion. I would like to help you work through this so we can get your suggestions on how this could have been more clear. I think you still aren't understanding how Entity casting works, as this is (mostly) untrue:

We cast user-set values

The idea is that the Entity class gives the developer developer-friendly types and the database database-friendly types. With a few exceptions, Entity does not cast user-set values (this was what your PR was adding). From the User Guide:

Casting only affects when values are read. No conversions happen that affect the permanent value in either the entity or the database.

https://codeigniter4.github.io/CodeIgniter4/models/entities.html#property-casting

If there's some confusion it probably comes from some of the Cast names. For example CSV casting is "database: csv_string, developer: array", whereas scalar casts are names for the developer types (Boolean Cast is "developer: boolean"). I'm not sure what can be done to improve that but I think the User Guide describes each cast type pretty well.

[rmilecki]

Thanks @MGatner, I just read everything carefully again and I think I understand it now.

AFAIU: casting is meant to affect reads only making accessing (reading) data easier.

Now I think about it it sounds sane and useful. It should be enough for most (or all) cases. It does what it's supposed to do.

My real problem is unreliable hasChanged() (#5905). While redesigning Entity (e.g. as in this pull request) could be a workaround for it, it's probably not the best choice. I guess this pull request was correctly Closed and we should look for another hasChanged() issue solution instead.

[kenjis]

I think a radical solution requires a redesigned new Entity. But that would be a major undertaking and would destroy compatibility.

[rmilecki]

@kenjis: we could introduce another class though, right?
\CodeIgniter\Entity\StrictEntity
\CodeIgniter\Entity\TypedEntity
\CodeIgniter\Entity\Entity2
or whatever we call it ;)

[kenjis]

@rmilecki Yes.

[lonnieezell]

I'm honestly not fond of having multiple entity classes personally, as it just makes things more confusing when trying to decide which to use. I think we'll need to brainstorm on a good solution for this.

[MGatner]

My opinion... the current Entity is trying to do too many things. It is pretty useful as a "Jack of all trades" but starts to fail when you need depth or complexity. Working with NoSQL it is almost useless, since data types are much more consistent between database and code. Trying to use them as dynamic objects not connected to databases sometimes runs into weird behavior.

I mostly agree with Lonnie that we don't need seven different Entities in the framework - mostly those should be an extension class in personal projects or libraries - but I do think we could get creative for version 5.