owen-it / laravel-auditing

Record the change log from models in Laravel
https://laravel-auditing.com
MIT License
2.94k stars 382 forks source link

Empty array values when auditing relations #919

Closed danielcarney96 closed 3 months ago

danielcarney96 commented 3 months ago
Q A
Bug? yes
New Feature? no
Framework Laravel
Framework version 10.43.0
Package version 13.6.4
PHP version 8.3.4

Actual Behaviour

When attaching a relation via auditAttach, the old value is always an empty array. When detaching via auditDetach, the new value is always an empty array. When using auditSync it depends on the values being changed.

This is occurring when managing belongsToMany relations due to the use of $old->diff and $new->diff in the Auditable trait.

Expected Behaviour

I would expect that the old values contain an array of whatever the state of the relation was before hand. Similarly, the new values should be whatever the new state of that relation is. In the same way that changing a single field will show the field before the change vs after.

For example, if I have a belongToMany relation with 1 existing value already, then I attach another, my audit trail should look like this: old_values: {"users":[{"id": 1}]} new_values: {"users":[{"id": 1}, {"id": 2}]}

currently the output is this: old_values: {"users":[]} new_values: {"users":[{"id": 2}]}

Steps to Reproduce

Attach another relation to a belongstomany relationship, when theres already an existing value.

Possible Solutions

Change the audit dispatching for relations here: https://github.com/owen-it/laravel-auditing/blob/master/src/Auditable.php#L760-L761

To the following:

$this->auditCustomOld[$relationName] = $old->toArray();
$this->auditCustomNew[$relationName] = $new->toArray();

Determining the diff should be done by the developer where needed, and should not be used in the audit trail.

parallels999 commented 3 months ago

Sounds like a expected behaivor, Only what has changed is being audited, if there are no changes there is nothing to review

Imagine this situation

your suggestion old_values: {"users":[{"id": 1},.......,{"id": 1000}]} new_values: {"users":[{"id": 1},.......,{"id": 1000}, {"id": 1001}]}

currently the output is this: old_values: {"users":[]} new_values: {"users":[{"id": 1001}]}

By simple inspection you would know that they added/removed one On your suggestion would be much more difficult to discover what changed.(Also to much duplicated data)

It is also consistent with the operation of all audits.

[
id => 1
value =>2
text => 'test' // if i change this field, only this field is audited, not the entire object with 100 fields
...
field_index_100 => 'value number 100'
]
danielcarney96 commented 3 months ago

i can discover what has changed using ->diff if needed, but that is not always the intention.

the current system makes it difficult to find the state of the relation before the changes were made

parallels999 commented 3 months ago

You could easily override the method. And add the logic that best suits your application You could also add your own custom method, so you could use both if necessary the current system avoid loading too much information that has not actually changed, if you want to see what was added previously you should go back in the audits