Fix SENTRY_DSN handling with nullable values

[salexi]

I will assume that when reading options for Raven\Client you want $_SERVER variable to overwrite any settings from config for example.

In laravel's case, we have a small issue. When reading the .env file laravel puts anything it finds there in $_SERVER as strings

If i write a proper SENTRY_DSN in .env file or if i leave it empty, everything is fine. But if i write SENTRY_DSN=false or SENTRY_DSN=null in the .env file, my app will crash.

$options has dsn with a correct value of null or false, however it's overwritten by $_SERVER['SENTRY_DSN'] which is "false" or "null" (as strings). This causes the Raven_Client::parseDSN($dsn); to fail

In this PR before assigning $dsn = @$_SERVER['SENTRY_DSN] i check if $_SERVER['SENTRY_DSN] contains "false" or "null" and if so, i assign $dsn = null.

This makes laravel not crash anymore.

[Jean85]

Maybe you should take a look at our official Laravel integration?
https://github.com/getsentry/sentry-laravel

Also, env variables are always strings, you shouldn't set them at all to obtain a null/false value.

[salexi]

Already using https://github.com/getsentry/sentry-laravel.

Also, env variables are always strings, you shouldn't set them at all to obtain a null/false value.

I find it confusing that in $options the value is correct (boolean false or null) but it's overwritten by a string from $_SERVER and while you are correct, setting it to null in .env doesn't seem such a bad idea, and i don't think it should crash the app.

Again, maybe you're right but maybe "null" or "false" shouldn't cause it to crash, maybe the parser should check against it and accept it just as it accepts null, or false as non strings.

If you don't find this fit, then ok. But i think we can agree that "null" and "false" could be used as a disabler and there is no way they could be confused for a proper dsn

[stayallive]

Not saying it's a good idea but maybe we can take hint from Laravel on reading "environment" variables. I do think setting it to null/false is a bad idea but people will do it anyway since some find it more readable or clear.

We could check for these values for example, what do you think @Jean85?

https://github.com/laravel/framework/blob/242d438700597aa6bdcbc8a21a14fe3a241c91a3/src/Illuminate/Support/helpers.php#L611-L624

[Jean85]

I agree with taking inspiration from that piece of Laravel code; but I would like to have tests on this feature too, then we can merge it.

[stayallive]

@Jean85 agreed.

@salexi would you like to add tests and add this piece of helper code, or would you like me to add that.

[salexi]

@stayallive Where would you like that piece of code added ?
Before this if (!empty($dsn)) { $options = array_merge($options, self::parseDSN($dsn)); } or inside Client::parseDSN() method ?

I could try to add it, or if you're more comfortable doing it, i won't mind.

[salexi]

@stayallive @Jean85
Tell me if you're ok with the changes now.
Thanks

[stayallive]

This looks fine for me, @Jean85 something similair should also be adapted for 2.0, but for 1.x I think this is fine.

[Jean85]

Thanks @salexi, I took the liberty of refactoring your tests with a dataprovider.

@stayallive that's why I requested tests, I intend to merge master into 2.0 before releasing, and take there all the new stuff that we merged on master after branching for the new release.

[salexi]

Great, thanks 👍

[stayallive]

FYI: 1.8.2 has been released with this PR in it. Thanks!