ideas
ideas copied to clipboard
[Proposal] Stop Eloquent from firing update events when restoring a softDeleted model
- Laravel Version: 5.8.26
- PHP Version: v7.3.6-1
Description:
When restoring a soft deleted model the updating and updated events are triggered due to the save function being called by the restore function. This makes it impossible to know whether an update is actually an update, or whether it is just a model being restored (i.e. only changing the deleted_at field to null) without manually checking what has changed.
This issue was touched upon in #1266, but it seems that it was preferable at the time to trigger both events. However, looking back over the issue thread this does not seem to make much sense to me for a number of reasons:
- It does not seem to be consistent with other events. For example, calling the
deletefunction on a model with thesoftDeletetrait also just "updates" thedeleted_atfield, but the update event is not fired. - It goes against the single responsibility principle by having the two update events respond to both updates and restores, especially when we consider that we have dedicated
restoringandrestoredevents. - It makes it difficult to monitor when an update is actually information on the model being updated or when it is simply a model being restored.
My use case to justify why I think it might be a good idea not to call the update events when restoring is imagine you're trying to keep an audit log, or history of changes for a model. You would want to log the changes when the model is updated.
static::updating(function (Meeting $meeting) {
$meeting->recordChanges([
'new_attributes' => array_diff($meeting->getAttributes(), $meeting->getOriginal()),
'old_attributes' => array_diff($meeting->getOriginal(), $meeting->getAttributes())
], auth()->user()->name . " updated meeting " . $meeting->title);
});
Similarly, the create, delete and restore events would log similar things, for example:
static::restoring(function (Meeting $meeting) {
$meeting->recordChanges([
'new_attributes' => $meeting->getOriginal(),
'old_attributes' => []
], auth()->user()->name . " restored meeting " . $meeting->title);
});
In addition to the restore being logged, unless I check which fields have been modified, I will also end up with empty changes in my history, such as:
[
'new_attributes' => [],
'old_attributes' => [],
'description' => 'John Doe updated meeting Quartly Review',
'created_at' => '2019-08-13 20:42:48',
'updated_at' => '2019-08-13 20:42:48'
]
Another way to solve this problem is to add another function to the softDelete trait called isRestoring which allows you check whether a restore is in progress, similarly to how the isForceDeleting function works. But given the reasons above, I think it would be better to change the restore function so that the updating events are not called on a restore.
What are peoples thoughts? If this is something worth doing, I am happy to put a pull request together.
**Moved from laravel/framework/issues/29555 as requested by @driesvints **
Agreed. A restoring event is not an updating event and should not be treated as one. You should be able to restore a model without firing an updating event.
Yes please. A delete, destroy, or restore is not an update IMO.
Also, isRestoring and isForceDeleting seem to be inline with the rest of the standards as well
Having looked at what restore() does, this works for me as an alternative, by explicitly disabling events while restoring, so all you get is the restored event firing.
public function restoreQuietly()
{
$result = self::withoutEvents(
function () {
return $this->restore();
});
$this->fireModelEvent('restored', false);
return $result;
}
+1