Skip to content

Exposes Sentry\Monolog\Handler service for easier monolog configuration. #225

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

Merged
merged 1 commit into from
Jul 16, 2019

Conversation

phpeek
Copy link
Contributor

@phpeek phpeek commented Jul 4, 2019

I ❤️ using this bundle.

This PR solves one problem that's still cumbersome to me.

sentry/sentry ships with Sentry\Monolog\Handler for smooth integration with monolog, however this bundle is not exposing it as a service making integration more difficult.

My PR configures Sentry\Monolog\Handler as a service, which - in theory - should simplify configuration.

I've updated example for reference, but it's commented out since not everyone might like it configured out-of-the-box.

@phpeek
Copy link
Contributor Author

phpeek commented Jul 4, 2019

Second option of integration is via https://github.com/getsentry/raven-php, but it's marked as deprecated.

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.

This seems an interesting addition, which would create no downsides if it's not used.

I would add some tests to check for the existence of the service definition, and instead of adding that to the examples, I would add that in the readme!

@phpeek
Copy link
Contributor Author

phpeek commented Jul 5, 2019

Good idea. Will do that later today. Thanks for looking at this !

@phpeek
Copy link
Contributor Author

phpeek commented Jul 5, 2019

I've added test You've requested, but it's failing due to a valid reason.

If someone would install this bundle without having monolog installed, exposing Handler as a service would fail due to lack of the class Monolog\Handler\AbstractProcessingHandler that is being extended.

sentry/sentry has monolog/monolog dependency but it's a dev one - which is valid.

Will wait for Your feedback, but I'm leaning towards closing this PR.

@Jean85
Copy link
Contributor

Jean85 commented Jul 5, 2019

Surely defining that service every time would be bad for sure. Instead of closing, maybe we can reroute this to a README-only PR? You could suggest the service definition there too.

@phpeek phpeek force-pushed the expose-sentry-monolog-handler branch from 9c466dc to be3aff6 Compare July 6, 2019 20:21
@phpeek phpeek force-pushed the expose-sentry-monolog-handler branch from be3aff6 to 1719c92 Compare July 6, 2019 20:23
@phpeek
Copy link
Contributor Author

phpeek commented Jul 6, 2019

As suggested, I've updated only README.md with optional instructions on how to setup monolog handler.

@Jean85 Jean85 mentioned this pull request Jul 16, 2019
@Jean85 Jean85 merged commit bbdb50d into getsentry:master Jul 16, 2019
@Jean85
Copy link
Contributor

Jean85 commented Jul 16, 2019

Thanks!


```yaml
monolog:
sentry:
Copy link

Choose a reason for hiding this comment

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

Doesn't this need to be

monolog:
    handlers:
        sentry:

?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right! #230

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.

3 participants