Subject should only return soft deleted models if that model uses SoftDeletes

[bluec]

We are logging activity of several models, some using SoftDeletes trait and others not.

When we set config option 'subject_returns_soft_deleted_models' => true then calling subject() on any model not using SoftDeletes trait throws an exception: Call to undefined method Illuminate\Database\Eloquent\Builder::withTrashed().

Can the subject() method below be made to check first whether the Model uses the SoftDeletes trait? Or perhaps check whether withTrashed() is a defined method on the Model? I tried a few things but couldn't figure out how to get the class of the Model..

https://github.com/spatie/laravel-activitylog/blob/68eb6e65382f94fe86ce5ff6572159f80d024ba9/src/Models/Activity.php#L28-L35

[Gummibeer]

The official way to get the class would be to resolve the morph_type via the morph-map. But this wouldn't be a solution if you use the relation method in a query-chain like:

Activity::whereHas('subject', function($query){ $query->where('active', true); })->get();

The only solution I could think about would be a simple try-catch block.

[Gummibeer]

@bluec or @MustafaHussaini can you make the added and linked unittest fail? Could be that I don't get the described issue - but I can't reproduce it in phpunit. If you can't let the test fail can you please attach your full code (Model with SoftDelete, Model without SoftDelete and code that produces the error)!? full = the important parts - I don't need any unrelated mutator methods or things like this. And your setup would also be good: Laravel-Version, Package-Version, PHP-Version and which Database and in which version?

https://github.com/spatie/laravel-activitylog/blob/fb19f4956e3df6715278106fb42a72139324bc62/tests/ActivityModelTest.php#L146-L179

[bluec]

Hi @Gummibeer you described the behaviour exactly here: https://github.com/spatie/laravel-activitylog/issues/445#issuecomment-430893149

Otherwise the config value true and not available scope method (commented trait use) should throw an exception - unknown method call.

[bluec]

@Gummibeer I have done a PR https://github.com/spatie/laravel-activitylog/pull/459 on your test to make it fail. You need to call ->with('subject')

[Gummibeer]

Thanks for your PR! I will work on this. But that's even more strange. Because if I just do $activity->subject it works. So even in Laravel/Eloquent Core it works different ways depending on how I load the relation. 😕 I can't promise any solution(-date) but will try to find the issue and a solution.

[bluec]

Thanks for looking into it. I've tried a ton of things including a simple try/catch block but I can't get anything to work.

[Gummibeer]

Ok, the problem why everything we do in the subject() relation method doesn't change anything is that it doesn't produce the error. The scope is re-applied in \Illuminate\Database\Eloquent\Relations\MorphTo::replayMacros() which uses \Illuminate\Support\Traits\ForwardsCalls to call the method. The exception isn't thrown during the initial call of withTrashed() but in the forwarded call which is done during get(). So just a try-catch around the query itself could catch the exception.

And this is why the initial call doesn't fail but pipe the method call \Illuminate\Database\Eloquent\Relations\MorphTo::__call: https://github.com/illuminate/database/blob/44411c7288fc7b7d4e5680cfcdaa46d348b5c981/Eloquent/Relations/MorphTo.php#L292-L299

I will do more research to find a way how we could prevent this.

[Gummibeer]

Ok, the real problem is that it's called during another query. I got a fix that works if the activity model is an instance with attributes:

public function subject(): MorphTo
{
    $morph = $this->morphTo();
    if (config('activitylog.subject_returns_soft_deleted_models')) {
        $query = $morph->createModelByType($this->{$morph->getMorphType()})->newQuery();

        if(!is_null($query->getMacro('withTrashed'))) {
            return $morph->withTrashed();
        }
    }

    return $morph;
}

But if the relation is loaded via with() or load() the attributes aren't filled and it's not possible to resolve the target/subject model.

So for now I really don't see a way how this could be fixed! 😖 I have some ideas but these aren't solution to be done in this package:

