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

Sentry: Add enabled config option #11

Merged
merged 1 commit into from
Sep 9, 2018
Merged

Sentry: Add enabled config option #11

merged 1 commit into from
Sep 9, 2018

Conversation

RiKap
Copy link
Contributor

@RiKap RiKap commented Aug 31, 2018

Add easy way to disable Sentry logging

@coveralls
Copy link

coveralls commented Aug 31, 2018

Coverage Status

Coverage remained the same at 44.444% when pulling 0ef3484 on RiKap:patch-2 into 67a02b3 on contributte:master.

@RiKap
Copy link
Contributor Author

RiKap commented Sep 5, 2018

@f3l1x What do you think?

@f3l1x
Copy link
Member

f3l1x commented Sep 5, 2018

I get the point. Isn't better to just not register the SentryLogger at all?

@RiKap
Copy link
Contributor Author

RiKap commented Sep 5, 2018

@f3l1x I don't think so. I just register all loggers in config.neon. When I put code on testing instance I do not need logs log into sentry. So I think that it is much easier to have, in config.local.neon this:

sentry:
    enable: true/false

than this:

extensions:
	sentry: Contributte\Logging\DI\SentryLoggingExtension
        url: '.....'

I see that is almost same, but config.local.neon I want to change only values not register new extensions, services ....

@f3l1x
Copy link
Member

f3l1x commented Sep 8, 2018

I ment don't register SentryLogger in DI. So your code will be same except for enabled: true/false.

@RiKap
Copy link
Contributor Author

RiKap commented Sep 8, 2018

@f3l1x When I register SentryLoggingExtension it automatically register SentryLogger. Yes I can skip registration of sentry extension and use logging.loggers configuration to just add SentryLogger in config.local.neon. But I still think that enabled: true/false is better than register SentryLogger or register anything in config.local.neon.

Anyway it is up to you. It is not dealbreaker for me, just nice thing to have. :)

@@ -27,6 +28,7 @@ public function loadConfiguration()
$config = $this->validateConfig($this->defaults);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You still don't get my point.

I was pointing this code:

if ($config['enabled'] === false) return;

Skip adding the whole definition, if it's not enabled (enabled: false).

WDYT?

@RiKap
Copy link
Contributor Author

RiKap commented Sep 8, 2018

@f3l1x Ahhh, sorry. I thought that you talk about configuration, not PR. PR is updated.

@f3l1x
Copy link
Member

f3l1x commented Sep 8, 2018

Cool, thanks.

@f3l1x f3l1x merged commit 83448a5 into contributte:master Sep 9, 2018
@f3l1x
Copy link
Member

f3l1x commented Sep 9, 2018

👍

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

Successfully merging this pull request may close these issues.

3 participants