Skip to content

Conversation

@HypeMC
Copy link
Contributor

@HypeMC HypeMC commented Sep 25, 2019

Resolves #232.

As mentioned in the issue, having both the Monolog handler & the ErrorListener installed results in having duplicate entries.

I added a configuration option to disable the ErrorListener. I also added the ability to configure the Monolog handler through the bundle.

I still need to add some tests as well, but I wanted to get some feedback first on whether or not this approach is good.

Copy link
Contributor

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

The approach overall seems good, thanks for tackling this; I've added some comments.

Copy link
Contributor

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

LGTM 👍 I've added two nitpicks, and you still have to fix and add tests.

@Jean85 Jean85 changed the title Disable error listener Options to disable error listener and/or enable Monolog handler Sep 27, 2019
@HypeMC
Copy link
Contributor Author

HypeMC commented Sep 27, 2019

@Jean85 Tests added

Copy link
Contributor

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

I've left some answers to your questions. I've also added a few comments. Thanks for your continuing effort 👍

Copy link
Contributor

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

I've added the changelog entries myself. Thank you very much for your contribution!

@krewetka
Copy link

krewetka commented Oct 29, 2020

Hi @Jean85 @HypeMC could you explain me how this is supposed to work?

When service Sentry\Monolog\Handler is private one and cannot be used in config files?

In SentryExtension I found only:

 if (! $errorHandler['enabled']) { $container->removeDefinition(Handler::class); return; } 

and then configuration for parameters of this service. I see it removed when not enabled but I don't see it public.

Adding to monolog config lines:

monolog: handlers: sentry: type: service id: Sentry\Monolog\Handler 

causes info that service with this id is not found 🤔

I had to add to my own services:

 Sentry\Monolog\Handler: public: true arguments: ['@Sentry\State\HubInterface', 400] `` to make it work but whole function `configureMonologHandler` is not really used :/ 
@Jean85
Copy link
Contributor

Jean85 commented Oct 29, 2020

Services don't need to be public to be used. You can use proper injection to obtain them.

@krewetka
Copy link

krewetka commented Oct 30, 2020

Ok, I will double check why it could not find the service in my case until I redeclared it myself in my own services.yml

I know they don't have to be public for dependency injection and most of them can be private

@krewetka
Copy link

@Jean85 looks like symfony cache strange behaviour which got me confused. Thanks for quick answer.

After cleaning everything once more ( and also removing cache folder) somehow it started to work with id: Sentry\Monolog\Handler

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

3 participants