Skip to content
This repository has been archived by the owner on Jun 1, 2019. It is now read-only.

Add ignored user for whom logging is disabled. #19

Merged
merged 5 commits into from
Oct 15, 2015
Merged

Add ignored user for whom logging is disabled. #19

merged 5 commits into from
Oct 15, 2015

Conversation

lowerends
Copy link
Contributor

This PR adds the config option to add a user id for whom logging is disabled. This can for example be useful for an administrator type of user.

@freekmurze
Copy link
Member

Thanks for your contribution, but I think ignoring certain log-calls isn't a good responsability of this package. There might be hundreds of reason why something shouldn't be logged. An mostly those reason are app specific. In your case your app should check the user before making a call to Activity::log().

@lowerends
Copy link
Contributor Author

I understand, but checking the user before every Activity::log() call may become cumbersome.
Would it be an option to have a reference to a handler in the config file that allows to write your own logic of why something shouldn't be logged? One could then write a custom handler that adheres to an interface (to be defined). If the handler returns true, the event is logged; if the handler returns false, the event is not logged.

@freekmurze
Copy link
Member

I mind mind setting up handlers for this would be overkill. I think it's very easy create a function in an app like this:

function myLogger() {
   if (<your conditions>) {
      Activity::log('logging like there's no tomorrow');
   }
}

so logging when some conditions are met doesn't become cumbersome. There are so many ways to go about it. You could have a function, or a class, or a service.

@lowerends
Copy link
Contributor Author

I'll just give it a try tomorrow to see how it would look like and update the PR to discuss further.

@freekmurze
Copy link
Member

Ok!

This change allows to write any custom logic as to whether logging
should be done or not. By default, everything is logged, so the change
is backwards compatible. If any logic is needed to enable or disable
logging, this can be done in a custom class that implements the
`IgnoreHandlerInterface`.
@lowerends
Copy link
Contributor Author

I just updated the PR to a more generic implementation. The change is actually quite minimal, but it opens a lot of flexibility to enable or disable logging. I'll update the readme depending on the outcome of this discussion.

@freekmurze
Copy link
Member

I'd like to merge this PR after these changes:

  • I think we could the change more generic by not using "ignore" but "before" in naming things
  • the default beforeHandler in the config should be null
  • do not use app()->make but use the contstructor for injecting
  • use php-cs-fixer to make sure you're using psr2 everywhere (this is an old package, so maybe there are some files that php-cs-fixer will change that you didn't modify, that's fine)

Thanks!

@lowerends
Copy link
Contributor Author

On it!

This commit implements the feedback by @freekmurze.
@lowerends
Copy link
Contributor Author

I just updated the PR to include your feedback. Let me know if there's anything that needs to be changed.

@freekmurze
Copy link
Member

We're getting there 😄
Could you update the readme with some instructions on how to use the latests changes? Then I'll merge, scrutinize it some more and tag it as a new version.

@lowerends
Copy link
Contributor Author

Great!

I added some instructions and an example to the readme.

freekmurze added a commit that referenced this pull request Oct 15, 2015
Add ignored user for whom logging is disabled.
@freekmurze freekmurze merged commit 4163be6 into spatie:master Oct 15, 2015
@freekmurze
Copy link
Member

Hi, I changed things a bit in the current master. The ignore-function is renamed to shouldLog. It should return true if you want thing to get logged. The function also received the text that is about to be logged and the id of the user that is going to be used (this could be a different user than the currently authenticated one.

I also went back to creating the handler out of the ioc container instead of using dep injection because that would be a breaking change (hadn't thought of that before).

Could you try the current master branch and let me know if this works for you?

@lowerends
Copy link
Contributor Author

Adding the text and the user that could be logged looks like a great addition. I'll try it out right away and let you know.

@lowerends
Copy link
Contributor Author

It looks like there's a small bug on this line: https://github.com/spatie/activitylog/blob/master/src/Spatie/Activitylog/ActivitylogSupervisor.php#L121:

$beforeHandler = $this->config('activitylog.beforeHandler');

should be

$beforeHandler = $this->config->get('activitylog.beforeHandler');

After I changed that locally everything seems to work fine. You want me to create a PR for this?

@freekmurze
Copy link
Member

Whoops, sorry about that. It's fixed now. Going to test it out a little bit and tag it soon as a new version.

Thank you very much for your contribution!

@lowerends
Copy link
Contributor Author

No worries. Great doing business like this :)

@freekmurze
Copy link
Member

I agree 😄

@freekmurze
Copy link
Member

This functionality is available in the newly tagged v2.2.0

@lowerends
Copy link
Contributor Author

Using it already 😎

@pongz79 pongz79 mentioned this pull request Jan 22, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants