-
Notifications
You must be signed in to change notification settings - Fork 169
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
Support sentry/sentry 2.3 and fix deprecations #298
Conversation
…n and avoid deprecation
@@ -22,7 +22,7 @@ | |||
"php": "^7.1", | |||
"jean85/pretty-package-versions": "^1.0", | |||
"ocramius/package-versions": "^1.3.0", | |||
"sentry/sdk": "^2.0", | |||
"sentry/sdk": "^2.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This locks against sentry/sentry:^2.3
. Because of this, this should be released as 3.4, to leave space for 3.3.x patches.
@@ -6,7 +6,6 @@ | |||
backupGlobals="false" | |||
bootstrap="vendor/autoload.php" | |||
cacheResult="false" | |||
processIsolation="true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've used an annotation directly on the only test that needs it, the End2EndTest
, so now the test suite is faster.
if (PrettyVersions::getVersion('sentry/sentry')->getPrettyVersion() !== '2.0.0') { | ||
$optionsChildNodes->booleanNode('capture_silenced_errors'); | ||
} | ||
if ($this->classSerializersAreSupported()) { | ||
$optionsChildNodes->arrayNode('class_serializers') | ||
->defaultValue([]) | ||
->prototype('scalar'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here (and below) I've removed all the conditions that were needed for backward compatibility with sentry/sentry
2.0, it's no longer needed.
@@ -114,7 +112,7 @@ public function getConfigTreeBuilder(): TreeBuilder | |||
->defaultValue($defaultValues->getPrefixes()) | |||
->prototype('scalar'); | |||
$optionsChildNodes->scalarNode('project_root') | |||
->defaultValue('%kernel.project_dir%'); | |||
->setDeprecated(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a double deprecation, it will pop up during cache warmup, so the user can be notified before reaching production.
public static function getCurrentHub(): HubInterface | ||
{ | ||
if (class_exists(SentrySdk::class)) { | ||
return SentrySdk::getCurrentHub(); | ||
} | ||
|
||
return Hub::getCurrent(); | ||
return SentrySdk::getCurrentHub(); | ||
} | ||
|
||
/** | ||
* This method avoids deprecations with sentry/sentry:^2.2 | ||
*/ | ||
public static function setCurrentHub(HubInterface $hub): void | ||
{ | ||
if (class_exists(SentrySdk::class)) { | ||
SentrySdk::setCurrentHub($hub); | ||
|
||
return; | ||
} | ||
|
||
Hub::setCurrent($hub); | ||
SentrySdk::setCurrentHub($hub); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those two methods are left here for BC.
->defaultValue([ | ||
'%kernel.project_dir%', | ||
]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed that this behavior doesn't work well, and doesn't reproduce the previous behavior, due to getsentry/sentry-php#953 😞
@@ -75,14 +70,19 @@ public function getConfigTreeBuilder(): TreeBuilder | |||
->defaultValue('%kernel.environment%') | |||
->cannotBeEmpty(); | |||
$optionsChildNodes->scalarNode('error_types'); | |||
$optionsChildNodes->arrayNode('in_app_include') | |||
->defaultValue([ | |||
'%kernel.project_dir%/src', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I'm trying to guess the source folder, since using the parent is not feasibile due to getsentry/sentry-php#953
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Jean85 With the change in sentry-php 2.3.1 all files are now included by default. Therefore, wouldn't an empty default value make more sense?
With the current default value '%kernel.project_dir%/src'
, it is impossible to exclude files inside '%kernel.project_dir%/src'
, since in_app_include
overrides in_app_exclude
. With an empty array as default value, this would still be possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is true, but it shouldn't have any practical effect you since you can override this default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You still might want to remove it since it takes precedence over in_app_exclude
which you need to know / remember to clear out the in_app_include
if you want to exclude anything that is in the src
dir.
If you keep it empty by default you can just start using the in_app_exclude
without also needing to clear the in_app_include
default.
It's not critical ofcourse but it is easier to not have it set and possibly make a in_app_exclude
value not work because of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, that's true! Done in #311
This PR fixes the build and supports the 2.3 client correctly, avoiding all deprecations.