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

Options to disable error listener and/or enable Monolog handler #247

Merged
merged 3 commits into from Sep 30, 2019
Merged

Options to disable error listener and/or enable Monolog handler #247

merged 3 commits into from Sep 30, 2019

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
Collaborator

@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.

composer.json Outdated Show resolved Hide resolved
src/DependencyInjection/Configuration.php Show resolved Hide resolved
src/DependencyInjection/SentryExtension.php Outdated Show resolved Hide resolved
src/DependencyInjection/Configuration.php Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@Jean85 Jean85 changed the title Disable error listener Options to disable error listener and/or enable Monolog handler Sep 27, 2019
composer.json Outdated Show resolved Hide resolved
@HypeMC
Copy link
Contributor Author

HypeMC commented Sep 27, 2019

@Jean85 Tests added

Copy link
Collaborator

@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 👍

test/DependencyInjection/SentryExtensionTest.php Outdated Show resolved Hide resolved
Copy link
Collaborator

@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
Collaborator

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow to disable ErrorListener completely
3 participants