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 "diff" result for dates and other objects #599

Conversation

befresh-mweimerskirch
Copy link
Contributor

@befresh-mweimerskirch befresh-mweimerskirch commented Jan 12, 2024

Subject

This pull request fixes an issue with the ArrayDiff class and consequentially the AuditReader.

AuditReader::diff currently shows dates (instances of DateTime or DateTimeImmutable) as changed even though they are the same.

This issue was introduced when the loose comparator ("==") was changed to the strict comparison ("===") in commit bf02fd4

This PR reverts the old behaviour for objects, while keeping the strict comparison for other types.

I also added tests. Some of the tests fail without the change, so I hope this illustrates the issue.

I am targeting this branch, because this is a fix that restores previous behaviour (and slightly improves it).

Changelog

### Fixed
- Objects now no longer show as different when their values are the same. This restores some of the old behaviour of the EntityAuditBundle

@befresh-mweimerskirch befresh-mweimerskirch changed the title Bugfix: "diff" returned for dates and other objects Fix "diff" result for dates and other objects Jan 12, 2024
… currently fail, because the strict comparison is used.
This is necessary because the strict comparison operator (===) will return false if the objects are not the same instance.
@befresh-mweimerskirch befresh-mweimerskirch force-pushed the bugfix/fix-diff-for-dates-and-other-objects branch from 1678e06 to 283df18 Compare January 12, 2024 14:39
VincentLanglet
VincentLanglet previously approved these changes Jan 12, 2024
@VincentLanglet VincentLanglet requested review from a team and phansys January 12, 2024 15:25
Copy link
Member

@phansys phansys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @befresh-mweimerskirch.

Could you please update the description telling how the method compareObjects() is different from the loose comparison (==) between objects?

tests/Utils/ArrayDiffTest.php Outdated Show resolved Hide resolved
src/Utils/ArrayDiff.php Show resolved Hide resolved
src/Utils/ArrayDiff.php Outdated Show resolved Hide resolved
{
$diff = new ArrayDiff();

$dateInstance1 = new \DateTimeImmutable('2014-01-01 00:00:00.000000');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, we should also add a test setting different time zones in the compared objects, in order to expose how the comparison behaves with different time zones using the same UTC offset.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add that test case on Monday.

src/Utils/ArrayDiff.php Show resolved Hide resolved
src/Utils/ArrayDiff.php Outdated Show resolved Hide resolved
@VincentLanglet
Copy link
Member

Thank you @befresh-mweimerskirch.

Could you please update the description telling how the method compareObjects() is different from the loose comparison (==) between objects?

If I understood correctly, the compareObject is pretty the same than == for object.
That's just our phpstan-strict-rule which forbid using loose comparison.

Co-authored-by: Javier Spagnoletti <[email protected]>
@befresh-mweimerskirch
Copy link
Contributor Author

@phansys

Could you please update the description telling how the method compareObjects() is different from the loose comparison (==) between objects?

Added to the todo section of the PR. I'll do that on Monday.

@VincentLanglet

If I understood correctly, the compareObject is pretty the same than == for object. That's just our phpstan-strict-rule which forbid using loose comparison.

No. When using the loose comparison operator on objects, the property values are also compared loosely. Imagine a class that has only a property "name". One object with name "TEST" and one object with name "test" as their respective value. Those two instances would be considered equal using the "==" operator.
My method on the other hand uses strict comparison for the values.

@befresh-mweimerskirch
Copy link
Contributor Author

As promised, I added a test case with two dates that have different timezones.
And I described how the method compareObjects() is different from the loose comparison (==) between objects

@VincentLanglet VincentLanglet merged commit 575513b into sonata-project:1.x Jan 22, 2024
22 checks passed
@VincentLanglet
Copy link
Member

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants