Entity not managed since version 1.12.6 when using refresh

[adrienfr]

Hello,

When migrating from 1.12.2 to 1.12.6, my PHPUnit fails when I have a $em->refresh($entity) inside a test.

I have a skeleton app to reproduce the issue : https://github.com/adrienfr/sf-skeleton

Here is the error :

There was 1 error:

1. App\Tests\Controller\AdminFlatControllerTest::testEditFlat
   Doctrine\ORM\ORMInvalidArgumentException: Entity App\Entity\Flat@00000000017029ce00000000242b0986 is not managed. An entity is managed if its fetched from the database or registered as new through EntityManager#persist

/Users/adrienwilmet/Sites/sf-skeleton/vendor/doctrine/orm/lib/Doctrine/ORM/ORMInvalidArgumentException.php:143
/Users/adrienwilmet/Sites/sf-skeleton/vendor/doctrine/orm/lib/Doctrine/ORM/UnitOfWork.php:2165
/Users/adrienwilmet/Sites/sf-skeleton/vendor/doctrine/orm/lib/Doctrine/ORM/UnitOfWork.php:2139
/Users/adrienwilmet/Sites/sf-skeleton/vendor/doctrine/orm/lib/Doctrine/ORM/EntityManager.php:653
/Users/adrienwilmet/Sites/sf-skeleton/tests/Controller/AdminFlatControllerTest.php:23

Here is my test :

/**
 * @internal
 */
class AdminFlatControllerTest extends CreateEntityWebTestCase
{
    public function testEditFlat()
    {
        $flat = $this->em->getRepository(Flat::class)->findOneByNote('test');
        $crawler = $this->client->request('GET', sprintf('/admin/flat/%d/edit', $flat->getId()));
        $this->assertEquals(200, $this->client->getResponse()->getStatusCode());
        $buttonNode = $crawler->selectButton('admin_edit_flat_submit');
        $form = $buttonNode->form();
        $form['admin_edit_flat[guests]'] = 2;
        $crawler = $this->client->submit($form);    
        $this->assertEquals(302, $this->client->getResponse()->getStatusCode());
        $this->em->refresh($flat);
        $this->assertEquals(2, $flat->getGuests());
    }
}

And CreateEntityWebTestCase :

<?php

namespace App\Tests;

use Doctrine\ORM\EntityManagerInterface;
use Symfony\Bundle\FrameworkBundle\Client;
use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
use Symfony\Component\PropertyAccess\PropertyAccess;
use Symfony\Component\PropertyAccess\PropertyAccessor;

/**
 * @internal
 */
class CreateEntityWebTestCase extends WebTestCase
{
    /** @var Client */
    protected $client;

    /** @var EntityManagerInterface */
    protected $em;

    /** @var PropertyAccessor */
    protected $propertyAccessor;

    protected function setUp(): void
    {
        parent::setUp();

        if (null !== static::$kernel) {
            static::$kernel->shutdown();
        }

        $this->client = self::createClient();
        $this->client->disableReboot();
        $this->em = self::$container->get('doctrine.orm.entity_manager');
        $this->em->beginTransaction();
        $this->em->getConnection()->setAutoCommit(false);
    }

    protected function tearDown(): void
    {
        $this->em->rollback();
        $this->em->close();
        $this->em = null;
        $this->client = null;

        parent::tearDown();
    }
}

Am I missing a configuration change somewhere ?

[frenchcomp]

I have similar issue since : #1099
Entity manager injected into my service are lazy loaded, but entity manager injected in Repository, defined as service, are not lazy loaded.

So Entity are managed by different instances of a same "manager configuration".

[ostrolucky]

Confirmed. Happens with just bundle update with given reproducer. cc @alcaeus @nicolas-grekas

[guillaumesmo]

I have a similar issue (probably related) with failing tests since upgrade to 2.0.6
For some reason it fails only in the tests, not in dev or prod environments

[2019-12-20 15:49:54] app.CRITICAL: Multiple non-persisted new entities were found through the given association graph: * A new entity was found through the relationship 'XXXXX#XXXXX' that was not configured to cascade persist operations for entity: XXXXX@000000002ddd4e7200000000753238a0. To solve this issue: Either explicitly call EntityManager#persist() on this unknown entity or configure cascade persist this association in the mapping for example @manytoone(..,cascade={"persist"}). If you cannot find out which entity causes the problem implement 'XXXXX#__toString()' to get a clue. [...] at /data/src/vendor/doctrine/orm/lib/Doctrine/ORM/ORMInvalidArgumentException.php:105)"} []

Last known-good version is 2.0.2, couldn't test other version for different reasons:
2.0.3: Doctrine ORM Manager named "doctrine.orm.default_entity_manager" does not exist
2.0.4: Resetting a non-lazy manager service is not supported. Declare the "doctrine.orm.default_entity_manager" service as lazy.
2.0.5: Resetting a non-lazy manager service is not supported. Declare the "doctrine.orm.default_entity_manager" service as lazy.

[alcaeus]

This is most likely related to the manager now being reset or cleared (depending on whether or not it is marked as lazy) on the kernel.reset event. @nicolas-grekas is there a workaround for this in tests or would you suggest reverting the entire thing for the time being?

[frenchcomp]

The issue is not related to the manager now being reset or cleared, but the manager instance is not always the same instance. In manager injected thanks to services.yml, the manager is lazy loaded, but manager injected via factory for repository or form is not lazy loaded.

So, we have two instances, some entities are loaded with the first, other with the second, and by example, if we test a form submit : entites fetched by the manager injected in the form are unknown for the manager injected in my persistance service.

[nicolas-grekas]

I won't be able to help here in the next few days sorry.

[frenchcomp]

For me, it's not an emergency, I rollback to 1.12.2. (I just pass a lot of times with xdebug to understand the issue this tomorow, if i can help, I come back the next year)
Happy hollidays

[alcaeus]

Hmm, we haven’t had these proxy issues in a while. I still wouldn’t rule out a connection to the reset logic, but I won’t have the time to investigate over the next couple of weeks.

[acasademont]

mmm, i'm not entirely sure this is related to #1114 so I opened a new issue, but it feels pretty similar.

[bendavies]

should be resolved by using ServiceEntityRepository+autoconfigure or the doctrine.repository_service tag as provided in #727

[adrienfr]

Thanks @bendavies but I already use ServiceEntityRepository (see here) and autoconfigure/autowire (see here). The entity/repository was created by the Maker Bundle.

[ostrolucky]

There is one important finding: This break happens only when using $this->client->disableReboot(). Removing this call will achieve same result on doctrine/doctrine-bundle 1.12.2

Honestly, new behaviour seems correct to me. Users should not expect identity map is kept as is between requests. That kind of thing causes memore leaks.

Perhaps you can ask in symfony/symfony for a feature to disableReset() instead. Meanwhile if you really need old behaviour, you can write compiler pass which removes kernel.reset tag from doctrine service.

[karser]

I encountered this bug on Doctrine 2.0.6, here is more details #1114 (comment)

[frenchcomp]

Hi @ostrolucky
Sorry, but this is not the behavior that I reproduce. It's not an issue only in test but also in productions.
All entities manager injected in my service thanks to @doctrine.orm.default_entity_manager are lazy service and implement the interface. But all entities managed injected in repository defined as service in the DI, by this following code :

  foo.bar:
    class: '%foo.bar.class%'
    factory: ['@doctrine.orm.default_entity_manager', 'getRepository']
    arguments: ['Foo:Bar']

are not a lazy service and directly the entity manager.

But, in a same HTTP request, there are two intances of my entity manager, (one as lazy service and one not). And, when I load an entity A from a repository and inject into another entity B loaded/persisted by my service, the entity manager will not know A and the operation will fail.

[ostrolucky]

@frenchcomp Well this issue is related to #1099, your issue doesn't seem so, as there shouldn't be any kernel reset happenning there, so you might need to troubleshoot some more and create new issue. Perhaps #1114 is your problem.

[acasademont]

After the findings in #1114 I think both issues are definitely related, both stem from the way the LazyProxy resets the EM and the Entity Repositories are created, they end up with 2 different instances of the EM that mess it all up.

[bendavies]

@frenchcomp please read the thread. You issue will be fixed by my comment. #1112 (comment)

[frenchcomp]

@bendavies Sorry but it's not possible for us. We have 4 huge projects, we can not rewrite 240 repositories class and much more services. And more, this change in behavior is not in a major release, it's a BC Break.

[adrienfr]

Hello @ostrolucky, I tried to comment $this->client->disableReboot() (adrienfr/sf-skeleton@cb94ff9) but it doesn't solve the issue, I still have :

1) App\Tests\Controller\AdminFlatControllerTest::testEditFlat
Doctrine\ORM\ORMInvalidArgumentException: Entity App\Entity\Flat@000000001629bded00000000538e06d9 is not managed. An entity is managed if its fetched from the database or registered as new through EntityManager#persist

The repository extends ServiceEntityRepository.

[ostrolucky]

I wasn't saying removing disableReboot() would fix the problem, I was saying you would get same error without disableReboot(). This is a break only when using disableReboot().

[adrienfr]

