ideas icon indicating copy to clipboard operation
ideas copied to clipboard

[Proposal] Stop Eloquent from firing update events when restoring a softDeleted model

Open pet1330 opened this issue 6 years ago • 4 comments

  • 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:

  1. It does not seem to be consistent with other events. For example, calling the delete function on a model with the softDelete trait also just "updates" the deleted_at field, but the update event is not fired.
  2. 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 restoring and restored events.
  3. 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 **

pet1330 avatar Aug 15 '19 12:08 pet1330

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.

bradietilley avatar Nov 07 '19 00:11 bradietilley

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

JoshuaDoshua avatar Feb 05 '20 19:02 JoshuaDoshua

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;
    }

ninthspace avatar Sep 29 '20 14:09 ninthspace

+1

asolopovas avatar Jul 04 '21 17:07 asolopovas