-
-
Notifications
You must be signed in to change notification settings - Fork 29
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
Split AbstractActiveRecord::isAttributeChanged()
method
#398
base: master
Are you sure you want to change the base?
Conversation
Tigrov
commented
Jan 24, 2025
Q | A |
---|---|
Is bugfix? | ❌ |
New feature? | ✔️/❌ |
Breaks BC? | ❌ |
Fixed issues | #285, #297 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #398 +/- ##
============================================
+ Coverage 85.18% 85.23% +0.05%
- Complexity 583 587 +4
============================================
Files 17 17
Lines 1404 1409 +5
============================================
+ Hits 1196 1201 +5
Misses 208 208 ☔ View full report in Codecov by Sentry. |
* | ||
* @return bool Whether the property value has been changed. | ||
*/ | ||
public function isPropertyChangedEqual(string $name): bool |
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 name isn't great, and I can't come up with a better one.
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 this method necessary? May be just remove it?
My suggestion for name: isPropertyLooselyChanged
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 think the method can be used sometimes.
Equal
is a common name from the documentation:
https://www.php.net/manual/en/language.operators.comparison.php
Loosely
may cause difficulty in understanding the meaning.
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 think we can rename it later if a better name comes along.
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.
Isn't it !isChanged() && isDirty()
combination?
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.
isDirty()
is the same as isChanged()
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.
Maybe isPropertyNonStrictChanged
?