dmaicher/doctrine-test-bundle

Connection created twice if listeners depend on repositories

ossinkine opened this issue · 16 comments

ossinkine commented

After updating my project from Symfony 4.4 to 5.4 tests began fail. I've looked deep into the issue and discovered that after this feature was introduced the subscribers (Doctrine listeneres) initialize after addEventListener call, and if the subscriber depends on Doctrine services (such a repository) the connection creation called twice. Here is a stack trace:

-> StaticConnectionFactory.php:27, DAMA\DoctrineTestBundle\Doctrine\DBAL\StaticConnectionFactory->createConnection()
   App_KernelTestDebugContainer.php:1355, ContainerLXdkVH1\App_KernelTestDebugContainer->getDoctrine_Dbal_DefaultConnectionService()
   App_KernelTestDebugContainer.php:1383, ContainerLXdkVH1\App_KernelTestDebugContainer->getDoctrine_Orm_DefaultEntityManagerService()
   App_KernelTestDebugContainer.php:1368, ContainerLXdkVH1\App_KernelTestDebugContainer->ContainerLXdkVH1\{closure:/var/www/html/var/cache/test/ContainerLXdkVH1/App_KernelTestDebugContainer.php:1367-1373}()
   EntityManager_9a5be93.php:36, Closure->__invoke()
   EntityManager_9a5be93.php:36, ContainerLXdkVH1\EntityManager_9a5be93->getMetadataFactory()
   AbstractManagerRegistry.php:182, Doctrine\Bundle\DoctrineBundle\Registry->getManagerForClass()
   ServiceEntityRepository.php:37, App\Repository\UserRepository->__construct()
   UserRepository.php:31, App\Repository\UserRepository->__construct()
   App_KernelTestDebugContainer.php:2674, ContainerLXdkVH1\App_KernelTestDebugContainer->getUserRepositoryService()
   getUserListenerService.php:23, ContainerLXdkVH1\getUserListenerService::do()
   App_KernelTestDebugContainer.php:1005, ContainerLXdkVH1\App_KernelTestDebugContainer->load()
   Container.php:422, ContainerLXdkVH1\App_KernelTestDebugContainer->getService()
   ServiceLocator.php:42, Symfony\Component\DependencyInjection\Argument\ServiceLocator->get()
   ContainerAwareEventManager.php:203, Symfony\Bridge\Doctrine\ContainerAwareEventManager->initializeSubscribers()
   ContainerAwareEventManager.php:121, Symfony\Bridge\Doctrine\ContainerAwareEventManager->addEventListener()
-> StaticConnectionFactory.php:44, DAMA\DoctrineTestBundle\Doctrine\DBAL\StaticConnectionFactory->createConnection()
   App_KernelTestDebugContainer.php:1355, ContainerLXdkVH1\App_KernelTestDebugContainer->getDoctrine_Dbal_DefaultConnectionService()
   App_KernelTestDebugContainer.php:1383, ContainerLXdkVH1\App_KernelTestDebugContainer->getDoctrine_Orm_DefaultEntityManagerService()
   App_KernelTestDebugContainer.php:1368, ContainerLXdkVH1\App_KernelTestDebugContainer->ContainerLXdkVH1\{closure:/var/www/html/var/cache/test/ContainerLXdkVH1/App_KernelTestDebugContainer.php:1367-1373}()
   EntityManager_9a5be93.php:239, Closure->__invoke()
   EntityManager_9a5be93.php:239, ContainerLXdkVH1\EntityManager_9a5be93->getConfiguration()
   ProxyCacheWarmer.php:54, Symfony\Bridge\Doctrine\CacheWarmer\ProxyCacheWarmer->warmUp()
   CacheWarmerAggregate.php:99, Symfony\Component\HttpKernel\CacheWarmer\CacheWarmerAggregate->warmUp()
   Kernel.php:587, App\Kernel->initializeContainer()
   Kernel.php:789, App\Kernel->preBoot()
   Kernel.php:128, App\Kernel->boot()
   KernelTestCase.php:83, Symfony\Bundle\FrameworkBundle\Test\KernelTestCase::bootKernel()
   KernelTestCase.php:103, Symfony\Bundle\FrameworkBundle\Test\KernelTestCase::getContainer()
   WebTestCase.php:65, App\Tests\Controller\DefaultControllerTest->getEntityManager()
   DefaultControllerTest.php:23, App\Tests\Controller\DefaultControllerTest->setUp()
   TestCase.php:1145, App\Tests\Controller\DefaultControllerTest->runBare()
   TestResult.php:726, PHPUnit\Framework\TestResult->run()
   TestCase.php:904, App\Tests\Controller\DefaultControllerTest->run()
   TestSuite.php:678, PHPUnit\Framework\DataProviderTestSuite->run()
   TestSuite.php:678, PHPUnit\Framework\TestSuite->run()
   TestSuite.php:678, PHPUnit\Framework\TestSuite->run()
   TestRunner.php:670, PHPUnit\TextUI\TestRunner->run()
   Command.php:143, Symfony\Bridge\PhpUnit\Legacy\CommandForV9->run()
   Command.php:96, PHPUnit\TextUI\Command::main()
   phpunit:22, include()
   simple-phpunit.php:436, require()
   phpunit:13, {main}()

