Laravel-json-api: Issue with SoftDeletes not firing delete events

[JeanLucEsser]

Hi there,

So, in order to use and store a specific date in the deleted_at column, you're not using a delete on the record, which would natively put the current date and time, but a save.

This has a downfall as model events deleting and deleted won't be fired by Laravel.

I changed the way you handle a delete in SoftDeletesModels to run a true delete followed by a save:

    /**
     * @param Model $record
     */
    protected function saveOrRestore(Model $record)
    {
        if ($this->willRestore($record)) {
            $record->restore();
        } else {
            if ($this->willDelete($record)) {
                $key = $this->getSoftDeleteKey($record);
                // save the original date so we can put it back later on
                $deletedAt = $record{$key};
                // delete on the record so that deleting and deleted events get fired
                $record->delete();
                // apply the original soft deleting date back before saving
                $record{$key} = $deletedAt;
            }
            $record->save();
        }
    }

    // ...

    /**
     * @param Model $record
     * @return bool
     */
    protected function willDelete(Model $record)
    {
        if (!$key = $this->getSoftDeleteKey($record)) {
            return false;
        }

        return null === $record->getOriginal($key) && $record->trashed();
    }

But this has other issues:

• first we make two calls to the db (delete then save)
• second let's say I want to delete a record while not using an adapter, I'm stuck with the current date and time decided by Laravel, same if I want to implement or use a soft delete cascade solution

So I came to the conclusion we're left with 2 solutions to this problem:

• accept only true as an option to the deleted-at field and let the framework always put in the current date and time (far from ideal)
• overwrite the runSoftDelete method in the Laravel SoftDeletes trait to use the value in the deleted_at column if provided

This last solution is the cleanest and would be my favorite, I'm just not sure how or where to implement it best. What are your thoughts on all this?

[lindyhopchris]

Thanks for raising this.

Re this:

second let's say I want to delete a record while not using an adapter, I'm stuck with the current date and time decided by Laravel, same if I want to implement or use a soft delete cascade solution

This is not a problem, because if the model is not deleted via the API it is appropriate that the server (i.e. Laravel) chooses the date/time.

For your two solutions...

accept only true as an option to the deleted-at field and let the framework always put in the current date and time (far from ideal)

This is breaking, plus I don't like that it forces an application-specific choice on the developer... each API should be able to choose how to implement this in their resource, rather than forcing a decision.

overwrite the runSoftDelete method in the Laravel SoftDeletes trait to use the value in the deleted_at column if provided

This isn't a viable solution, because it's not really for this package to override how Laravel works.

Note sure about this, but perhaps the solution is to accept that we need to run both delete() and save()... i.e. not work around the fact that Laravel gives us no choice but to do that. Then if the client has used a date for the delete value, we can fill that after the delete but before the save... i.e. overwrite the server deleted_at date with the date that is provided by the client.

[JeanLucEsser]

If I’m not mistaken, your proposed solution is precisely what I changed in your SoftDeletesModels. I think it’s the best approach, i don’t see another one.

Overwriting Laravel’s own SoftDelete to handle cascade for exemple should depend on each application implementation and not your library, I agree with you on that.

If what I did in the code above suits you I can make a PR. We could rename the method saveOrRestoreOrDelete to be more precise on what it does.

[lindyhopchris]

Ah yes, it is the same as what you've done! So that's good we're both thinking along the same lines!

A PR would be great. I think leave the name of the method as saveOrRestore() because otherwise it will be a breaking change as people could have overloaded the method.

[lindyhopchris]

@JeanLucEsser no need for a PR, I've just pushed the fix for this.

[JeanLucEsser]

Thank you, I’m so sorry i was about to do it but I had to deal with an urgent matter before. Glad you found the time. You rock.

[lindyhopchris]

@JeanLucEsser no problem at all, just had a spare moment and as we had effectively written the solution above it was pretty easy to implement. Not sure when I'll tag the next release but should be in the next week or two. Feel free to tie to the develop branch until I do.