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.
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.
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.
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.
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.
Those two methods are left here for BC.
| ->defaultValue([ | ||
| '%kernel.project_dir%', | ||
| ]) |
There was a problem hiding this comment.
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.
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.
@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.
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.
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.
Yup, that's true! Done in #311
This PR fixes the build and supports the 2.3 client correctly, avoiding all deprecations.