-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Fix incorrect behaviour when comparing castable attributes in originalIsEquivalent
method
#3439
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: 5.x
Are you sure you want to change the base?
Conversation
Hi @GromNaN, |
src/Eloquent/DocumentModel.php
Outdated
$attribute = $this->castAttribute($key, $attribute); | ||
$original = $this->castAttribute($key, $original); | ||
|
||
if ($attribute === $original) { |
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 comparison should use ===
for scalars and ==
for objects. For example, anything that casts to a BSON object instance (e.g. a UUID stored as a BSON binary) will never pass this check, even if they are equivalent.
There are also some instances of comparisons that would produce the same BSON result (e.g. involving MongoDB\BSON\Int64
, but I think we can ignore those in this check.
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.
answer in next thread
return true; | ||
} | ||
|
||
return serialize($attribute) === serialize($original); |
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.
Why are you checking the result of serialize
here?
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.
because of using ===
$b1 = new Binary(hex2bin('0c103357380648c9a84b867dcb625cfb'), Binary::TYPE_UUID);
$b2 = new Binary(hex2bin('0c103357380648c9a84b867dcb625cfb'), Binary::TYPE_UUID);
$b1 === $b2; //false
serialize($b1) === serialize($b2); //true
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.
Yeah, that's what I suspected. Using serialize
is not the right way to go about this. See my comment above about using ==
when comparing objects. Strictly speaking you should be comparing the BSON representation, but since that's not always easily possible I think a loose object comparison (which will leverage a class' compare_objects
handler) is a safe middle ground.
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.
Sorry, not sure I understood correctly, but why serialization is not right way here, can you explain a bit more, pls.
Example from your previous thread works correct. I guess there is even possible to leave just serialization. This example also working correctly:
$d1 = new Int64(12);
$d2 = new Int64(12);
$d1 === $d2; //false
serialize($d1) === serialize($d2); //true
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.
===
is an identity check, meaning that values have to be identical. For scalar values this works fine, but it breaks down for objects as PHP will compare the internal object IDs as you've shown in the previous example.
==
is an equality check, meaning there is some advanced comparison logic happening for objects. For objects that have a compare_objects
handler implemented (which can only be done for internal classes such as MongoDB\BSON\Binary
, it will call the first handler it finds (first checking the object on the left side of the operation, then that on the right) and return that result. If there is no compare_objects
handler, it falls back to a property equality check for objects of the same class and returns false
when the objects are of different classes.
This equality check is what we want here -- loosely speaking, when two values are equal (ignoring PHP's type coercion shenanigans around numeric strings) we can assume that they will map to the same representation in the database. serialize
is an unnecessary complication at that point.
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.
The theory of differences ==
/===
i know but with all due respect to your work and your knowledge, I cannot agree with you that serialize
is unnecessary complication. What you described much more complicate for me... at first check both values is scalar or no, next compare this values by ===
. If it's not scalar - compare objects ==
. In most cases first compare will work. As I said before the easiest way here - leave just serialization (even remove first if
) which will work as expected for all types. Also comparing objects trough serialization is giving us feature to affect compare behaviour (not sure how it can be used :D).
It's just my thought, you're deciding is it respond your repo paradigm or no 🤝
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.
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.
Pull Request Overview
This PR fixes an issue where attributes with custom casts were incorrectly appearing in the getDirty()
list even when unchanged, due to the originalIsEquivalent
method not properly handling custom cast comparisons.
- Adds serialization-based comparison for class-castable attributes in
originalIsEquivalent
method - Introduces test models (
Options
,OptionsCast
) to demonstrate custom casting behavior - Adds a test case to verify that custom cast attributes are properly tracked for dirty state
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
src/Eloquent/DocumentModel.php |
Fixes the core issue by adding serialization comparison for class-castable attributes |
tests/Models/User.php |
Adds options attribute with custom cast for testing |
tests/Models/OptionsCast.php |
Implements custom cast class for converting between Options object and database format |
tests/Models/Options.php |
Simple value object for testing custom cast behavior |
tests/ModelTest.php |
Adds test case to verify the fix works correctly |
Hello,
Found issue, when I'm using custom cast on attribute and save data in
object
type this attribute every time appeared ingetDirty
list.This behaviour because of ignoring custom casts in
originalIsEquivalent
.