You can delete() over and over something already deleted...

this is important in my mind because this means you have to be extremely carefull and make sure your users do not try to softdelete something already softdeleted. so you need to perform strong checks on "top" level (Controllers) because the low level (Eloquent Model) just does not care and runs the deletion over and over.... which actually should be the opposite?

There is like a lack of consistency. When you need to retrieve a softdeleted model, this wont work: \App\Post::find('id'); bceause you need to do this: \App\Post::withTrashed()->find('id'); which is fine. I'm ok with that concept. So why is not there the same behavior/consistency with the delete() and restore() methods? shouldn't it be:

$post->withTrashed()->delete();

which is absurd of course, but would make it so that you cannot just straight softdelete something already softdeleted.

Not sure if I'm clear...

On this you should probably go to the issue section on GitHub and create an issue.

I want to make sure I did not miss something before messing with the big boys on GH.

Look at what I have to code to avoid that :

https://i.imgur.com/k9CsdST.png

I have to check if ->trashed()...

similar ugly stuff within my restored() event observer. which I actually had to get rid off! guess why? because I had to fallback to the restoring() event in order to be able to check on ``->trashed()```

but I want restored(), not restoring() all because the Eloquent Model is absolutely fine with this:

$post->delete();
$post->delete();
$post->delete();
$post->delete();
$post->delete();
$post->delete();
$post->delete();

It's not a bug, based on the source code it's intentional.

When you call ->delete(), it sets a property $exists to false. The delete method checks if $exists is false, if it is, it just returns.

You're over thinking this. Once you delete a Post, you usually return a response, or redirect. If you're using SoftDeletes, then the model isn't going to be in the query, unless you ask it to.

I don't believe there needs to be an exception thrown here. If there was, there would be a method similar to findOrFail like deleteOrFail

You could submit a PR, which includes a deleteOrFail method. It might be a handy addition. But the fact that we're 6 versions in to Laravel and no one has needed this, suggests it may not be merged. You could just create your own deleteOrFail method if you're worried.

you already have the post model in memory. You are not comparing to anything like a real-world scenario.

@jaytee correct, but it still fires the deleting() and deleted() model events regardless !

@snapey maybe. maybe not. this forces to do controls/checks at this app level, while the delete() method should be able to handle it itselft imo.

I'm not sure why it would fire the events. This is the source code. As you can see, if it doesn't exist, then it won't even run the event dispatchers:

/**
     * Delete the model from the database.
     *
     * @return bool|null
     *
     * @throws \Exception
     */
    public function delete()
    {
        if (is_null($this->getKeyName())) {
            throw new Exception('No primary key defined on model.');
        }

        // If the model doesn't exist, there is nothing to delete so we'll just return
        // immediately and not do anything else. Otherwise, we will continue with a
        // deletion process on the model, firing the proper events, and so forth.
        if (! $this->exists) {
            return;
        }

        if ($this->fireModelEvent('deleting') === false) {
            return false;
        }

        // Here, we'll touch the owning models, verifying these timestamps get updated
        // for the models. This will allow any caching to get broken on the parents
        // by the timestamp. Then we will go ahead and delete the model instance.
        $this->touchOwners();

        $this->performDeleteOnModel();

        // Once the model has been deleted, we will fire off the deleted event so that
        // the developers may hook into post-delete operations. We will then return
        // a boolean true as the delete is presumably successful on the database.
        $this->fireModelEvent('deleted', false);

        return true;
    }

Based on the source code also, the only time $exists = true is if it's passed as true explicitly (by the user), or when inserting a new model

Either way, it shouldn't matter. You should only be calling delete once, and then continuing with whatever it is you need to do. All deleted models are excluded from queries by default, and Laravel won't respond to route model binding for models that are deleted, so it's fine.

Look at it another way.

Suppose another process deleted the model. Your code comes along and gets an exception which you did not expect.

In the current behaviour it still goes through the motions even though the model is already deleted. Yes, model events would be fired in this case.

Bear in mind, I am talking about the specific case where you find a model (or route model binding does) and between that the subsequent deletion the model is deleted in another job/controller.

@snapey the part that annoys me the most is the fact that it fires the events any case ...

I had to update all my model events in order to care of that behavior (which should never happen I got you, but if it can happen then it can happen...)

<?php

declare(strict_types=1);

namespace App\Observers;

use App\Tag;

class TaggableObserver
{
    private static $deleted_at = null;

    public function deleting($taggable)
    {
        /** @var \App\Traits\Taggable $taggable */
        $tagsToUpdate = $taggable->tags()->pluck('id');

        if (! method_exists($taggable, 'runSoftDelete')) {
            Tag::whereIn('id', $tagsToUpdate)->decrement('count', 1);
        } else {
            /** @var \Illuminate\Database\Eloquent\SoftDeletes $taggable */
            if ($taggable->isForceDeleting()) {
                if ($taggable->trashed()) {
                    Tag::whereIn('id', $tagsToUpdate)->decrement('trashed_count', 1);
                } else {
                    Tag::whereIn('id', $tagsToUpdate)->decrement('count', 1);
                }
            } else {
                if (! $taggable->trashed()) {
                    Tag::whereIn('id', $tagsToUpdate)->decrement('count', 1);
                    Tag::whereIn('id', $tagsToUpdate)->increment('trashed_count', 1);
                }
            }
        }

        Tag::removeOrphanedTags();
    }

    /*public function deleted($taggable){}*/

    /*public function forceDeleted($taggable){}*/

    public function restoring($taggable)
    {
        static::$deleted_at = $taggable->deleted_at;
    }

    public function restored($taggable)
    {
        if (null !== static::$deleted_at) {
            /** @var \App\Traits\Taggable $taggable */
            $tagsToUpdate = $taggable->tags()->pluck('id');
            Tag::whereIn('id', $tagsToUpdate)->increment('count', 1);
            Tag::whereIn('id', $tagsToUpdate)->decrement('trashed_count', 1);
        }
    }
}