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..

laravel-activitylog/src/Models/Activity.php

Lines 28 to 35 in 68eb6e6

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

[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?

laravel-activitylog/tests/ActivityModelTest.php

Lines 146 to 179 in fb19f49

	public function it_does_not_throw_exception_if_with_trashed_method_is_not_defined()
	{
	$this->app['config']->set('activitylog.subject_returns_soft_deleted_models', true);
	$causer = User::first();
	$subject1 = Article::create(['name' => "name article subject1"]);
	$subject2 = ArticleSoftDelete::create(['name' => "name article_soft_delete subject2"]);
	activity()->on($subject1)->by($causer)->log('foobar subject1');
	activity()->on($subject2)->by($causer)->log('foobar subject2');
	$activities1 = Activity::forSubject($subject1)->get();
	$this->assertCount(1, $activities1);
	$this->assertEquals($subject1->getKey(), $activities1->first()->subject_id);
	$this->assertEquals($subject1->getKey(), $activities1->first()->subject->getKey());
	$this->assertEquals('foobar subject1', $activities1->first()->description);
	$subject1->delete();
	$activities1 = Activity::forSubject($subject1)->get();
	$this->assertCount(1, $activities1);
	$this->assertEquals($subject1->getKey(), $activities1->first()->subject_id);
	$this->assertNull($activities1->first()->subject);
	$activities2 = Activity::forSubject($subject2)->get();
	$this->assertCount(1, $activities2);
	$this->assertEquals($subject2->getKey(), $activities2->first()->subject_id);
	$this->assertEquals('foobar subject2', $activities2->first()->description);
	$subject2->delete();
	$activities2 = Activity::forSubject($subject2)->get();
	$this->assertCount(1, $activities2);
	$this->assertEquals($subject2->getKey(), $activities2->first()->subject_id);
	$this->assertEquals($subject2->getKey(), $activities2->first()->subject->getKey());
	}

[bluec]

Hi @Gummibeer you described the behaviour exactly here: #445 (comment)

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 #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.

[minamassoud]

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