-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Entity rework #7995
Entity rework #7995
Conversation
a37b9d5
to
9995e42
Compare
c5576d9
to
90f84ae
Compare
$entity->fill(['username' => 'johnsmith', 'active' => false, 'memo' => ['foo', 'bar']]); | ||
$model->save($entity); | ||
|
||
$user = $model->asObject(get_class($entity))->find(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@samsonasik Probably get_class($entity)
causes the Rector error:
[ERROR] Could not process "tests/system/Entity/EntityLiveTest.php" file, due to:
"assert(count($exprType) === 1)". On line: 170
Do you know something?
a349acd
to
404e749
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fear this is leaning into the issue from last time. Most of the handling is correct but I would argue it is in the wrong place. We keep tying our data representations to the actual database driver. This does move the responsibility off of Entity (somewhat) but now bakes those assumptions into Cast
.
What about NoSQL databases? in-memory mocks? entities bound for API endpoints? Why should they all be bound to MySQL's limitations?
Single responsibility: let Entity hold the data and represent the object, let Cast convert between different data types and act as a gate for bad data, let Model send associative arrays of data back and forth from storage.
Since Model is the only class that actually cares about the infrastructure: it should handle any necessary data conversions. The problem is that BaseModel/Model is already a mess, and we've pushed most of our driver-specific implementation into the Query Builders.
Do you mean Model should have logic for CI's QueryBuilder has the feature to use Custom Result Objects: |
@kenjis I'm afraid I don't have good answers, only concerns. I believe the current architecture is bad, and the mess of our code between entity-model-builder shows for it. Every time an issue or feature comes up and we commit more to this architecture I get worried we are digging ourselves in deeper. But the reality is that we can't afford a complete refactor right now, nor could we even release it (except maybe as a standalone). For the issue at hand: if we can find a way to let Model (SQL-specific) handle to/from database I think that's the best place for the new code to reside, architecturally speaking. |
From now on, $attributes have PHP values, not raw values from database.
The previous code no longer makes sense.
Should not pass null to Cash handlers.
But I don't know it makes sense.
[ERROR] Could not process "tests/system/Entity/EntityLiveTest.php" file, due to: "assert(count($exprType) === 1)". On line: 170
Description
Fixes #5905
Supersedes #6284
Entity::$attributes
have (cast) PHP values, not raw values from databaseCastInterface::toDatabase()
andCastInterface::fromDatabase()
Entity::toDatabase()
to return array for database andEntity::fromDatabase()
to rebuild Entity from database dataChecklist: