Implement error logging and better testing set up

Author: Maxime Rainville

README.md

@@ -236,22 +236,45 @@ return `null` if the DataObject has been deleted.
 `DataObjectEvent::getObject(true) will attempt to retrieve the exact version of the DataObject that fired the event,
 assuming it was versioned.
 
-## Testing Your Events
+## Handling Errors in Event Listeners
 
-### Writing Event Tests
+Exceptions thrown by an event listener will not stop the execution of follow events. By default, those exceptions will be sent to `EventService::handleError()` who will logged them to the default Silverstripe CMS logger.
+
+You can provide your own error handler with Injector.
+
+```yml
+---
+Name: custom-event-service
+After:
+  - '#event-service'
+---
+SilverStripe\Core\Injector\Injector:
+  ArchiPro\EventDispatcher\AsyncEventDispatcher:
+      errorhandler: [MyCustomEventHandler, handleError]
+```
+
+## Testing your Events
 
 When testing your event listeners, you'll need to:
 1. Dispatch your events
 2. Run the event loop
 3. Assert the expected outcomes
 
+You can also use the `TestEventService` to test your events. The `TestEventService` will replace the default `EventService` and log any exceptions thrown by listeners.
+
+You need to require the `colinodell/psr-testlogger` package in your dev dependencies to use the `TestEventService`.
+
+```
+composer require colinodell/psr-testlogger --dev
+```
+
 Here's an example test:
 
 ```php
 use Revolt\EventLoop;
 use SilverStripe\Dev\SapphireTest;
 use SilverStripe\Core\Injector\Injector;
-use ArchiPro\Silverstripe\EventDispatcher\Service\EventService;
+use ArchiPro\Silverstripe\EventDispatcher\Service\TestEventService;
 
 class MyEventTest extends SapphireTest
 {
@@ -260,8 +283,9 @@ class MyEventTest extends SapphireTest
         // Create your test event
         $event = new MyCustomEvent('test message');
         
-        // Get the event service
-        $service = Injector::inst()->get(EventService::class);
+        // Get the Test Event Service ... this will replace the default EventService with a TestEventService
+        // with an implementation that will log errors to help with debugging.
+        $service = TestEventService::bootstrap();
         
         // Add your test listener ... or if you have already
         $wasCalled = false;
@@ -281,6 +305,12 @@ class MyEventTest extends SapphireTest
             MyCustomEventListener::wasCalled(),
             'Assert some side effect of the event being handled'
         );
+
+        $this->assertCount(
+            0,
+            $service->getTestLogger()->records,
+            'No errors were logged'
+        );
     }
 }
 ```

_config/events.yml

@@ -1,23 +1,24 @@
 ---
-Name: events
+Name: event-service
 After:
   - '#coreservices'
 ---
 SilverStripe\Core\Injector\Injector:
   # Define the listener provider
   ArchiPro\EventDispatcher\ListenerProvider:
     class: ArchiPro\EventDispatcher\ListenerProvider
-  
+
   # Default event dispatcher
   ArchiPro\EventDispatcher\AsyncEventDispatcher:
     class: ArchiPro\EventDispatcher\AsyncEventDispatcher
     constructor:
       listenerProvider: '%$ArchiPro\EventDispatcher\ListenerProvider'
+      errorhandler: [ArchiPro\Silverstripe\EventDispatcher\Service\EventService, handleError]
   Psr\EventDispatcher\EventDispatcherInterface:
     alias: '%$ArchiPro\EventDispatcher\AsyncEventDispatcher'
 
   # Bootstrap the event service
   ArchiPro\Silverstripe\EventDispatcher\Service\EventService:
-    constructor: 
+    constructor:
       dispatcher: '%$ArchiPro\EventDispatcher\AsyncEventDispatcher'
-      listenerProvider: '%$ArchiPro\EventDispatcher\ListenerProvider'
+      listenerProvider: '%$ArchiPro\EventDispatcher\ListenerProvider'

composer.json

@@ -9,13 +9,15 @@
         "silverstripe/versioned": "^1.13 || ^2.0",
         "psr/event-dispatcher": "^1.0",
         "psr/event-dispatcher-implementation": "^1.0",
-        "archipro/revolt-event-dispatcher": "^0.0.0"
+        "archipro/revolt-event-dispatcher": "^0.1.0",
+        "psr/log": "^1 || ^2 || ^3"
     },
     "require-dev": {
         "phpunit/phpunit": "^9.5",
         "squizlabs/php_codesniffer": "^3.0",
         "friendsofphp/php-cs-fixer": "^3.0",
-        "phpstan/phpstan": "^1.10"
+        "phpstan/phpstan": "^1.10",
+        "colinodell/psr-testlogger": "^1.0"
     },
     "autoload": {
         "psr-4": {
@@ -43,5 +45,8 @@
             "composer/installers": true,
             "silverstripe/vendor-plugin": true
         }
+    },
+    "suggest": {
+        "colinodell/psr-testlogger": "To use the TestEventService, you must require the 'colinodell/psr-testlogger' package in your dev dependencies."
     }
 }

src/Service/EventService.php

@@ -6,9 +6,11 @@
 use ArchiPro\EventDispatcher\AsyncEventDispatcher;
 use ArchiPro\EventDispatcher\ListenerProvider;
 use ArchiPro\Silverstripe\EventDispatcher\Contract\ListenerLoaderInterface;
+use Psr\Log\LoggerInterface;
 use SilverStripe\Core\Config\Configurable;
 use SilverStripe\Core\Injector\Injectable;
 use SilverStripe\Core\Injector\Injector;
+use Throwable;
 
 /**
  * Core service class for handling event dispatching in Silverstripe.
@@ -148,4 +150,19 @@ public function disableDispatch(): void
     {
         $this->suppressDispatch = true;
     }
+
+    /**
+     * Handle an error that occurred during event dispatching by logging them
+     * with the default Silverstripe CMS error handler logger.
+     *
+     * @internal This method is wired to the AsyncEventDispatcher with the Injector
+     *
+     * @see _config/events.yml
+     */
+    public static function handleError(Throwable $error): void
+    {
+        Injector::inst()
+            ->get(LoggerInterface::class)
+            ->error($error->getMessage(), ['exception' => $error]);
+    }
 }

src/Service/TestEventService.php

@@ -0,0 +1,65 @@
+<?php
+
+namespace ArchiPro\Silverstripe\EventDispatcher\Service;
+
+use ArchiPro\EventDispatcher\AsyncEventDispatcher;
+use ArchiPro\EventDispatcher\ListenerProvider;
+use Closure;
+use ColinODell\PsrTestLogger\TestLogger;
+use SilverStripe\Core\Injector\Injector;
+use Throwable;
+
+/**
+ * Extension of the AsyncEventDispatcher for testing purposes.
+ *
+ * This service will throw exceptions when errors occur to make it easier to debug issues.
+ */
+class TestEventService extends EventService
+{
+    private TestLogger $logger;
+
+    public function __construct()
+    {
+        if (!class_exists(TestLogger::class)) {
+            throw new \Exception(
+                'To use the TestEventService, you must require the "colinodell/psr-testlogger" ' .
+                'package in your dev dependencies.'
+            );
+        }
+
+        $this->logger = new TestLogger();
+
+        $listenerProvider = Injector::inst()->get(ListenerProvider::class);
+        $dispatcher = new AsyncEventDispatcher(
+            $listenerProvider,
+            Closure::fromCallable([$this, 'recordError'])
+        );
+        parent::__construct($dispatcher, $listenerProvider);
+    }
+
+    /**
+     * Bootstrap the TestEventService. Will replace the default EventService with a TestEventService.
+     */
+    public static function bootstrap(): self
+    {
+        $service = new self();
+        Injector::inst()->registerService($service, AsyncEventDispatcher::class);
+        return $service;
+    }
+
+    /**
+     * Catch errors and store them for later inspection.
+     */
+    private function recordError(Throwable $message): void
+    {
+        $this->logger->error($message->getMessage(), ['exception' => $message]);
+    }
+
+    /**
+     * Test logger where exception thrown by listeners are logged.
+     */
+    public function getTestLogger(): TestLogger
+    {
+        return $this->logger;
+    }
+}

tests/php/Service/EventServiceTest.php

@@ -4,6 +4,8 @@
 
 use ArchiPro\Silverstripe\EventDispatcher\Service\EventService;
 use ArchiPro\Silverstripe\EventDispatcher\Tests\TestListenerLoader;
+use ColinODell\PsrTestLogger\TestLogger;
+use Psr\Log\LoggerInterface;
 use Revolt\EventLoop;
 use SilverStripe\Core\Injector\Injector;
 use SilverStripe\Dev\SapphireTest;
@@ -114,4 +116,20 @@ public function testEventDispatchWithDisabledDispatch(): void
         // Assert listener was called
         $this->assertTrue($result->handled, 'Event listener should have been called when dispatch is re-enabled');
     }
+
+    public function testHandleError(): void
+    {
+        // Arrange
+        $testLogger = new TestLogger();
+        Injector::inst()->registerService($testLogger, LoggerInterface::class);
+        $ex = new \Exception('Test error');
+
+        // Act
+        EventService::handleError($ex);
+
+        // Assert
+        $this->assertCount(1, $testLogger->records, 'Error should be to default error logger');
+        $this->assertEquals('Test error', $testLogger->records[0]['message'], 'Error message should be "Test error"');
+        $this->assertEquals($ex, $testLogger->records[0]['context']['exception'], 'Error should be the same exception');
+    }
 }

tests/php/Service/TestEventServiceTest.php

@@ -0,0 +1,46 @@
+<?php
+
+namespace ArchiPro\Silverstripe\EventDispatcher\Tests\Service;
+
+use ArchiPro\Silverstripe\EventDispatcher\Service\TestEventService;
+use Exception;
+use Revolt\EventLoop;
+use SilverStripe\Dev\SapphireTest;
+
+class TestEventServiceTest extends SapphireTest
+{
+    private TestEventService $service;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+        $this->service = TestEventService::bootstrap();
+    }
+
+    public function testGetTestLogger(): void
+    {
+        // Create test event
+        $event = new class () {};
+
+        // Add test listener
+        $this->service->addListener(get_class($event), function ($event) {
+            throw new Exception('Test exception');
+        });
+
+        $this->assertFalse(
+            $this->service->getTestLogger()->hasErrorRecords(),
+            'No exceptions have been thrown yet'
+        );
+
+        // Dispatch event
+        $this->service->dispatch($event);
+
+        EventLoop::run();
+
+        $this->assertCount(
+            1,
+            $this->service->getTestLogger()->records,
+            'Running the event loop will cause an error to be logged'
+        );
+    }
+}