I'm not sure this a right place to fix it but looks like event listener might be attached another way.

dmaicher commented

Hm I don't see how this can be related to this bundle. Do you have the same issue without using this bundle?

ossinkine commented

Yes, without this bundle works good, because default connection factory used. In the stacktrace you can see that second initialization starts in StaticConnectionFactory.

I've removed repository as dependency from listener as workaroud but maybe there is a prettier solution.

  • 👍1
dmaicher commented

@ossinkine ah ok. Now I get it. I misread the stacktrace.

I will have a look later today if there is something we can change here

dmaicher commented

So I had a deeper look and the only solution I currently see is to create a custom EventManager that decorates the existing event manager and is lazy in registering the PostConnectEventListener until the event is actually dispatched.

But this seems rather complex and also would break things for people that expect the event manager to be an instance of Symfony\Bridge\Doctrine\ContainerAwareEventManager 😕

So to solve this "circular reference"-like issue the best seems indeed to remove that dependency on your end.

Let me know if you have another idea.

dmaicher commented

Or actually we could maybe register the PostConnectEventListener using DI config on the Symfony\Bridge\Doctrine\ContainerAwareEventManager directly 🤔 Will have another look

ossinkine commented

Your fix is looking good and working for me 👍

  • 🎉1
dmaicher commented

@ossinkine unfortunately there were side-effects to the fix and I decided to revert it

So for now I don't have a solution for this edge-case with the circular dependency via the ContainerAwareEventManager

ossinkine commented

@dmaicher sad to hear this. Maybe reopen the issue to see the bug still exists?

  • 👍1
raziel057 commented

Hello, I just notice an issue related to this one while testing to modify a loggable entity:
https://github.com/KnpLabs/DoctrineBehaviors/blob/master/docs/loggable.md

My Unit Test update an entity and a log should be inserted in database (confirmed if I disable the DAMA\DoctrineTestBundle extension, or if I apply the patch c3ea20e)

When I enable the extension, my entity is modified successfully and I can trace the persist and flush of the Log but it seems not related to the same connection as I'm able to assert the entity changes but not the creation of the log. When I disable the extension or apply the patch c3ea20e I can see the log created successfully.

dmaicher commented

@raziel057 yes indeed 😕 This can be an issue if registering the PostConnectEventListener in turn causes other listeners to be instantiated that may have a dependency on a dbal connection themselves.

Now since we dropped support for DBAL 2 maybe there is a way to make patch c3ea20e work without side-effects... 🤔 need to find some time to dig into it

dmaicher commented

I think I found a way to resolve this issue 😊 I will create a PR within the next days

raziel057 commented

@dmaicher It would be really nice. Thanks for this.

dmaicher commented

@raziel057 would you be able to test branch fix_issue_191 ? Needs some polishing but I believe this will fix the issue.

raziel057 commented

@dmaicher I confirm that it works fine when I apply the branch fix_issue_191

  • 🎉1
raziel057 commented

Thank you very much for this great bundle!

  • 👍1