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

Missing config file results in exception not being sent to Sentry #163

Closed
nmfzone opened this issue Oct 28, 2018 · 4 comments
Closed

Missing config file results in exception not being sent to Sentry #163

nmfzone opened this issue Oct 28, 2018 · 4 comments

Comments

@nmfzone
Copy link

nmfzone commented Oct 28, 2018

  • Laravel Version: 5.7.11
  • PHP Version: 7.2.11
  • Database Driver & Version: Mysql 14.4

Description:

The exception not being sent to the sentry. The problem actually happen on my production app with Laravel 5.6, using sentry-laravel version 0.9.*. Then, I decided to upgrade senty-laravel to 0.10.1. But, the issue still persist.

Finally, I try to use on fresh Laravel installation. But, the issue still.

Steps To Reproduce:

  1. Create new Laravel project
    composer create-project laravel/laravel MyApp --prefer-dist
  2. Add sentry to the app
    composer require sentry/sentry-laravel
  3. Set the SENTRY_LARAVEL_DSN to .env
  4. Add sentry to log channels
'channels' => [
    // ...
    'sentry' => [
        'driver' => 'sentry',
    ],
],
  1. Add sentry channel to stack channel
'stack' => [
    'driver' => 'stack',
    // Add the Sentry log channel to the stack
    'channels' => ['single', 'sentry'],
],
@nmfzone
Copy link
Author

nmfzone commented Oct 28, 2018

Okay, finally I found the problem. The problem is on the SentryLaravelServiceProvider.
There is no mergeConfigFrom() call in that service provider. That's the key. Because when the user not publish the sentry config file, set SENTRY_LARAVEL_DSN on the .env is useless.

The simplest solution to solve this issue, instead set SENTRY_LARAVEL_DSN on the .env, you need to set SENTRY_DSN on the .env.

@stayallive
Copy link
Collaborator

stayallive commented Oct 28, 2018

Okay good one, need to check when the merge config was introduced so we can implement it (still supporting 5.0 so need to be sure it was available then or add version switch for it).

However, I do want to note that in the installation docs creating the config is mentioned and not as optional :)

@stayallive stayallive changed the title Exception not being sent to the Sentry Missing config file results in exception not being sent to Sentry Oct 28, 2018
@nmfzone
Copy link
Author

nmfzone commented Oct 29, 2018

@stayallive Oh nice, my bad. I used Sentry since a long time ago, and I don't remember at that time, creating (publish) the config is mentioned in docs or not. And, the important thing, before this package introduce SENTRY_LARAVEL_DSN, I used only SENTRY_DSN without creating config file, all works fine.

Fyi, as far as I know, publish config is always optional on Laravel package. So, maybe mergeConfigFrom() should be considered. Thanks

@stayallive
Copy link
Collaborator

I agree, fixing this by 0d49fca

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

No branches or pull requests

2 participants