Enable error handlers back (#322)

* Reinforce E2E tests with log of sent events * Add failing test for notices * Fix wrong setup in test * Improve E2E tests; add case for fatals * Try to revert the integration disabling to have the full error reporting back * Fix test after last modification * Require --dev symfony/process for client isolation * Do not capture deprecations in E2E tests * Fix CS * Fix PHPStan * Fix last PHPStan error * Remove unneeded alias * Remove unused class * Try to avoid double reporting of fatal errors * Add changelog entry * Fix after-merge issues
getsentry · May 15, 2020 · 06960f1 · 06960f1
1 parent 02e6902
commit 06960f1
Show file tree

Hide file tree

Showing 12 changed files with 152 additions and 65 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ## Unreleased
  - ...
 
+## 4.0.0 (TBA)
+ - Enable back all error listeners from base SDK integration (#322)
+
 ## 3.5.1 (2020-05-07)
  - Capture events using the `Hub` in the `MessengerListener` to avoid loosing `Scope` data (#339, thanks to @sh41)
  - Capture multiple events if multiple exceptions are generated in a Messenger Worker context (#340, thanks to @emarref)

diff --git a/composer.json b/composer.json
@@ -21,6 +21,7 @@
     "require": {
         "php": "^7.1",
         "jean85/pretty-package-versions": "^1.0",
+        "ocramius/package-versions": "^1.3.0",
         "sentry/sdk": "^2.1",
         "symfony/config": "^3.4||^4.0||^5.0",
         "symfony/console": "^3.4||^4.0||^5.0",
@@ -44,6 +45,7 @@
         "symfony/messenger": "^4.3||^5.0",
         "symfony/monolog-bundle": "^3.4",
         "symfony/phpunit-bridge": "^5.0",
+        "symfony/process": "^3.4||^4.0||^5.0",
         "symfony/yaml": "^3.4||^4.0||^5.0"
     },
     "conflict": {

diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon
@@ -235,11 +235,6 @@ parameters:
 			count: 1
 			path: test/End2End/App/Kernel.php
 
-		-
-			message: "#^Class PHPUnit_Framework_TestCase not found\\.$#"
-			count: 1
-			path: test/End2End/End2EndTest.php
-
 		-
 			message: "#^Class Symfony\\\\Bundle\\\\FrameworkBundle\\\\Client not found\\.$#"
 			count: 1

diff --git a/src/DependencyInjection/IntegrationFilterFactory.php b/src/DependencyInjection/IntegrationFilterFactory.php
diff --git a/src/DependencyInjection/SentryExtension.php b/src/DependencyInjection/SentryExtension.php
@@ -17,6 +17,7 @@
 use Symfony\Component\DependencyInjection\Exception\LogicException;
 use Symfony\Component\DependencyInjection\Loader;
 use Symfony\Component\DependencyInjection\Reference;
+use Symfony\Component\ErrorHandler\Error\FatalError;
 use Symfony\Component\HttpKernel\DependencyInjection\Extension;
 use Symfony\Component\HttpKernel\Event\ExceptionEvent;
 use Symfony\Component\HttpKernel\KernelEvents;
@@ -128,18 +129,15 @@ private function passConfigurationToOptions(ContainerBuilder $container, array $
             }
         }
 
-        if (\array_key_exists('excluded_exceptions', $processedOptions) && $processedOptions['excluded_exceptions']) {
-            $ignoreOptions = [
-                'ignore_exceptions' => $processedOptions['excluded_exceptions'],
-            ];
-
-            $integrations[] = new Definition(IgnoreErrorsIntegration::class, [$ignoreOptions]);
-        }
+        // we ignore fatal errors wrapped by Symfony because they produce double event reporting
+        $processedOptions['excluded_exceptions'][] = FatalError::class;
+        $ignoreOptions = [
+            'ignore_exceptions' => $processedOptions['excluded_exceptions'],
+        ];
 
-        $integrationsCallable = new Definition('callable', [$integrations]);
-        $integrationsCallable->setFactory([IntegrationFilterFactory::class, 'create']);
+        $integrations[] = new Definition(IgnoreErrorsIntegration::class, [$ignoreOptions]);
 
-        $options->addMethodCall('setIntegrations', [$integrationsCallable]);
+        $options->addMethodCall('setIntegrations', [$integrations]);
     }
 
     /**

diff --git a/test/DependencyInjection/SentryExtensionTest.php b/test/DependencyInjection/SentryExtensionTest.php
@@ -8,8 +8,6 @@
 use Sentry\Breadcrumb;
 use Sentry\ClientInterface;
 use Sentry\Event;
-use Sentry\Integration\ErrorListenerIntegration;
-use Sentry\Integration\ExceptionListenerIntegration;
 use Sentry\Integration\IntegrationInterface;
 use Sentry\Monolog\Handler;
 use Sentry\Options;
@@ -339,14 +337,6 @@ public function testIntegrations(): void
 
         $found = false;
         foreach ($integrations as $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 IntegrationMock) {
                 $found = true;
             }

diff --git a/test/End2End/App/Controller/MainController.php b/test/End2End/App/Controller/MainController.php
@@ -31,13 +31,27 @@ public function exception(): Response
         throw new \RuntimeException('This is an intentional error');
     }
 
+    public function fatal(): Response
+    {
+        $foo = eval("return new class() implements \Serializable {};");
+
+        return new Response('This response should not happen: ' . json_encode($foo));
+    }
+
     public function index(): Response
     {
         $this->sentry->captureMessage('Hello there');
 
         return new Response('Hello there');
     }
 
+    public function notice(): Response
+    {
+        @trigger_error('This is an intentional notice', E_USER_NOTICE);
+
+        return new Response('Hello there');
+    }
+
     public function subrequest(): Response
     {
         $request = $this->requestStack->getCurrentRequest();

diff --git a/test/End2End/App/config.yml b/test/End2End/App/config.yml
@@ -1,3 +1,8 @@
+sentry:
+  options:
+    capture_silenced_errors: true
+    error_types: E_ALL & ~E_USER_DEPRECATED
+
 framework:
   router: { resource: "%routing_config_dir%/routing.yml" }
   secret: secret
@@ -8,6 +13,17 @@ services:
     alias: Sentry\State\HubInterface
     public: true
 
+  Sentry\ClientBuilderInterface:
+    class: Sentry\ClientBuilder
+    arguments: 
+      - '@Sentry\Options'
+    calls:
+      - method: setTransportFactory
+        arguments:
+          - '@Sentry\SentryBundle\Test\End2End\StubTransportFactory'
+
+  Sentry\SentryBundle\Test\End2End\StubTransportFactory: ~
+
   Sentry\SentryBundle\Test\End2End\App\Controller\MainController:
     autowire: true
     tags:

diff --git a/test/End2End/App/routing.yml b/test/End2End/App/routing.yml
@@ -2,6 +2,10 @@ exception:
   path:  /exception
   defaults: { _controller: 'Sentry\SentryBundle\Test\End2End\App\Controller\MainController::exception' }
 
+fatal:
+  path:  /fatal
+  defaults: { _controller: 'Sentry\SentryBundle\Test\End2End\App\Controller\MainController::fatal' }
+
 200:
   path:  /200
   defaults: { _controller: 'Sentry\SentryBundle\Test\End2End\App\Controller\MainController::index' }
@@ -13,3 +17,7 @@ secured200:
 subrequest:
   path:  /subrequest
   defaults: { _controller: 'Sentry\SentryBundle\Test\End2End\App\Controller\MainController::subrequest' }
+
+notice:
+  path:  /notice
+  defaults: { _controller: 'Sentry\SentryBundle\Test\End2End\App\Controller\MainController::notice' }
diff --git a/test/End2End/End2EndTest.php b/test/End2End/End2EndTest.php
@@ -2,7 +2,6 @@
 
 namespace Sentry\SentryBundle\Test\End2End;
 
-use PHPUnit\Framework\TestCase;
 use Sentry\SentryBundle\Test\End2End\App\Kernel;
 use Sentry\State\HubInterface;
 use Symfony\Bundle\FrameworkBundle\Client;
@@ -11,7 +10,6 @@
 use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
 
-class_alias(TestCase::class, \PHPUnit_Framework_TestCase::class);
 if (! class_exists(KernelBrowser::class)) {
     class_alias(Client::class, KernelBrowser::class);
 }
@@ -21,11 +19,20 @@ class_alias(Client::class, KernelBrowser::class);
  */
 class End2EndTest extends WebTestCase
 {
+    public const SENT_EVENTS_LOG = '/tmp/sentry_e2e_test_sent_events.log';
+
     protected static function getKernelClass(): string
     {
         return Kernel::class;
     }
 
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        file_put_contents(self::SENT_EVENTS_LOG, '');
+    }
+
     public function testGet200(): void
     {
         $client = static::createClient(['debug' => false]);
@@ -38,6 +45,7 @@ public function testGet200(): void
         $this->assertSame(200, $response->getStatusCode());
 
         $this->assertLastEventIdIsNotNull($client);
+        $this->assertEventCount(1);
     }
 
     public function testGet200BehindFirewall(): void
@@ -52,6 +60,7 @@ public function testGet200BehindFirewall(): void
         $this->assertSame(200, $response->getStatusCode());
 
         $this->assertLastEventIdIsNotNull($client);
+        $this->assertEventCount(1);
     }
 
     public function testGet200WithSubrequest(): void
@@ -66,6 +75,7 @@ public function testGet200WithSubrequest(): void
         $this->assertSame(200, $response->getStatusCode());
 
         $this->assertLastEventIdIsNotNull($client);
+        $this->assertEventCount(1);
     }
 
     public function testGet404(): void
@@ -88,6 +98,7 @@ public function testGet404(): void
         }
 
         $this->assertLastEventIdIsNotNull($client);
+        $this->assertEventCount(1);
     }
 
     public function testGet500(): void
@@ -111,6 +122,44 @@ public function testGet500(): void
         }
 
         $this->assertLastEventIdIsNotNull($client);
+        $this->assertEventCount(1);
+    }
+
+    public function testGetFatal(): void
+    {
+        $client = static::createClient();
+
+        try {
+            $client->insulate(true);
+            $client->request('GET', '/fatal');
+
+            $response = $client->getResponse();
+
+            $this->assertInstanceOf(Response::class, $response);
+            $this->assertSame(500, $response->getStatusCode());
+            $this->assertStringNotContainsString('not happen', $response->getContent() ?: '');
+        } catch (\RuntimeException $exception) {
+            $this->assertStringContainsStringIgnoringCase('error', $exception->getMessage());
+            $this->assertStringContainsStringIgnoringCase('contains 2 abstract methods', $exception->getMessage());
+            $this->assertStringContainsStringIgnoringCase('MainController.php', $exception->getMessage());
+            $this->assertStringContainsStringIgnoringCase('eval()\'d code on line', $exception->getMessage());
+        }
+
+        $this->assertEventCount(1);
+    }
+
+    public function testNotice(): void
+    {
+        $client = static::createClient();
+        $client->request('GET', '/notice');
+
+        $response = $client->getResponse();
+
+        $this->assertInstanceOf(Response::class, $response);
+        $this->assertSame(200, $response->getStatusCode());
+
+        $this->assertLastEventIdIsNotNull($client);
+        $this->assertEventCount(1);
     }
 
     private function assertLastEventIdIsNotNull(KernelBrowser $client): void
@@ -123,4 +172,12 @@ private function assertLastEventIdIsNotNull(KernelBrowser $client): void
 
         $this->assertNotNull($hub->getLastEventId(), 'Last error not captured');
     }
+
+    private function assertEventCount(int $expectedCount): void
+    {
+        $events = file_get_contents(self::SENT_EVENTS_LOG);
+        $this->assertNotFalse($events, 'Cannot read sent events log');
+        $listOfEvents = array_filter(explode(StubTransportFactory::SEPARATOR, trim($events)));
+        $this->assertCount($expectedCount, $listOfEvents, 'Wrong number of events sent: ' . PHP_EOL . $events);
+    }
 }
diff --git a/test/End2End/StubTransportFactory.php b/test/End2End/StubTransportFactory.php
@@ -0,0 +1,39 @@
+<?php
+
+namespace Sentry\SentryBundle\Test\End2End;
+
+use Sentry\Event;
+use Sentry\Options;
+use Sentry\Transport\TransportFactoryInterface;
+use Sentry\Transport\TransportInterface;
+
+class StubTransportFactory implements TransportFactoryInterface
+{
+    public const SEPARATOR = '###';
+
+    public function create(Options $options): TransportInterface
+    {
+        return new class() implements TransportInterface {
+            public function send(Event $event): ?string
+            {
+                touch(End2EndTest::SENT_EVENTS_LOG);
+
+                if ($event->getMessage()) {
+                    $message = $event->getMessage();
+                } elseif ($event->getExceptions()) {
+                    $message = $event->getExceptions()[0]['value'];
+                } else {
+                    $message = 'NO MESSAGE NOR EXCEPTIONS';
+                }
+
+                file_put_contents(
+                    End2EndTest::SENT_EVENTS_LOG,
+                    $event->getId() . ': ' . $message . PHP_EOL . StubTransportFactory::SEPARATOR . PHP_EOL,
+                    FILE_APPEND
+                );
+
+                return $event->getId();
+            }
+        };
+    }
+}
diff --git a/test/SentryBundleTest.php b/test/SentryBundleTest.php
@@ -127,16 +127,16 @@ public function testContainerHasTestCommandRegisteredCorrectly(): void
         $this->assertArrayHasKey('console.command', $consoleListener->getTags());
     }
 
-    public function testIntegrationsListenersAreDisabledByDefault(): void
+    public function testIntegrationsListenersAreEnabled(): void
     {
         $container = $this->getContainer();
 
         $hub = $container->get(HubInterface::class);
 
         $this->assertInstanceOf(HubInterface::class, $hub);
         $this->assertInstanceOf(IntegrationInterface::class, $hub->getIntegration(RequestIntegration::class));
-        $this->assertNull($hub->getIntegration(ErrorListenerIntegration::class));
-        $this->assertNull($hub->getIntegration(ExceptionListenerIntegration::class));
+        $this->assertNotNull($hub->getIntegration(ErrorListenerIntegration::class));
+        $this->assertNotNull($hub->getIntegration(ExceptionListenerIntegration::class));
     }
 
     private function getContainer(): ContainerBuilder