Fix for default integrations not disabled (#327)

* Fix for default integrations not disabled

* Add tests

* Add changelog entry

* Do not use `expectNotToPerformAssertions` (yet)

Author: stayallive

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## Unreleased
 
+- Fix for default integrations not disabled (#327)
+
 ## 1.6.1
 
 - Fix queue events with missing handler suffix (#322)

src/Sentry/Laravel/ServiceProvider.php

@@ -101,10 +101,12 @@ protected function configureAndRegisterClient(): void
             $basePath = base_path();
             $userConfig = $this->getUserConfig();
 
-            // We do not want this setting to hit our main client because it's Laravel specific
             unset(
+                // We do not want this setting to hit our main client because it's Laravel specific
                 $userConfig['breadcrumbs'],
-                // this is kept for backwards compatibilty and can be dropped in a breaking release
+                // We resolve the integrations through the container later, so we initially do not pass it to the SDK yet
+                $userConfig['integrations'],
+                // This is kept for backwards compatibilty and can be dropped in a future breaking release
                 $userConfig['breadcrumbs.sql_bindings']
             );
 
@@ -114,10 +116,7 @@ protected function configureAndRegisterClient(): void
                     'prefixes' => [$basePath],
                     'in_app_exclude' => ["{$basePath}/vendor"],
                 ],
-                $userConfig,
-                [
-                    'integrations' => $this->getIntegrations(),
-                ]
+                $userConfig
             );
 
             $clientBuilder = ClientBuilder::create($options);
@@ -135,13 +134,19 @@ protected function configureAndRegisterClient(): void
 
             $options = $clientBuilder->getOptions();
 
-            if ($options->hasDefaultIntegrations()) {
-                $integrations = $options->getIntegrations();
+            $userIntegrations = $this->resolveIntegrationsFromUserConfig();
+
+            $options->setIntegrations(static function (array $integrations) use ($options, $userIntegrations) {
+                $allIntegrations = array_merge($integrations, $userIntegrations);
+
+                if (!$options->hasDefaultIntegrations()) {
+                    return $allIntegrations;
+                }
 
                 // Remove the default error and fatal exception listeners to let Laravel handle those
                 // itself. These event are still bubbling up through the documented changes in the users
                 // `ExceptionHandler` of their application or through the log channel integration to Sentry
-                $options->setIntegrations(array_filter($integrations, static function (SdkIntegration\IntegrationInterface $integration): bool {
+                return array_filter($allIntegrations, static function (SdkIntegration\IntegrationInterface $integration): bool {
                     if ($integration instanceof SdkIntegration\ErrorListenerIntegration) {
                         return false;
                     }
@@ -155,8 +160,8 @@ protected function configureAndRegisterClient(): void
                     }
 
                     return true;
-                }));
-            }
+                });
+            });
 
             $hub = new Hub($clientBuilder->getClient());
 
@@ -185,7 +190,7 @@ protected function hasDsnSet(): bool
      *
      * @return array
      */
-    private function getIntegrations(): array
+    private function resolveIntegrationsFromUserConfig(): array
     {
         $integrations = [new Integration];
 
@@ -195,7 +200,13 @@ private function getIntegrations(): array
             if ($userIntegration instanceof SdkIntegration\IntegrationInterface) {
                 $integrations[] = $userIntegration;
             } elseif (\is_string($userIntegration)) {
-                $integrations[] = $this->app->make($userIntegration);
+                $resolvedIntegration = $this->app->make($userIntegration);
+
+                if (!($resolvedIntegration instanceof SdkIntegration\IntegrationInterface)) {
+                    throw new \RuntimeException('Sentry integrations should a instance of `\Sentry\Integration\IntegrationInterface`.');
+                }
+
+                $integrations[] = $resolvedIntegration;
             } else {
                 throw new \RuntimeException('Sentry integrations should either be a container reference or a instance of `\Sentry\Integration\IntegrationInterface`.');
             }

test/Sentry/IntegrationsOptionTest.php

@@ -3,6 +3,9 @@
 namespace Sentry\Laravel\Tests;
 
 use Sentry\Integration\IntegrationInterface;
+use Sentry\Integration\ErrorListenerIntegration;
+use Sentry\Integration\ExceptionListenerIntegration;
+use Sentry\Integration\FatalErrorListenerIntegration;
 
 class IntegrationsOptionTest extends SentryLaravelTestCase
 {
@@ -50,6 +53,7 @@ public function testCustomIntegrationByInstance()
 
     /**
      * Throws \ReflectionException in <=5.8 and \Illuminate\Contracts\Container\BindingResolutionException since 6.0
+     *
      * @expectedException \Exception
      */
     public function testCustomIntegrationThrowsExceptionIfNotResolvable()
@@ -73,6 +77,60 @@ static function () {
             ],
         ]);
     }
+
+    public function testDisabledIntegrationsAreNotPresent()
+    {
+        $integrations = $this->getHubFromContainer()->getClient()->getOptions()->getIntegrations();
+
+        foreach ($integrations as $integration) {
+            $this->ensureIsNotDisabledIntegration($integration);
+        }
+
+        $this->assertTrue(true, 'Not all disabled integrations are actually disabled.');
+    }
+
+    public function testDisabledIntegrationsAreNotPresentWithCustomIntegrations()
+    {
+        $this->resetApplicationWithConfig([
+            'sentry.integrations' => [
+                new IntegrationsOptionTestIntegrationStub,
+            ],
+        ]);
+
+        $integrations = $this->getHubFromContainer()->getClient()->getOptions()->getIntegrations();
+
+        $found = false;
+
+        foreach ($integrations as $integration) {
+            $this->ensureIsNotDisabledIntegration($integration);
+
+            if ($integration instanceof IntegrationsOptionTestIntegrationStub) {
+                $found = true;
+            }
+        }
+
+        $this->assertTrue($found, 'No IntegrationsOptionTestIntegrationStub found in final integrations enabled');
+    }
+
+    /**
+     * Make sure the passed integration is not one of the disabled integrations.
+     *
+     * @param \Sentry\Integration\IntegrationInterface $integration
+     */
+    private function ensureIsNotDisabledIntegration(IntegrationInterface $integration)
+    {
+        if ($integration instanceof ErrorListenerIntegration) {
+            $this->fail('Should not have ErrorListenerIntegration registered');
+        }
+
+        if ($integration instanceof ExceptionListenerIntegration) {
+            $this->fail('Should not have ExceptionListenerIntegration registered');
+        }
+
+        if ($integration instanceof FatalErrorListenerIntegration) {
+            $this->fail('Should not have FatalErrorListenerIntegration registered');
+        }
+    }
 }
 
 class IntegrationsOptionTestIntegrationStub implements IntegrationInterface