Fix CR issues

ste93cry · ste93cry · commit 27f602b82b93 · 2021-02-13T02:16:58.000+01:00
diff --git a/.github/workflows/static-analysis.yaml b/.github/workflows/static-analysis.yaml
@@ -17,8 +17,6 @@ jobs:
 
       - name: Setup PHP
         uses: shivammathur/setup-php@v2
-        with:
-          php-version: '7.4'
 
       - name: Install dependencies
         run: composer update --no-progress --no-interaction --prefer-dist
diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml
@@ -65,3 +65,59 @@ jobs:
         with:
           file: './coverage.xml'
           fail_ci_if_error: true
+
+  missing-optional-packages-tests:
+    name: Tests without optional packages
+    runs-on: ubuntu-latest
+    strategy:
+      fail-fast: false
+      matrix:
+        php:
+          - '7.2'
+          - '8.0'
+        dependencies:
+          - lowest
+          - highest
+
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v2
+
+      - name: Setup PHP
+        uses: shivammathur/setup-php@v2
+        with:
+          php-version: ${{ matrix.php }}
+          coverage: xdebug
+
+      - name: Setup Problem Matchers for PHPUnit
+        run: echo "::add-matcher::${{ runner.tool_cache }}/phpunit.json"
+
+      - name: Determine Composer cache directory
+        id: composer-cache
+        run: echo "::set-output name=directory::$(composer config cache-dir)"
+
+      - name: Cache Composer dependencies
+        uses: actions/cache@v2
+        with:
+          path: ${{ steps.composer-cache.outputs.directory }}
+          key: ${{ runner.os }}-${{ matrix.php }}-composer-${{ matrix.dependencies }}-${{ hashFiles('**/composer.lock') }}
+          restore-keys: ${{ runner.os }}-${{ matrix.php }}-composer-${{ matrix.dependencies }}-
+
+      - name: Remove optional packages
+        run: composer remove doctrine/dbal doctrine/doctrine-bundle symfony/messenger --dev --no-update
+
+      - name: Install highest dependencies
+        run: composer update --no-progress --no-interaction --prefer-dist
+        if: ${{ matrix.dependencies == 'highest' }}
+
+      - name: Install lowest dependencies
+        run: composer update --no-progress --no-interaction --prefer-dist --prefer-lowest
+        if: ${{ matrix.dependencies == 'lowest' }}
+
+      - name: Run tests
+        run: vendor/bin/phpunit --coverage-clover=build/coverage-report.xml
+
+      - name: Upload code coverage
+        uses: codecov/codecov-action@v1
+        with:
+          file: build/coverage-report.xml
diff --git a/composer.json b/composer.json
@@ -54,7 +54,7 @@
     },
     "suggest": {
         "monolog/monolog": "Allow sending log messages to Sentry by using the included Monolog handler.",
-        "doctrine/dbal": "Allow distributed tracing of SQL queries using Sentry."
+        "doctrine/doctrine-bundle": "Allow distributed tracing of database queries using Sentry."
     },
     "autoload": {
         "files": [
diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon
@@ -36,7 +36,17 @@ parameters:
 			path: src/Tracing/Doctrine/DBAL/TracingDriver.php
 
 		-
-			message: "#^Method Doctrine\\\\DBAL\\\\Driver\\:\\:connect\\(\\) invoked with 4 parameters, 1 required\\.$#"
+			message: "#^Call to an undefined method Doctrine\\\\DBAL\\\\Driver\\|Sentry\\\\SentryBundle\\\\Tracing\\\\Doctrine\\\\DBAL\\\\Compatibility\\\\ExceptionConverterDriverInterface\\:\\:connect\\(\\)\\.$#"
+			count: 1
+			path: src/Tracing/Doctrine/DBAL/TracingDriver.php
+
+		-
+			message: "#^Call to an undefined method Doctrine\\\\DBAL\\\\Driver\\|Sentry\\\\SentryBundle\\\\Tracing\\\\Doctrine\\\\DBAL\\\\Compatibility\\\\ExceptionConverterDriverInterface\\:\\:getDatabasePlatform\\(\\)\\.$#"
+			count: 2
+			path: src/Tracing/Doctrine/DBAL/TracingDriver.php
+
+		-
+			message: "#^Call to an undefined method Doctrine\\\\DBAL\\\\Driver\\|Sentry\\\\SentryBundle\\\\Tracing\\\\Doctrine\\\\DBAL\\\\Compatibility\\\\ExceptionConverterDriverInterface\\:\\:getSchemaManager\\(\\)\\.$#"
 			count: 1
 			path: src/Tracing/Doctrine/DBAL/TracingDriver.php
 
@@ -206,7 +216,7 @@ parameters:
 			path: tests/Tracing/Doctrine/DBAL/TracingDriverTest.php
 
 		-
-			message: "#^Trying to mock an undefined method convertException\\(\\) on class Sentry\\\\SentryBundle\\\\Tests\\\\Tracing\\\\Doctrine\\\\DBAL\\\\ExceptionConverterDriverInterface\\.$#"
+			message: "#^Trying to mock an undefined method convertException\\(\\) on class Sentry\\\\SentryBundle\\\\Tests\\\\Tracing\\\\Doctrine\\\\DBAL\\\\StubExceptionConverterDriverInterface\\.$#"
 			count: 1
 			path: tests/Tracing/Doctrine/DBAL/TracingDriverTest.php
 
diff --git a/src/DependencyInjection/SentryExtension.php b/src/DependencyInjection/SentryExtension.php
@@ -17,6 +17,7 @@
 use Sentry\SentryBundle\EventListener\ErrorListener;
 use Sentry\SentryBundle\EventListener\MessengerListener;
 use Sentry\SentryBundle\SentryBundle;
+use Sentry\SentryBundle\Tracing\Doctrine\DBAL\ConnectionConfigurator;
 use Sentry\SentryBundle\Tracing\Doctrine\DBAL\TracingDriverMiddleware;
 use Sentry\Serializer\RepresentationSerializer;
 use Sentry\Serializer\Serializer;
@@ -167,6 +168,7 @@ private function registerTracingConfiguration(ContainerBuilder $container, array
         $container->setParameter('sentry.tracing.dbal.connections', $isConfigEnabled ? $config['dbal']['connections'] : []);
 
         if (!$isConfigEnabled) {
+            $container->removeDefinition(ConnectionConfigurator::class);
             $container->removeDefinition(TracingDriverMiddleware::class);
         }
     }
diff --git a/src/Tracing/Doctrine/DBAL/Compatibility/DriverInterface.php b/src/Tracing/Doctrine/DBAL/Compatibility/DriverInterface.php
@@ -0,0 +1,12 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Sentry\SentryBundle\Tracing\Doctrine\DBAL\Compatibility;
+
+/**
+ * @internal
+ */
+interface DriverInterface
+{
+}
diff --git a/src/Tracing/Doctrine/DBAL/Compatibility/ExceptionConverterDriverInterface.php b/src/Tracing/Doctrine/DBAL/Compatibility/ExceptionConverterDriverInterface.php
@@ -4,11 +4,9 @@
 
 namespace Sentry\SentryBundle\Tracing\Doctrine\DBAL\Compatibility;
 
-use Doctrine\DBAL\Driver as DriverInterface;
-
 /**
  * @internal
  */
-interface ExceptionConverterDriverInterface extends DriverInterface
+interface ExceptionConverterDriverInterface
 {
 }
diff --git a/src/Tracing/Doctrine/DBAL/TracingDriver.php b/src/Tracing/Doctrine/DBAL/TracingDriver.php
@@ -19,7 +19,7 @@
  * This is a simple implementation of the {@see DriverInterface} interface that
  * decorates an existing driver to support distributed tracing capabilities.
  * This implementation IS and MUST be compatible with all versions of Doctrine
- * DBAL >= 2.11.
+ * DBAL >= 2.10.
  *
  * @internal
  */
diff --git a/src/Tracing/Doctrine/DBAL/TracingDriverConnection.php b/src/Tracing/Doctrine/DBAL/TracingDriverConnection.php
@@ -13,7 +13,8 @@
 
 /**
  * This implementation wraps a driver connection and adds distributed tracing
- * capabilities to Doctrine DBAL.
+ * capabilities to Doctrine DBAL. This implementation IS and MUST be compatible
+ * with all versions of Doctrine DBAL >= 2.10.
  */
 final class TracingDriverConnection implements DriverConnectionInterface
 {
@@ -45,7 +46,7 @@ final class TracingDriverConnection implements DriverConnectionInterface
     /**
      * @internal
      */
-    public const SPAN_OP_TRANSACTION_ROLLBACK = 'sql.transaciton.rollback';
+    public const SPAN_OP_TRANSACTION_ROLLBACK = 'sql.transaction.rollback';
 
     /**
      * @var HubInterface The current hub
diff --git a/src/aliases.php b/src/aliases.php
@@ -4,7 +4,7 @@
 
 namespace Sentry\SentryBundle;
 
-use Doctrine\DBAL\Connection;
+use Doctrine\DBAL\Driver as DoctrineDriverInterface;
 use Doctrine\DBAL\Driver\DriverException as LegacyDriverExceptionInterface;
 use Doctrine\DBAL\Driver\Exception as DriverExceptionInterface;
 use Doctrine\DBAL\Driver\ExceptionConverterDriver as LegacyExceptionConverterDriverInterface;
@@ -15,6 +15,7 @@
 use Sentry\SentryBundle\EventListener\RequestListenerControllerEvent;
 use Sentry\SentryBundle\EventListener\RequestListenerRequestEvent;
 use Sentry\SentryBundle\EventListener\SubRequestListenerRequestEvent;
+use Sentry\SentryBundle\Tracing\Doctrine\DBAL\Compatibility\DriverInterface;
 use Sentry\SentryBundle\Tracing\Doctrine\DBAL\Compatibility\ExceptionConverterDriverInterface;
 use Sentry\SentryBundle\Tracing\Doctrine\DBAL\Compatibility\MiddlewareInterface;
 use Symfony\Component\HttpKernel\Event\ControllerEvent;
@@ -67,24 +68,27 @@ class_alias(GetResponseEvent::class, SubRequestListenerRequestEvent::class);
     }
 }
 
-if (class_exists(Connection::class)) {
-    if (!interface_exists(Result::class)) {
-        /** @psalm-suppress UndefinedClass */
-        class_alias(Statement::class, Result::class);
-    }
+if (interface_exists(Statement::class) && !interface_exists(Result::class)) {
+    /** @psalm-suppress UndefinedClass */
+    class_alias(Statement::class, Result::class);
+}
 
-    if (!interface_exists(DoctrineMiddlewareInterface::class)) {
-        /** @psalm-suppress UndefinedClass */
-        class_alias(MiddlewareInterface::class, DoctrineMiddlewareInterface::class);
-    }
+if (interface_exists(DriverExceptionInterface::class) && !interface_exists(LegacyDriverExceptionInterface::class)) {
+    /** @psalm-suppress UndefinedClass */
+    class_alias(DriverExceptionInterface::class, LegacyDriverExceptionInterface::class);
+}
 
-    if (!interface_exists(LegacyExceptionConverterDriverInterface::class)) {
-        /** @psalm-suppress UndefinedClass */
-        class_alias(ExceptionConverterDriverInterface::class, LegacyExceptionConverterDriverInterface::class);
-    }
+if (!interface_exists(DoctrineMiddlewareInterface::class)) {
+    /** @psalm-suppress UndefinedClass */
+    class_alias(MiddlewareInterface::class, DoctrineMiddlewareInterface::class);
+}
 
-    if (!interface_exists(LegacyDriverExceptionInterface::class)) {
-        /** @psalm-suppress UndefinedClass */
-        class_alias(DriverExceptionInterface::class, LegacyDriverExceptionInterface::class);
-    }
+if (!interface_exists(DoctrineDriverInterface::class)) {
+    /** @psalm-suppress UndefinedClass */
+    class_alias(DriverInterface::class, DoctrineDriverInterface::class);
+}
+
+if (!interface_exists(LegacyExceptionConverterDriverInterface::class)) {
+    /** @psalm-suppress UndefinedClass */
+    class_alias(ExceptionConverterDriverInterface::class, LegacyExceptionConverterDriverInterface::class);
 }
diff --git a/tests/DependencyInjection/Compiler/DbalTracingPassTest.php b/tests/DependencyInjection/Compiler/DbalTracingPassTest.php
@@ -18,7 +18,7 @@ final class DbalTracingPassTest extends DoctrineTestCase
 {
     public function testProcessWithDoctrineDBALVersionAtLeast30(): void
     {
-        if (!$this->isDoctrineDBALVersion3Installed()) {
+        if (!self::isDoctrineDBALVersion3Installed()) {
             $this->markTestSkipped('This test requires the version of the "doctrine/dbal" Composer package to be >= 3.0.');
         }
 
@@ -84,7 +84,7 @@ public function testProcessWithDoctrineDBALVersionAtLeast30(): void
 
     public function testProcessWithDoctrineDBALVersionLowerThan30(): void
     {
-        if ($this->isDoctrineDBALVersion3Installed()) {
+        if (self::isDoctrineDBALVersion3Installed()) {
             $this->markTestSkipped('This test requires the version of the "doctrine/dbal" Composer package to be < 3.0.');
         }
 
diff --git a/tests/DependencyInjection/ConfigurationTest.php b/tests/DependencyInjection/ConfigurationTest.php
@@ -4,6 +4,7 @@
 
 namespace Sentry\SentryBundle\Tests\DependencyInjection;
 
+use Doctrine\Bundle\DoctrineBundle\DoctrineBundle;
 use Jean85\PrettyVersions;
 use PHPUnit\Framework\TestCase;
 use Sentry\SentryBundle\DependencyInjection\Configuration;
@@ -38,7 +39,7 @@ public function testProcessConfigurationWithDefaultConfiguration(): void
             'tracing' => [
                 'dbal' => [
                     'enabled' => false,
-                    'connections' => ['%doctrine.default_connection%'],
+                    'connections' => class_exists(DoctrineBundle::class) ? ['%doctrine.default_connection%'] : [],
                 ],
             ],
         ];
diff --git a/tests/DependencyInjection/Fixtures/php/dbal_tracing_enabled.php b/tests/DependencyInjection/Fixtures/php/dbal_tracing_enabled.php
@@ -8,7 +8,7 @@
 $container->loadFromExtension('sentry', [
     'tracing' => [
         'dbal' => [
-            'enabled' => false,
+            'enabled' => true,
             'connections' => ['default'],
         ],
     ],
diff --git a/tests/DependencyInjection/Fixtures/php/full.php b/tests/DependencyInjection/Fixtures/php/full.php
@@ -44,7 +44,8 @@
     ],
     'tracing' => [
         'dbal' => [
-            'enabled' => true,
+            'enabled' => false,
+            'connections' => ['default'],
         ],
     ],
 ]);
diff --git a/tests/DependencyInjection/Fixtures/xml/dbal_tracing_enabled.xml b/tests/DependencyInjection/Fixtures/xml/dbal_tracing_enabled.xml
@@ -8,7 +8,7 @@
 
     <sentry:config>
         <sentry:tracing>
-            <sentry:dbal enabled="false">
+            <sentry:dbal enabled="true">
                 <sentry:connection>default</sentry:connection>
             </sentry:dbal>
         </sentry:tracing>
diff --git a/tests/DependencyInjection/Fixtures/xml/full.xml b/tests/DependencyInjection/Fixtures/xml/full.xml
@@ -38,7 +38,9 @@
         </sentry:options>
         <sentry:messenger enabled="true" capture-soft-fails="false" />
         <sentry:tracing>
-            <sentry:dbal enabled="true" />
+            <sentry:dbal enabled="false">
+                <sentry:connection>default</sentry:connection>
+            </sentry:dbal>
         </sentry:tracing>
     </sentry:config>
 </container>
diff --git a/tests/DependencyInjection/Fixtures/yml/dbal_tracing_enabled.yml b/tests/DependencyInjection/Fixtures/yml/dbal_tracing_enabled.yml
@@ -1,6 +1,6 @@
 sentry:
     tracing:
         dbal:
-            enabled: false
+            enabled: true
             connections:
                 - default
diff --git a/tests/DependencyInjection/Fixtures/yml/full.yml b/tests/DependencyInjection/Fixtures/yml/full.yml
@@ -39,4 +39,6 @@ sentry:
         capture_soft_fails: false
     tracing:
         dbal:
-            enabled: true
+            enabled: false
+            connections:
+                - enabled
diff --git a/tests/DependencyInjection/SentryExtensionTest.php b/tests/DependencyInjection/SentryExtensionTest.php
@@ -16,6 +16,7 @@
 use Sentry\SentryBundle\EventListener\RequestListener;
 use Sentry\SentryBundle\EventListener\SubRequestListener;
 use Sentry\SentryBundle\SentryBundle;
+use Sentry\SentryBundle\Tracing\Doctrine\DBAL\ConnectionConfigurator;
 use Sentry\SentryBundle\Tracing\Doctrine\DBAL\TracingDriverMiddleware;
 use Sentry\Serializer\RepresentationSerializer;
 use Sentry\Serializer\Serializer;
@@ -264,16 +265,19 @@ public function testIgnoreErrorsIntegrationIsNotAddedTwiceIfAlreadyConfigured():
 
     public function testTracingDriverMiddlewareIsConfiguredWhenDbalTracingIsEnable(): void
     {
-        $container = $this->createContainerFromFixture('full');
+        $container = $this->createContainerFromFixture('dbal_tracing_enabled');
 
         $this->assertTrue($container->hasDefinition(TracingDriverMiddleware::class));
+        $this->assertTrue($container->hasDefinition(ConnectionConfigurator::class));
+        $this->assertNotEmpty($container->getParameter('sentry.tracing.dbal.connections'));
     }
 
     public function testTracingDriverMiddlewareIsRemovedWhenDbalTracingIsDisabled(): void
     {
-        $container = $this->createContainerFromFixture('dbal_tracing_disabled');
+        $container = $this->createContainerFromFixture('full');
 
         $this->assertFalse($container->hasDefinition(TracingDriverMiddleware::class));
+        $this->assertFalse($container->hasDefinition(ConnectionConfigurator::class));
         $this->assertEmpty($container->getParameter('sentry.tracing.dbal.connections'));
     }
 
diff --git a/tests/DoctrineTestCase.php b/tests/DoctrineTestCase.php
@@ -4,13 +4,19 @@
 
 namespace Sentry\SentryBundle\Tests;
 
+use Doctrine\Bundle\DoctrineBundle\DoctrineBundle;
 use Doctrine\DBAL\Driver\ResultStatement;
 use PHPUnit\Framework\TestCase;
 
 abstract class DoctrineTestCase extends TestCase
 {
-    protected function isDoctrineDBALVersion3Installed(): bool
+    protected static function isDoctrineDBALVersion3Installed(): bool
     {
         return !interface_exists(ResultStatement::class);
     }
+
+    protected static function isDoctrineBundlePackageInstalled(): bool
+    {
+        return class_exists(DoctrineBundle::class);
+    }
 }
diff --git a/tests/Tracing/Doctrine/DBAL/TracingDriverConnectionTest.php b/tests/Tracing/Doctrine/DBAL/TracingDriverConnectionTest.php
diff --git a/tests/Tracing/Doctrine/DBAL/TracingDriverMiddlewareTest.php b/tests/Tracing/Doctrine/DBAL/TracingDriverMiddlewareTest.php
diff --git a/tests/Tracing/Doctrine/DBAL/TracingDriverTest.php b/tests/Tracing/Doctrine/DBAL/TracingDriverTest.php

Original file line number	Diff line number	Diff line change
`@@ -17,6 +17,7 @@`
`17`	`17`	`use Sentry\SentryBundle\EventListener\ErrorListener;`
`18`	`18`	`use Sentry\SentryBundle\EventListener\MessengerListener;`
`19`	`19`	`use Sentry\SentryBundle\SentryBundle;`
	`20`	`+use Sentry\SentryBundle\Tracing\Doctrine\DBAL\ConnectionConfigurator;`
`20`	`21`	`use Sentry\SentryBundle\Tracing\Doctrine\DBAL\TracingDriverMiddleware;`
`21`	`22`	`use Sentry\Serializer\RepresentationSerializer;`
`22`	`23`	`use Sentry\Serializer\Serializer;`
`@@ -167,6 +168,7 @@ private function registerTracingConfiguration(ContainerBuilder $container, array`
`167`	`168`	`$container->setParameter('sentry.tracing.dbal.connections', $isConfigEnabled ? $config['dbal']['connections'] : []);`
`168`	`169`
`169`	`170`	`if (!$isConfigEnabled) {`
	`171`	`+ $container->removeDefinition(ConnectionConfigurator::class);`
`170`	`172`	`$container->removeDefinition(TracingDriverMiddleware::class);`
`171`	`173`	`}`
`172`	`174`	`}`
Original file line number	Diff line number	Diff line change
`@@ -4,11 +4,9 @@`
`4`	`4`
`5`	`5`	`namespace Sentry\SentryBundle\Tracing\Doctrine\DBAL\Compatibility;`
`6`	`6`
`7`		`-use Doctrine\DBAL\Driver as DriverInterface;`
`8`		`-`
`9`	`7`	`/**`
`10`	`8`	`* @internal`
`11`	`9`	`*/`
`12`		`-interface ExceptionConverterDriverInterface extends DriverInterface`
	`10`	`+interface ExceptionConverterDriverInterface`
`13`	`11`	`{`
`14`	`12`	`}`
Original file line number	Diff line number	Diff line change
`@@ -19,7 +19,7 @@`
`19`	`19`	`* This is a simple implementation of the {@see DriverInterface} interface that`
`20`	`20`	`* decorates an existing driver to support distributed tracing capabilities.`
`21`	`21`	`* This implementation IS and MUST be compatible with all versions of Doctrine`
`22`		`- * DBAL >= 2.11.`
	`22`	`+ * DBAL >= 2.10.`
`23`	`23`	`*`
`24`	`24`	`* @internal`
`25`	`25`	`*/`