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

Revert PR #74 and fix getChanges() #75

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

felabrecque
Copy link
Contributor

@felabrecque felabrecque commented Jan 6, 2025

Revert PR #74 since the getDirty() Model method, in the saved event, should return an empty array (the model was saved; attributes are no longer dirty).

I also fixed Model getChanges()

@yormy
getChanges() is what should be used to know which fields were updated.
the returned array contains the attribute name that was updated as the key and the new value as the value.
To get the previous value of those fields, we need to catch them in the saving event first. Then in the saved event, we can use them and compare them to the newly saved value.

Spatie ActivityLog does it this way.
@see https://github.com/spatie/laravel-activitylog/blob/2b3f96c1a5fe3b265197e09cbed813efc96fb6e8/src/Traits/LogsActivity.php#L43

Previously, getChanges contained all the encrypted fields even when none were updated.
(the reason is that the changes attributes was updated AFTER then encryption of the attributes had happened; all the encrypted field values were encrypted and they are different compared to the unencrypted value)

Now, they will only appear if they were updated. Also, their value will be the unencrypted one. Previously, the encrypted value was returned, which is of no use in an event listener.

Fix Model getChanges()
@felabrecque felabrecque changed the title Revert https://github.com/spatie/laravel-ciphersweet/pull/74 Revert PR #74 and fix getChanges() Jan 6, 2025
@felabrecque
Copy link
Contributor Author

Any news on this ? What is missing to merge this PR ?

@freekmurze freekmurze merged commit 8a9ec6a into spatie:main Jan 17, 2025
1 check passed
@freekmurze
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

2 participants