-
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
fix: Entity::hasChanged() is unreliable for casts #6284
Conversation
eed96bd
to
b604cf3
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.
This makes me very uncomfortable. I think we've already established that Entity
is rather confusing, and my opinion on how to improve that is to separate "data source logic" from "developer logic" and keep Entity methods as a way of traversing those realms.
hasChanged()
is a data source logic method that I interpret as: "does this instance still correctly represent its originating data?" Casting is a developer logic method: "no matter the underlying data type, represent it like this." This works great as a read-only Database Transfer Object (DTO) but the problem comes when we allow modifying attributes (technically breaks the DTO design pattern). Since sweeping changes are off the table, the way to keep this clean is to keep set methods as data source logic methods which means they need to remain ignorant of the developer representation of the data. Altering hasChanged()
to compare developer representations of the underlying data source is mixing the two sides of logic in an unhelpful way.
A silly example... I want to anonymize my user data so I define a new JoeCast
:
public function get(): string
{
return 'Joe';
}
Then I apply it to my user's names:
class User extends Entity
{
protected $casts = [
'firstname' => 'joe',
];
}
Now I can use my developer logic safely anywhere I want to display a user: <?= $user->firstname ?>
. However, this sequence of methods that are entirely data source logic now fails:
$user->firstname = 'Jill';
model(UserModel::class)->save($user);
Addendum, since most of the discussion so far has been about typing (e.g. My guess is this assumption is what is mostly behind the "35 age" example we've been using: since the SQL column data type cannot change we are safe to manipulate types in developer logic. However as soon as the data source becomes collection, array store, NoSQL database, JSON file, etc... that assumption will wreak havoc. |
b604cf3
to
8849e4d
Compare
74bf6bf
to
9c179ad
Compare
9c179ad
to
2136183
Compare
@kenjis What's the update here? |
Just rebased to resolve conflicts. Since Entity is designed to have "raw data" (values retrieved from a database), it is difficult to determine if the value as PHP has changed. Raw data can change depending on the database driver and/or configuration. |
Need to rebase after merging #6285Description
Fixes #5905
hasChanged()
is unreliable for castsfix--> fix: Entity::hasChanged() returns wrong result to mapped property #6285hasChanged()
returns wrong result to mapped propertyChecklist: