spatie / laravel-activitylog

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

`tapActivity` method is being called twice #1184

Closed debiprasad closed 7 months ago

debiprasad commented 1 year ago

I have added tapActivity method in a model to prefix the event name.

    public function tapActivity(Activity $activity, string $eventName)
    {
        $activity->event = 'website_'.$eventName;
    }

I expected the following result:

$activity = Activity::all()->last();

$activity->event; // returns 'website_created`

Instead, I get the following result:

$activity->event; // returns 'website_website_created`

To Reproduce Add Spatie\Activitylog\Traits\LogsActivity trait to any model. Add the tapActivity method as shown above. Trigger any of these events: created, updated and deleted.

Expected behavior

$activity = Activity::all()->last();

$lastActivity->event; // returns 'website_created`

Versions

rabol commented 1 year ago

same here

rabol commented 1 year ago

Depend on how you look at it, the issue is either in the trait LogsAtivity line 84, where tapActivity is called if it exists and then on line 88 the $logger->log() is called. Or in ActivityLogger->log() method, the tapActivity is also called on line 170

krembo commented 1 year ago

Same here. It's a result of a feature introduced in this PR - #1031 There was a try to suggest a removal here - #1170 - which was blocked by the maintainers...

The solution in #1170 was good as tapActivity is called in log() method anyway.

xdubois commented 1 year ago

I'm facing the samee issue, it's kind of weird it is called a feature

renky commented 1 year ago

I faced the same issue. I try to log all activities on related sub-models on the parent-model. Therefor I use the tapActivity method on the submodel and rewrite it to the parent and add the properties to "sub-arrays" (if there is a better way to get this result - I'm open for suggestions).

Finally my method looks like that:

    public function tapActivity(Activity $activity, string $eventName): void
    {
        $activity->subject_type = $this->parent::class;
        $activity->subject_id = $this->parent->id;

        $activity->properties = $activity->properties->
            map(fn($attributes) => ['relationName' => $attributes]);
    }

I expected to get an entry like this:

     App\Models\Activity {
        ...
        properties: "{"attributes":{"relationName":{"value":"oldvalue"}},"old":{"relationName":{"value":"newvalue",
        ...
      },

But indeed I get this: I expected to get an entry like this:

     App\Models\Activity {
        ...
        properties: "{"attributes":{"relationName":{"relationName":{"value":"oldvalue"}}},"old":{"relationName":{"relationName":{"value":"newvalue"}}}}",
        ...
      },

So it is indeed called twice in the same request with the same activity model.

Unfortunatelly this is open since several months now - Why was the PR https://github.com/spatie/laravel-activitylog/pull/1170 blocked? It was never clarified, what feature would be removed since tapActivity is called anyway from the log method?!

renky commented 1 year ago

Btw: I use a very crazy workaround for it now:

    public function tapActivity(Activity $activity, string $eventName): void
    {
        if ($activity->alreadyCalled) {
            unset ($activity->alreadyCalled);
            return;
        }
        $activity->alreadyCalled = true;

         // do what you want to do herer....
    }

maybe it helps somebody - sure, it will fail with a "alreadyCalled is no column in database" as soon as this problem is fixed, but I can live with that... finally it helps me removing this workarround as soon as it is not needed any more... every other workarround like detecting if the change was already there, would survive forever and never be removed....

darkphoenixff4 commented 11 months ago

In my case, it's getting called four times, and creating three event records: updated, updated (Model) and changed (attribute). Is there a way to set it so that it only creates one event per update, rather than three?

spatie-bot commented 7 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.

horices commented 3 months ago

How can I fix this problem? Does this actually qualify as a bug, and why hasn't this problem been fixed?