Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Causer for Logging Model Events #614

Closed
wants to merge 1 commit into from
Closed

Added Causer for Logging Model Events #614

wants to merge 1 commit into from

Conversation

jny986
Copy link

@jny986 jny986 commented Oct 10, 2019

Added logCauser variable for setting logging of causer if not false and user is logged will add causer to logging
#503

Added logCauser variable for setting logging of causer if not false and user is logged will add causer to logging
@Gummibeer
Copy link
Collaborator

Hey, what exactly should your PR solve/fix? I've added the unittest to the current code base and it passed.
We already set the default causer in

protected function getActivity(): ActivityContract
{
if (! $this->activity instanceof ActivityContract) {
$this->activity = ActivitylogServiceProvider::getActivityModelInstance();
$this
->useLog($this->defaultLogName)
->withProperties([])
->causedBy($this->auth->guard($this->authDriver)->user());
}
return $this->activity;
}

@jny986
Copy link
Author

jny986 commented Oct 10, 2019

@Gummibeer Thanks for pointing that out.

I missed that and it is obviously just that with the 2 different Auth drivers that the person is using in the issue that I linked too that is causing the problem.

Would it be worth changing to using the Auth::user() instead of a configurable driver?

@jny986 jny986 closed this Oct 10, 2019
@Gummibeer
Copy link
Collaborator

It already uses the default auth driver:

$this->authDriver = $config['activitylog']['default_auth_driver'] ?? $auth->getDefaultDriver();

So I don't see a benefit in switching to the facade instead of dependency injection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants