LogsActivity::$changesPipes should be cleared

[chWagnr]

Describe the bug
While writing tests we discovered that LogsActivity::$changesPipes contains the values of the previous tests. The pipes are added in the booted() like in the doc.

There is also an Observer registered for the model in the EventServiceProvider which causes the boot. LogsActivity::$changesPipes is static and never cleared so with every test the value is incremented with every boot.

Creating a model instance in the test also causes this problem.

To Reproduce
Add pipe in model's booted method:

class YourModel extends Model
{
    use LogsActivity;
    protected static function booted(): void
    {
        static::addLogChange(new class() implements LoggablePipe {
            public function handle(EventLogBag $event, \Closure $next): EventLogBag
            {
                return $event;
            }
        });
    }
}    

Execute the test:

test('dump pipes 1', function () {
    new YourModel();
    dump(YourModel::$changesPipes);
});

test('dump pipes 2', function () {
    new YourModel();
    dump(YourModel::$changesPipes);
});

Dumps:

array:1 [
  0 => Spatie\Activitylog\Contracts\LoggablePipe@anonymous^ {#2999}
] 
array:2 [
  0 => Spatie\Activitylog\Contracts\LoggablePipe@anonymous^ {#2999}
  1 => Spatie\Activitylog\Contracts\LoggablePipe@anonymous^ {#3741}
] 

Expected behavior
LogsActivity::$changesPipes must be cleared before boot.

Versions (please complete the following information)

• PHP: 8.3
• Laravel: 10.48.4
• Package: 4.8.0

[Gummibeer]

I'm not sure but that sounds more like a PHPUnit backupStaticProperties config thing. 🤔
Yes, static variables are "complicated" for long running processes. But as in theory every test should be a fresh runtime that sounds more like a PHPUnit issue. Haven't tested it with octane but with octane it should work as octane only boots the model once. The issue is that the model is booted again but the runtime isn't cleared. Which normal never happens for Laravel runtimes. Beside octane another place to check would be the queue worker/horizon as it's a long running process as well. Not 100% sure how each job is processed there.

[chWagnr]

On horizon it's also booted once as far as I see

[Gummibeer]

Okay, so after all it comes down to a PHPUnit "problem". As listening for the boot event is the way to go in Laravel I don't really know what else to do. Have you tried the PHPUnit backupStaticProperties config?

[Jacobs63]

Hello,

just debugged the same issue, I've made a quick workaround:

<?php

declare(strict_types=1);

namespace App\ActivityLog;

use App\ActivityLog\Pipes\ExcludeUnchangedAttributesPipe;
use Spatie\Activitylog\Contracts\LoggablePipe;
use Spatie\Activitylog\LogOptions;
use Spatie\Activitylog\Traits\LogsActivity;

trait LogsActivityTrait
{
    use LogsActivity;

    public function getActivitylogOptions(): LogOptions
    {
        return LogOptions::defaults()->logAll();
    }

    protected static function bootLogsActivityTrait(): void
    {
        static::addLogChange(new ExcludeUnchangedAttributesPipe());
    }

    public static function addLogChange(LoggablePipe $pipe): void
    {
        static::$changesPipes[$pipe::class] = $pipe; // <<< THE FIX
    }
}

This ensures that even if a log change pipe is added multiple times, it will still only be added to the pipes array once.
The drawback is that you cannot reuse the same pipe on one class multiple times (which you'd likely not do anyways).

[chWagnr]

@Gummibeer Setting the value of backupStaticProperties from default false to true resolves the issue.

@Jacobs63 Nice solution!

[LukeAbell]

We have a multitenant octane application and are having an issue with this. changesPipes is shared between requests and causing logs to be recorded mixing up tenants data.

Our custom trait looks like this:

use Spatie\Activitylog\LogOptions;
use Spatie\Activitylog\Traits\LogsActivity as BaseLogsActivity;

trait LogsActivity
{
    use BaseLogsActivity;

    public function getActivitylogOptions(): LogOptions
    {
        return LogOptions::defaults()
            ->logOnlyDirty()
            ->logAll()
            ->dontSubmitEmptyLogs();
    }

We're adding the log change before making changes to the model.

I think this functionality should be refactored to not be static?

[sebastiandedeyne]

Looks like this still uses a static property from the pre-Octane days. A new (major?) version should probably rely on the Laravel container with scoped instead.