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

Enable SentryBundle only in the "prod" env #1080

Closed
wants to merge 1 commit into from

Conversation

nicolas-grekas
Copy link
Member

Q A
License MIT

Fixes getsentry/sentry-symfony#421 (the part about `THE ERROR HANDLER HAS CHANGED!)

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

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'm sorry but I'm 👎 on this. It would break a lot of test break on the user side, because usage of Sentry in code could implode due to services missing.

Also, depending on how much Sentry code is used into the users', it could make it impossible to run test at all.

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Jan 26, 2021

This is only for new projects. Existing projects won't be affected.
No userland code should rely on Sentry-bundle, at least by default.
Even if one needs to, using code from sentry does not require enabling the bundle of course.

I think this is the correct default.

@Jean85
Copy link
Contributor

Jean85 commented Jan 26, 2021

No userland code should rely on Sentry-bundle, at least by default.
Even if one needs to, using code from sentry does not require enabling the bundle of course.

This is a fragile assumption. If I need to enrich the data sent to Sentry, I may need to interact with parts of the bundle, or the bundle could interact with the data in unforeseen ways.

Test may be broken; or worse, get false greens.

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Jan 26, 2021

That won't happen in any new projects.

Yet, the default right now breaks inspecting test suites that erroneously alter the error handler without restoring it.

@Jean85
Copy link
Contributor

Jean85 commented Jan 26, 2021

Apart from the warning, it shouldn't break tests, we take care of passing through all errors to error handlers below.

Furthermore, that will be solved by getsentry/sentry-symfony#337, but it's a lot of work to be done.

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Jan 26, 2021

Apart from the warning, it shouldn't break tests, we take care of passing through all errors to error handlers below.

correct, "Apart from the warning", which is not that a minor detail ;)

that will be solved by getsentry/sentry-symfony#337, but it's a lot of work to be done.

it's a lot of work yes! that's why I think we should do this change today, to relax the "pressure" on this issue.

@nicolas-grekas
Copy link
Member Author

@Jean85 please consider my arguments.

@Jean85
Copy link
Contributor

Jean85 commented Jan 28, 2021

I still see many downsides in this way. Non having the bundle loaded will also not trigger any deprecation coming from the whole Sentry stack, for example.

I'm against this because I personally reversed that approach from inside the bundle due to this getsentry/sentry-symfony#46 (comment) (and following discussion).

@Nyholm
Copy link
Member

Nyholm commented Feb 2, 2021

Hm..

Indeed this will fix the problem and not affecting exiting users. But it seams like it is a workaround instead of fixing the actual cause. I'll follow getsentry/sentry-symfony#421 and see if we can come up with a better solution.

@ste93cry
Copy link

ste93cry commented Feb 4, 2021

My two cents: while it's true that this change won't affect existing users, it makes installing the bundle broken for new people because as any other bundle providing services, if my project relies on them it won't work in dev environment until I change the bundles.php explicitly, while I think that the recipe should make it work out-of-the-box (that's what recipes are for, in my opinion). Apart from this, I see this as a workaround rather than a proper solution, and I strongly believe we should instead fix this in the SDK (bundle or core). The preferred solution to me is, as already said multiple times, to rely on Monolog which is the de-facto standard way of logging things in Symfony

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

Successfully merging this pull request may close these issues.

Overriding error handler changes symfony error handler
4 participants