• add a withTrashed() scope that doesn't do anything
• don't eager load with trashed and load deleted subjects in single queries
• split your activity collection into two (one with subjects that support withTrashed() and one that don't) and enable/disable withTrashed() via a static property

My code above could be a good starting point for some of these solutions.

[bluec]

Thanks for this. I didn't think there was an easy fix :(

I initially added withTrashed() scopes that don't do anything but I feel this taints the application. I am going to consider the other options including just not using SoftDeletes at all. I recall reading that @freekmurze is not a fan of them and I think he may have a point!

[Gummibeer]

I'm also not a fan of them. No of my projects uses them for anything. Entries are deleted or not - keeping them for backup purposes isn't the job of the application/model. If you use soft-deletes to show/hide or activate/deactivate entries use corresponding columns. So it looks nice on the first sight but I don't see a reason to use soft-deletes.

[Mina-R-Meshriky]

if I understand the problem correctly then why don't you do it like below

 public function subject(): MorphTo
 {
      if ( method_exists($this->morphTo()->getRelated(),'runSoftDelete') 
         && config('activitylog.subject_returns_soft_deleted_models')) {
         return $this->morphTo()->withTrashed();
    }

return $this->morphTo();
 }

[Gummibeer]

@Mina-R-Meshriky thanks for your idea, I haven't checked it but so far I see it this would trigger two queries. One just to determine if it should use soft-deletes or not.

At all there is no issue if you do $activity->subject but if you eager-load them during the activity query itself Activity::forSubject($user)->with('subject')->get().

I've tried to describe it above - the code way is:

1. Activity::... creates a query instance ActivityQuery
2. with('subject') creates a ForwardsCalls instance which caches the withTrashed() macro call
3. the ActivityQuery is executed and got a result of n activities
4. foreach activity it creates a new query to get the subject SubjectQuery
5. the cached method calls from ForwardsCalls are applied to SubjectQuery
6. Exception

There is no way to know in step 2 if any of the unknown subjects of the unknown activities uses soft-deletes or not. The only solution would be to find a way to cache a method which exists (created by this package) and this method decides if to apply soft-deletes or not. With an ActivitySoftDeletePolyfill trait and a applyWithTrashedIfNeeded() method we could solve this. But every subject model has to use this trait and I don't think that this should happen in this project. It's no problem with this package itself - it's a common laravel one. If you mix soft-deletes in a polymorphic table and use withTrashed() in the relationship method.

Because we allow to override all needed methods and to disable soft-deletes via config I don't see a reason to mess up with deep core logic just to allow mixed-soft-deletes-subjects.

If I'm wrong and your example doesn't issue a new database request and it solves the issue I would be happy to apply your suggestion!?

[Mina-R-Meshriky]

when I dd($this->morphTo()) I found that the parent and the related models were already loaded in the collection.

[Gummibeer]

Have you done it after you got your activity instance or during the activity query? This is the major difference. If I have an $activity instance everything works fine and you even don't get an exception if you call withTrashed() on a subject that doesn't use soft-deletes.

[spatie-bot]

Dear contributor,

because this issue seems to be inactive for quite some time now, I've automatically closed it. If you feel this issue deserves some attention from my human colleagues feel free to reopen it.

[tomfortmuller]

I've run into this issue and it looks like a fix hasn't been found yet. Could you check for a deleted_at column on the model?

    public function subject(): MorphTo
    {
        if (config('activitylog.subject_returns_soft_deleted_models') && $this->deleted_at) {
            return $this->morphTo()->withTrashed();
        }

        return $this->morphTo();
    }

[Gummibeer]

hey @tomfortmuller ,

the columns doesn't have to exist on $this but the morph relation. And exactly this is the major problem. That we have to decide when to apply or not the scope without knowing which model is resolved in the relation. One morph relation can have multiple models and each one can have a different setting. So lets say that the subject can be the user and car. The user implements SoftDelete but the car doesn't. In this case we had to apply the withTrashed() scope to the user rows but not to the car ones. The only way to really fix it would be a fix in the trait itself. For example via a condition. But I don't know if SQL conditions are supported by all laravel supported drivers.

[LeonAlvarez]

I run into same issue with a similar use case what I did was

appliesTo' => function ($q) {
     $q->withoutGlobalScope(SoftDeletingScope::class);
}

If you want to do it on the relation load

public function appliesTo(): MorphTo
{
      return $this->morphTo('applies_to', 'subject_type', 'subject_id', 'id')
          ->withoutGlobalScope(SoftDeletingScope::class);
}

[MichMich]

This still seems to be an issue. Any progress on this?

[Gummibeer]

@MichMich So far I know the underlying issue isn't solved. So as long as no one finds a crazy hacky solution this won't change. 🤔

[Jamesking56]

Can we not just use a try/catch here to fix this? Seems like the simplest solution to me

[Gummibeer]

Not really as the relation is/could be mixed. The real problem here is that we have to define the withTrashed() scope on the relationship. But the relation is morphable so it can contain relations to ModelA and ModelB. Now we get into trouble - our ModelA uses soft deletes but ModelB doesn't. But the relation doesn't know about the difference and Laravel/Eloquent tries to call the withTrashed() scope on both models - which results in an exception. But we don't get any info about which records failed and also can't repeat only these records without the scope.

But I will spend some time to get again into all this and check it with current Laravel 8 - probably something has changed over time.

[Gummibeer]

Okay, nothing has changed - we have no way from the outside to manipulate the applied scopes based on the type of the morphed relation record. The closest I got was this - but it works only for single records $activity->subject but not for loading the whole relation $activities->load('subject').

if(array_key_exists(SoftDeletes::class, class_uses_recursive($morphTo->getQuery()->getModel()))) {
    dump(SoftDeletes::class);
} 

Here are both backtraces how the closure/macro in \Illuminate\Database\Eloquent\SoftDeletingScope::addWithTrashed() is called. backtrace-relation.json backtrace-single.json

[MichMich]

Can't you check if the model has a scopeWithTrashed method, and if not, add a dummy one that does not alter the query?

[Gummibeer]

That's something the user could do but I don't see why we should "fix" this as it's not a bug with this package but a general morphed relation Laravel one. So, I would accept a PR that adds a new section to the docs (like we did yesterday for Pivot Models). https://spatie.be/docs/laravel-activitylog/v3/advanced-usage/logging-model-events#logging-on-pivot-models The section should completely describe how to make mixed soft/hard deleted subjects work together.

At all my recommendation is to don't mix soft and hard deletes in an app and think like a thousand times about the usage of soft deletes at all. Because in ~99% of the cases they are misused.

[crynobone]

Just an update, this issue is now resolved in Laravel Framework 10.20.0