@ostrolucky ok thank you for the clarification.
Then what solution can be found for this issue? This test (here) seems pretty basic to me and I use it quite a lot in different projects, I don't really see what should be changed.

[ostrolucky]

refresh -> find

[adrienfr]

Thank you @ostrolucky. Before replacing every refresh in our tests, I just want to confirm that this is the only solution (vs. doing a rollback and using a major version to implement this change)? It seems that I'm not the only one having issues with the new behavior.

[guillaumesmo]

I think we're all reporting different issues and a wild guess is that other issues will arise, but there's no need to blame anyone

On my side, composer require doctrine/doctrine-bundle:2.0.2 fixes the issue for now until this all settles down

[stof]

@frenchcomp defining services using ['@doctrine.orm.default_entity_manager', 'getRepository'] has always been incompatible with resetting the entity manager. That's why it has always been discouraged by the DoctrineBundle team, and that the ServiceEntityRepository feature has been built in the bundle, to make sure that there was a way to define repositories as services without being hit by such incompatibility.

[frenchcomp]

@stof, the issue is not with reseting, there are the issue at the first call. There are two instances of the entity manager in the Symfony Container.

Yes, it's currently discouraged, but, this behavior was indicated in the old SF documentation (in 2.3). You want deprecated this, ok, so put a deprecated warning and change in the next major release. :)

But the issue here, is not the reset, but there are two instance of the entity manager for a same namespace domaine.

[alcaeus]

@frenchcomp can you confirm if downgrading to 2.0.2 fixes the problem of having two different instances for the entity manager service?

[ostrolucky]

@frenchcomp defining services using ['@doctrine.orm.default_entity_manager', 'getRepository'] has always been incompatible with resetting the entity manager. That's why it has always been discouraged by the DoctrineBundle team, and that the ServiceEntityRepository feature has been built in the bundle, to make sure that there was a way to define repositories as services without being hit by such incompatibility.

Do you have some reference for that? AFAIK ServiceEntityRepository was built only to support autowiring.

@frenchcomp can you confirm if downgrading to 2.0.2 fixes the problem of having two different instances for the entity manager service?

There is no need to confirm such thing, you can see in #1118 that service reset is indeed causing this issue and there was no service reset being done till be9794b

I think we're all reporting different issues and a wild guess is that other issues will arise, but there's no need to blame anyone

Yes, that's why I would love to see #1118 merged and released ASAP, so we stop confusing one problem for the other.

[stof]

@frenchcomp the issue is with resetting. To support resetting (as the ORM instance does not support that itself), DoctrineBundle wraps the EntityManager in a proxy object (and resetting can then reset the proxy object to uninitialized state).

When using the default ORM repository factory, the EntityManager instance is passed to them as new $repositoryClass(..., $this) by the EM. But it means that the repository gets a reference to the inner EntityManager instance, not to the proxy (this is the usual issue that code relying on passing $this around is incompatible with decoration).

Do you have some reference for that? AFAIK ServiceEntityRepository was built only to support autowiring.

autowiring needed to have repositories defined as services. That's all. ServiceEntityRepository was built to support defining them as services in a working way, by ensuring that the repository holds a reference to the proxy EntityManager (while still making $em->getRepository() working to get the repository).

There is no need to confirm such thing, you can see in #1118 that service reset is indeed causing this issue and there was no service reset being done till be9794b

To be exact, there is no reset being done by the bundle itself before this commit.
The fact that repository services defined using factory: ['@doctrine.orm.default_entity_manager', 'getRepository'] are incompatible with service resetting is known since years by projects using the resetting already (which is a must have when working a long-running queue consumer for instance, as there is no other way to recover from an entity manager getting closed by an error).

[frenchcomp]

The ServiceEntityRepository has been introduce in October 2017 , in a minor version of Doctrine Bundle, 2 year ago.

We not use the version 2.0 of Doctrine Bundle but 1.12, the new behavior introduces into last patch is a BC Break on a minor version. I'm in favor to support only ServiceEntityRepository in the next major release, but this patch break will many projects currently in production with the current major release.

The major of projects using Doctrine are supported by a very small team. Follow conventions is important. :) Throw a deprecated warning also.

[ostrolucky]

there is no other way to recover from an entity manager getting closed by an error.

I outlined different approach at symfony/symfony#35216 (comment). I don't know why people came up with such complicated, unreliable mechanism instead of just clearing and resetting flag.

When using the default ORM repository factory, the EntityManager instance is passed to them as new $repositoryClass(..., $this) by the EM

But we are not using default ORM repository factory, bundle ships its own. We could choose to ignore passed instance of EntityManager and use lazy loaded one instead (plus handling edge cases like multiple EMs)

