active-record icon indicating copy to clipboard operation
active-record copied to clipboard

Add `hasChanges` to AR

Open Auramel opened this issue 6 years ago • 6 comments

Hi! I have idea - add method to \yii\db\ActiveRecord. I think it conveniently.

/**
 * @return bool
 */
public function hasChanges(): bool
{
    return sizeof($this->getDirtyAttributes()) > 0;
}

And, maybe, rename dirtyAttributes() to changes() or changedAttributes()?

Auramel avatar May 30 '18 10:05 Auramel

Sounds good to me.

samdark avatar May 30 '18 16:05 samdark

I would like to remind you that AR is is not a final class but only a base for user models - every new method reserves some name and may make naming things harder for the end user. I'm pretty sure that many people already use hasChanges() for they own purpose and many others would like to do this in future.

rob006 avatar Jun 02 '18 20:06 rob006

I would rather avoid to change naming logic, and keep "dirty"'s one.

I would go for BaseActiveRecord::isDirty().

However BaseActiveRecord::isAttributeChanged() should be BaseActiveRecord::isAttributeDirty() to be consistent with markAttributeDirty() and getDirtyAttributes().

CedricYii avatar Jun 08 '18 15:06 CedricYii

@rob006 and @CedricYii both have a point.

samdark avatar Jun 12 '18 06:06 samdark

Yep, I'm also for consistent naming and in that case add isDirty won't be a big problem as it's not so common to use isDirty method name in your models. So I'm for renaming isAttributeChanged to isAttributeDirty and adding isDirty. (or it could be vice versa, renaming everything to changed and changes)

If the proposal ok, I can make the PR.

armpogart avatar Jul 07 '18 14:07 armpogart

@armpogart please do. Dirty sounds better because @rob006 arguments.

samdark avatar Jul 07 '18 21:07 samdark