Skip to content
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

Closed
wants to merge 3 commits into from

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Jul 21, 2022

Need to rebase after merging #6285

Description
Fixes #5905

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Jul 21, 2022
Copy link
Member

@MGatner MGatner left a 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);

system/Entity/Cast/BaseCast.php Outdated Show resolved Hide resolved
@MGatner
Copy link
Member

MGatner commented Jul 21, 2022

Addendum, since most of the discussion so far has been about typing (e.g. 35 versus '35')... Entity is mildly tied to Model (not BaseModel) in that the hasChanged() methods affect database interactions. Since Model is inherently SQL-based there is an impulse to assume Entity is a representation of SQL data but I would argue that this is a big violation.

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.

@kenjis kenjis marked this pull request as draft July 22, 2022 01:48
@kenjis kenjis force-pushed the fix-Entity-hasChanged branch 2 times, most recently from 74bf6bf to 9c179ad Compare August 31, 2022 09:08
@kenjis kenjis added the stale Pull requests with conflicts label Aug 16, 2023
@kenjis kenjis removed the stale Pull requests with conflicts label Sep 23, 2023
@MGatner
Copy link
Member

MGatner commented Sep 27, 2023

@kenjis What's the update here?

@kenjis
Copy link
Member Author

kenjis commented Sep 27, 2023

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.

@kenjis kenjis closed this Sep 29, 2023
@kenjis kenjis mentioned this pull request Sep 30, 2023
5 tasks
@kenjis
Copy link
Member Author

kenjis commented Sep 30, 2023

@MGatner I sent a brand new PR #7995

@kenjis kenjis deleted the fix-Entity-hasChanged branch September 30, 2023 02:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Entity::hasChanged() is unreliable for casts
3 participants