[adrienfr]

I tried to update to DoctrineBundle 2.0.2 but now I have this error when using $this->getDoctrine() in controllers extending AbstractController :

The DoctrineBundle is not registered in your application. Try running "composer require symfony/orm-pack".

while DoctrineBundle is still in my bundles.php and with a bin/console debug:container doctrine, I have :

Information for Service "doctrine"
==================================

 References all Doctrine connections and entity managers in a given Container.

 ---------------- ----------------------------------------- 
  Option           Value                                    
 ---------------- ----------------------------------------- 
  Service ID       doctrine                                 
  Class            Doctrine\Bundle\DoctrineBundle\Registry  
  Tags             -                                        
  Public           yes                                      
  Synthetic        no                                       
  Lazy             no                                       
  Shared           yes                                      
  Abstract         no                                       
  Autowired        no                                       
  Autoconfigured   no                                       
 ---------------- ----------------------------------------- 

(can't upgrade to 2.0.6 because of the same issue as 1.12.6 (Entity not managed after a refresh)). Do you have any clue?

[alcaeus]

This was closed due to GitHub being too eager...reopening as we've "only" fixed the duplicate service problem.

[adrienfr]

Hello @alcaeus,

I tried to update to 2.0.7 and to remove the kernel.reset tag on doctrine for test env here :

// Kernel.php
public function process(ContainerBuilder $container)
{
    if ('test' === $this->environment){
        $container->getDefinition('doctrine')->clearTag('kernel.reset');
    }
}

php bin/console debug:container doctrine --env=test :

Information for Service "doctrine"

References all Doctrine connections and entity managers in a given Container.

---

Option Value

---

Service ID doctrine
Class Doctrine\Bundle\DoctrineBundle\Registry
Tags -
Public yes
Synthetic no
Lazy no
Shared yes
Abstract no
Autowired no
Autoconfigured no

---

The error is still here :

Entity App\Entity\Flat@000000004ae3f97a000000000e47f602 is not managed. An entity is managed if its fetched from the database or registered as new through EntityManager#persist

I don't have many options left, this issue is preventing me from upgrading to SF 4.4 (I have the "The DoctrineBundle is not registered in your application. Try running "composer require symfony/orm-pack"." error with DoctrineBundle 2.0.2) and I have tons of $this->em->refresh(...) in my tests.

In the given reproducer, do you see something that I could change in order to solve this?

[ostrolucky]

@adrienfr It's just about priorities. This will work

protected function build(ContainerBuilder $container)
{
    parent::build($container);

    $container->addCompilerPass(new class implements CompilerPassInterface {
        public function process(ContainerBuilder $container)
        {
            $container->getDefinition('doctrine')->clearTag('kernel.reset');
        }
    }, PassConfig::TYPE_BEFORE_OPTIMIZATION, 1);
}

We will document this compiler pass in upgrade guide so we can help people who rely on this behaviour migrating the code. We still think this is correct behaviour though and people were relying on a buggy behaviour so we are keeping it.

[adrienfr]

Thank you very much @ostrolucky !

[fcojavierdomenech]

@adrienfr It's just about priorities. This will work

protected function build(ContainerBuilder $container)
{
    parent::build($container);

    $container->addCompilerPass(new class implements CompilerPassInterface {
        public function process(ContainerBuilder $container)
        {
            $container->getDefinition('doctrine')->clearTag('kernel.reset');
        }
    }, PassConfig::TYPE_BEFORE_OPTIMIZATION, 1);
}

We will document this compiler pass in upgrade guide so we can help people who rely on this behaviour migrating the code. We still think this is correct behaviour though and people were relying on a buggy behaviour so we are keeping it.

I added this patch on my Kernel and works perfectly, thanks.

What should we do in order to avoid relying on this "buggy behaviour"?

[ostrolucky]

What should we do in order to avoid relying on this "buggy behaviour"?

Assume doctrine doesn't keep state between requests. In case mentioned in this thread pretty much following:

refresh -> find

[fcojavierdomenech]

Thanks ostrolucky, I'll keep it in mind for next refactors.

[ostrolucky]

Documented in #1134

[kojidev]

@ostrolucky They've deleted the doc in 2.0.x tag:

https://github.com/doctrine/DoctrineBundle/blob/1.12.x/UPGRADE-2.0.md

https://github.com/doctrine/DoctrineBundle/blob/2.0.x/UPGRADE-2.0.md

[ostrolucky]

They didn't delete it, just didn't merge it there yet, as there wasn't release since then.

[kojidev]

Oh, that's right, sorry