spatie / laravel-activitylog

Log activity inside your Laravel app
https://docs.spatie.be/laravel-activitylog
MIT License
5.36k stars 714 forks source link

Nested null relation throws TypeError #1235

Closed Spice-King closed 8 months ago

Spice-King commented 1 year ago

Describe the bug Nested relations does not handle null related model correctly and instead pulls the relation via the function, choking since it's now not a model.

To Reproduce This is a bit contrived example, but it does cut it to the most short reproduction case. Our production one makes more logical sense as to why we do this. An Item has a Material, which has a Cut, which has a property called name, thus requesting the attribute "material.cut.name" on the Item model. This one always exists, but we have a backMaterial relation, which does not.

<?php

namespace App\Models;

use Illuminate\Database\Eloquent\Model;
use Spatie\Activitylog\LogOptions;
use Spatie\Activitylog\Traits\LogsActivity;

class Test extends Model
{
    use LogsActivity;

    public function getActivitylogOptions(): LogOptions
    {
        return LogOptions::defaults()
            ->logOnly([
                // disabled to show the edge case in isolation
                // "*",
                // "parent.name",
                "parent.parent.name",
            ]);
    }

    public function parent()
    {
        return $this->belongsTo(self::class, "parent_id");
    }
}

Test::create(); //exception, record still created though
Test::whereNull("parent_id")->first()->delete(); //exception, the record is deleted
Test::whereHas("parent", fn($q) => $q->whereNull("parent_id")->first()->delete(); //exception, the record is deleted

Expected behavior No exceptions.

Screenshots N/A

Versions (please complete the following information)

Additional context A possible fix for this might be this, though someone can also argue that some part of this is a breaking change.

--- a/src/Traits/LogsActivity.php
+++ b/src/Traits/LogsActivity.php
@@ -398,6 +398,9 @@
             $attributeName[] = $relatedModelName = static::getRelatedModelRelationName($relatedModel, array_shift($relatedModelNames));

             $relatedModel = $relatedModel->$relatedModelName ?? $relatedModel->$relatedModelName();
+            if ($relatedModel === null || $relatedModel instanceof \Illuminate\Database\Eloquent\Relations\Relation) {
+                return [$attribute => null];
+            }
         } while (! empty($relatedModelNames));

         $attributeName[] = $relatedAttribute;

It's also possible it's decided that it's not a supported case and should explicitly throw an error for it instead of getRelatedModelRelationName throwing a TypeError when given a non model. Looks like the original logic was added in 09035a48ad01943cc1eb553b3baf4058b6d7cab0 (v1.12.0), blame stopped add 5a345aca0abca4fe8cd0263f19b86d1760fac02b (v4.0.0), but I found the thread.

As an aside, this also does not handle HasMany like relations which have a collection, but also they have no special handling anyways up until this point.

Exception App\Models\TestModel::getRelatedModelRelationName(): Argument #1 ($model) must be of type Illuminate\Database\Eloquent\Model, Illuminate\Database\Eloquent\Relations\BelongsTo given, called in /var/www/html/vendor/spatie/laravel-activitylog/src/Traits/LogsActivity.php on line 398 {"userId":67,"exception":"[object] (TypeError(code: 0): App\\Models\\Inventory\\MaterialManagement\\MaterialItem::getRelatedModelRelationName(): Argument #1 ($model) must be of type Illuminate\\Database\\Eloquent\\Model, Illuminate\\Database\\Eloquent\\Relations\\BelongsTo given, called in /var/www/html/vendor/spatie/laravel-activitylog/src/Traits/LogsActivity.php on line 398 at /var/www/html/vendor/spatie/laravel-activitylog/src/Traits/LogsActivity.php:408)

Stack Trace

#0 /var/www/html/vendor/spatie/laravel-activitylog/src/Traits/LogsActivity.php(398): App\\Models\\Inventory\\MaterialManagement\\MaterialItem::getRelatedModelRelationName(Object(Illuminate\\Database\\Eloquent\\Relations\\BelongsTo), 'Species')
#1 /var/www/html/vendor/spatie/laravel-activitylog/src/Traits/LogsActivity.php(337): App\\Models\\Inventory\\MaterialManagement\\MaterialItem::getRelatedModelAttributeValue(Object(App\\Models\\Inventory\\MaterialManagement\\MaterialItem), 'backMaterial.Sp...')
#2 /var/www/html/vendor/spatie/laravel-activitylog/src/Traits/LogsActivity.php(267): App\\Models\\Inventory\\MaterialManagement\\MaterialItem::logChanges(Object(App\\Models\\Inventory\\MaterialManagement\\MaterialItem))
#3 /var/www/html/vendor/spatie/laravel-activitylog/src/Traits/LogsActivity.php(54): App\\Models\\Inventory\\MaterialManagement\\MaterialItem->attributeValuesToBeLogged('deleted')
#4 /var/www/html/vendor/laravel/framework/src/Illuminate/Events/Dispatcher.php(421): App\\Models\\Inventory\\MaterialManagement\\MaterialItem::Spatie\\Activitylog\\Traits\\{closure}(Object(App\\Models\\Inventory\\MaterialManagement\\MaterialItem))
#5 /var/www/html/vendor/laravel/framework/src/Illuminate/Events/Dispatcher.php(249): Illuminate\\Events\\Dispatcher->Illuminate\\Events\\{closure}('eloquent.delete...', Array)
#6 /var/www/html/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasEvents.php(188): Illuminate\\Events\\Dispatcher->dispatch('eloquent.delete...', Array)
#7 /var/www/html/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php(1412): Illuminate\\Database\\Eloquent\\Model->fireModelEvent('deleted', false)
#8 /var/www/html/app/Http/Controllers/Inventory/TestController.php(177): Illuminate\\Database\\Eloquent\\Model->delete()
spatie-bot commented 8 months ago

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.