-
Notifications
You must be signed in to change notification settings - Fork 2k
fix: Fixed Entity::injectRawData()
#9748
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 4.7
Are you sure you want to change the base?
Conversation
dce5359
to
faca2d0
Compare
Seems like a BC break. It's a developer's responsibility to assign properties that will not break the entity. The differences between the arrays' methods are clear: https://codeigniter.com/user_guide/models/entities.html#bulk-accessing-properties - and when each of them is used is pretty obvious to me, but if you think this could use more clarification, then please send a PR. |
probably add optional flag?, eg: public function injectRawData(array $data, bool $mergeToExistingAttributes = false) |
Okay. If we find a solution, I'll move it to 4.7. @michalsn , description for methods. No specification for "_user _name _property". They're skipping. CodeIgniter4/system/Entity/Entity.php Line 184 in 015e3df
@samsonasik my opinion is a system error. Does not require a flag. Case:
It is important to understand: After the appearance of DataCaster, entities cannot be a simple reflection of a row from the database. Therefore, I allow the possibility of having "virtual" properties, which may not be stored in the database or may be filled in later. I found out in michalsn/codeigniter-nested-model For example, in relationships OneToOne, OneToMany... Having a DataCaster, the object's synchronization is almost the same - Do we want to go back to saving scalar types in the original (copy for the database)? Then we need to review the entities: synchronization, has changed.. I hope I got the meaning right. |
I think there might be a bit of a misunderstanding. The fact that we have model casting now doesn’t really change how entities are used (except that we shouldn't mix entity casting and model casting at the same time). You're still free to define your own properties - just avoid using the attributes property, since it's reserved for data representing database columns (in the context of entities used with the database). We can never really know how creative other developers have been, and a change like this (even if it seems minor) could impact their work. That said, I'm not stubborn about it. If most people think it's a good idea, I'm happy to go along with it. @samsonasik's idea is a good one, but it wouldn't change anything in the OP's case, since the entity from the database is seeded with |
I'm already exhausted. Everything works, but every time something special appears. Every time I want to drop the code... I have already used private properties in a custom entity. Some behavior needs to be updated and the get/set magic is no longer needed. This is a more stable option. Therefore, I work manually with the attributes. Do we need an interface to abstract the class? To completely rewrite the entity. |
faca2d0
to
7222141
Compare
Description
After a DB query, the entity may break if the result does not contain all the "fields == properties".
Example, do we need a "virtual"
$entity->user
property, which stores by default?User
. It may not be necessary to insert it into the database, but it's important in other work.There are other questions:
Entity::toArray()
but allowed inEntity::toRawArray()
I would add this to the current PR. Answering questions may require correction if we find abnormal behavior.
Checklist: