Added tests to ensure ModuleManager::register does not change anything upon error.

src/ModuleManager.php

@@ -42,6 +42,7 @@ class ModuleManager
     {
         $name = $library->getName();
         $package = $library->getComposerPackage();
+        $em = $this->g->getEntityManager();
 
         $this->g->getLogger()->debug("Registering module {$name}...");
 
@@ -53,6 +54,8 @@ class ModuleManager
             $this->g->getLogger()->debug("Creating module model for {$name}");
             $m = new ModuleModel($name);
 
+            $em->beginTransaction();
+
             $class = $library->getRootNamespace() . 'Module';
             try {
                 $klass = new \ReflectionClass($class);
@@ -69,9 +72,16 @@ class ModuleManager
             }
 
             // Subscribe to the module's events.
-            $subscriptions = $library->getSubscriptionPatterns();
-            foreach ($subscriptions as $s) {
-                $this->g->getEventManager()->subscribe($s, $class, $name);
+            try {
+                $subscriptions = $library->getSubscriptionPatterns();
+                foreach ($subscriptions as $s) {
+                    $this->g->getEventManager()->subscribe($s, $class, $name);
+                }
+            } catch (\Throwable $e) {
+                $em->rollBack();
+                $em->clear();
+
+                throw $e;
             }
 
             // Run the module's onRegister handler.
@@ -80,10 +90,19 @@ class ModuleManager
             try {
                 $class::onRegister($this->g, $m);
                 $this->g->getEntityManager()->persist($m);
+
                 $this->g->getEntityManager()->flush();
+                $em->commit();
+                return;
             } catch (Throwable $e) {
+                $em->rollBack();
+                $em->clear();
+
                 $this->g->getLogger()->error("Calling {$class}::onRegister failed with exception: {$e->getMessage()}");
                 unset($m);
+
+                // Propagate the exception.
+                throw $e;
             }
         }
     }

tests/DefectiveModule/Module.php

@@ -5,6 +5,7 @@ namespace LotGD\Core\Tests\DefectiveModule;
 use LotGD\Core\Exceptions\CoreException;
 use LotGD\Core\Game;
 use LotGD\Core\Events\EventContext;
+use LotGD\Core\Models\Character;
 use LotGD\Core\Module as ModuleInterface;
 use LotGD\Core\Models\Module as ModuleModel;
 
@@ -18,6 +19,9 @@ class Module implements ModuleInterface {
 
     public static function onRegister(Game $g, ModuleModel $module)
     {
+        $character = Character::create(["name" => "Test"]);
+        $character->save($g->getEntityManager());
+
         throw new DefectiveModuleException("Exception");
     }
 

tests/DefectiveModule/lotgd.yml

@@ -1,3 +1,4 @@
 entityNamespace: "LotGD\\Core\\Tests\\FakeModule\\Models\\"
 subscriptionPatterns:
-    - "e/lotgd/core/tests/event"
+    - "e/lotgd/core/tests/defective-event"
+    - "e/lotgd/core/tests/gom-event"

tests/ModuleManagerTest.php

@@ -13,6 +13,7 @@ use LotGD\Core\EventHandler;
 use LotGD\Core\EventManager;
 use LotGD\Core\EventSubscription;
 use LotGD\Core\LibraryConfiguration;
+use LotGD\Core\Models\Character;
 use LotGD\Core\ModuleManager;
 use LotGD\Core\Module;
 use LotGD\Core\Exceptions\ModuleAlreadyExistsException;
@@ -201,11 +202,53 @@ class ModuleManagerTest extends CoreModelTestCase
         $this->assertEquals($name, $modules[1]->getLibrary());
     }
 
+    public function testRegisterWithFailedEvents()
+    {
+        $class = FakeModule::class;
+        $name = 'lotgd/tests2';
+        $subscriptions = array(
+            '/pattern1/',
+            '#asasd/',
+        );
+        $library = $this->getMockBuilder(LibraryConfiguration::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+        $library->method('getName')->willReturn($name);
+        $library->method('getRootNamespace')->willReturn('LotGD\\Core\\Tests\\FakeModule\\');
+        $library->method('getSubscriptionPatterns')->willReturn($subscriptions);
+
+        $eventManager = new EventManager($this->g);
+        $this->game->method('getEventManager')->willReturn($eventManager);
+
+        $eventsBefore = count($eventManager->getSubscriptions());
+
+        $subscriptionThrownException = false;
+        try {
+            $this->mm->register($library);
+        } catch(\Throwable $e) {
+            $subscriptionThrownException = true;
+        }
+
+        $this->assertTrue($subscriptionThrownException);
+
+        // Assert module has not been installed.
+        $modules = $this->mm->getModules();
+        $this->assertArrayNotHasKey(1, $modules);
+
+        // Assert events are not registered
+        $eventsAfter = count($eventManager->getSubscriptions());
+
+        // Randomly flush
+        $this->getEntityManager()->flush();
+
+        $this->assertSame($eventsBefore, $eventsAfter, "Events after failed subscription are actually more.");
+    }
+
     public function testRegisteringDefectiveModule()
     {
         $class = DefectiveModule::class;
         $name = "lotgd/tests3";
-        $subscriptions = [];
+        $subscriptions = ["#e/lotgd/core/tests/dat-event#"];
         $library = $this->getMockBuilder(LibraryConfiguration::class)
             ->disableOriginalConstructor()
             ->getMock();
@@ -213,11 +256,12 @@ class ModuleManagerTest extends CoreModelTestCase
         $library->method('getRootNamespace')->willReturn('LotGD\\Core\\Tests\\DefectiveModule\\');
         $library->method('getSubscriptionPatterns')->willReturn($subscriptions);
 
-        $eventManager = $this->getMockBuilder(EventManager::class)
-            ->disableOriginalConstructor()
-            ->getMock();
+        // Defective Module adds a character. Count the characters before.
+        $charactersBefore = count($this->getEntityManager()->getRepository(Character::class)->findAll());
 
+        $eventManager = new EventManager($this->g);
         $this->game->method('getEventManager')->willReturn($eventManager);
+
         $modulesBefore = $this->mm->getModules();
         try {
             // onRegister throws an exception. This exception needs to be captured and handled by mm->register without actually
@@ -229,7 +273,21 @@ class ModuleManagerTest extends CoreModelTestCase
         }
         $modulesAfter = $this->mm->getModules();
 
-        $this->assertFalse($exceptionCaptured);
+        $this->assertTrue($exceptionCaptured);
         $this->assertCount(count($modulesBefore), $modulesAfter);
+
+        // Make sure there are no event leftovers.
+        $subscriptions_db = $eventManager->getSubscriptions();
+        $found = 0;
+        foreach($subscriptions_db as $subscription) {
+            if (in_array($subscription->getPattern(), $subscriptions)) {
+                $found++;
+            }
+        }
+        $this->assertSame(0, $found);
+
+        // Count characters. Must stay the same!
+        $charactersAfter = count($this->getEntityManager()->getRepository(Character::class)->findAll());
+        $this->assertSame($charactersBefore, $charactersAfter, "Modules flushed did not get not added to the database.");
     }
 }