Allow multiple entity manager

[jdeniau]

See explanations in code

[jdeniau]

ObjectManagerPersister take an EntityManager as parameter, but we give a Registy

[theofidry]

it's curious that this works...

[jdeniau]

I don't understand my comment, but it seems that ObjectManagerPersister uses à Doctrine Object manager (the default one I think) when the ManagerRegistryPersister uses doctrine registry, and then is able to select wich ObjectManagerPersister to select.

They both implements the same PersisterInterface

I think that the ObjectManagerPersister should work when using only one entity manager

[jdeniau]

This allows having multiple instance of Doctrine\ORM\EntityManager

[theofidry]

👍

[jdeniau]

(this matches previous comment)

[theofidry]

Thanks for the PR @jdeniau!

I'll have a look after new year if you don't mind. It's indeed a tricky problem and there's a couple of open issues and PRs related to this. So I think I'll have to review a bit everything and also assess if there is any deeper issue or work to do as well.

[jdeniau]

I'll have a look after new year if you don't mind.

Of course no problem ! 🎉

[theofidry]

@jdeniau sorry this PR went a bit unnoticed. Would it be possible to update it with the latest changes?

[jdeniau]

@theofidry My branch was originally based upon your feature/multiple-entity-managers branch.

I rebased this PR on master, we now have our mixed work on multiple entity manager mixed.

The review might be harder to do (or you will review your work too), but basically, I just merged master and resolved some conflicts 😉

I had a conflict on the Purger class, I picked the changes made here

[jdeniau]

This PR does now superseed #99

[theofidry]

👌 Looks like the CI is failing.

I suppose this should be part of a major release? I'm afraid it is introducing some BC breaks...

[jdeniau]

👌 Looks like the CI is failing.

Hi @theofidry , I fixed the CI, the bugs came from the Purger class that now extends ObjectManagerPurger and thus the private properties were not enabled anymore.

The only failing test is the same failing test as master.

I suppose this should be part of a major release? I'm afraid it is introducing some BC breaks...
Theoretically, there should be no BC break, as the XML files are updated.

But I am not 100% either, and an internal refactoring might be a valid argument to create a major release, so it looks good to me.

[jdeniau]

For the record if you push a major release, you might want to drop the deprecated classes (Purger for example).
There might be some cleaning to do after thi PR is merged in that case

[jdeniau]

@theofidry I took time to re-work on that PR again.
The CI is failing but for the same reason as #168 . Probably because the lock file is not set and some dependency did change since last commit on master ? 🤔

I will try to test intensively in the next days with doctrine/orm, and I come back to you if everything is OK.

[COil]

@theofidry I took time to re-work on that PR again.
The CI is failing but for the same reason as #168 . Probably because the lock file is not set and some dependency did change since last commit on master ? 🤔
I will try to test intensively in the next days with doctrine/orm, and I come back to you if everything is OK.

Hi jdeniau, did you have time to make progress on this PR? I am discovering this right now, and I'll be interested in this feature too. See you.

[jdeniau]

Hi @COil ,

I ran into a lot of new issues with this PR, probably due to #155 and the entity id management.

As I was upgrading the whole package of behat / alice and the all ecosystem (Symfony2Extension to SymfonyExtension for the main upgrade), I did not manage to make this work, and won't take anymore time on it.

The solution I came by is the following though:

<?php

declare(strict_types=1);

namespace Mapado\TicketingBundle\Tests\Context;

use App\Entity\SomeEntity;
use Behat\Behat\Context\Context;
use Behat\Gherkin\Node\TableNode;
use Doctrine\ORM\Mapping\ClassMetadataInfo;
use Doctrine\Persistence\ManagerRegistry;
use Nelmio\Alice\FileLoaderInterface;
use Symfony\Component\Yaml\Yaml;

class FixtureLoaderContext implements Context
{
    // we were probably messing up with ids. I managed to remove nearly all ids from fixtures, 
    // but for some files, it was just too complex.
    private const CLASS_WITH_FORCED_IDS = [
        SomeEntity::class,
        // …
    ];

    /**
     * @var FileLoaderInterface
     */
    private $aliceFileLoader;

    /**
     * @var string
     */
    private $basePath;

    /** @var ManagerRegistry */
    private $doctrine;

    public function __construct(
        FileLoaderInterface $aliceFileLoader,
        ManagerRegistry $doctrine,
        string $basePath
    ) {
        $this->aliceFileLoader = $aliceFileLoader;
        $this->doctrine = $doctrine;
        $this->basePath = $basePath;
    }

    /**
     * @SuppressWarnings(PHPMD.UnusedFormalParameter("persister"))
     *
     * @Given the following fixtures are loaded:
     * @Given the following fixtures files are loaded:
     * @Given the following fixtures files are loaded with the persister :persister:
     *
     * @param TableNode<string> $fixtures Path to the fixtures
     */
    public function thereAreSeveralFixtures(TableNode $fixtures, ?string $persister = null): void
    {
        $files = [];
        foreach ($fixtures->getRows() as $row) {
            $files[] = $row[0];
        }

        $this->loadFiles($files);
    }

    /**
     * @SuppressWarnings(PHPMD.UnusedFormalParameter("persister"))
     *
     * @Given the fixtures file :file is loaded with the persister :persister
     */
    public function theFixturesFileIsLoadedWithThePersister(string $file, ?string $persister = null): void
    {
        $this->loadFiles([$file]);
    }

    /**
     * TODO keep this method in case we want to load all file simultaneously (or see following TODO)
     *
     * @param array<string> $files
     */
    private function loadFiles(array $files): void
    {
        $fixturesFiles = [];
        $objects = [];

        foreach ($files as $file) {
            $fixturesFile = $this->basePath . $file;

            $fixturesFiles[] = $fixturesFile;

            // TODO this is probably useless as "do load file" does flush on each file now
            // but it was a list of files that I wanted to be flushed in DB BEFORE other files do load, as we have some db id fetching
            if (in_array($file, ['10.some_file.yml'])) {
                $objects = $this->doLoadFiles($fixturesFiles, $objects);
                $fixturesFiles = [];
            }
        }

        if ($fixturesFiles) {
            $this->doLoadFiles($fixturesFiles, $objects);
        }
    }

    /**
     * @param array<string> $files
     * @param array<object> $objects
     */
    private function doLoadFiles(array $files, array $objects)
    {
        // force offer rule id to be set
        foreach (self::CLASS_WITH_FORCED_IDS as $class) {
            $entityManager = $this->doctrine->getManagerForClass($class);
            if (!$entityManager) {
                throw new \RuntimeException('Unable to get entity manager for class ' . $class);
            }
            $metadata = $entityManager->getClassMetadata($class);
            if (!$metadata instanceof ClassMetadataInfo) {
                throw new \RuntimeException('Class with forced id metata must implement ClassMetadataInfo. This should not happen.');
            }

            // I force the `GENERATOR_TYPE_NONE` here to keep the old alice data fixture compatibility
            // You may want to use something different here ¯\_(ツ)_/¯
            $metadata->setIdGeneratorType(\Doctrine\ORM\Mapping\ClassMetadata::GENERATOR_TYPE_NONE);
        }

        $this->preventObjectNameConflict($files);

        $loader = $this->aliceFileLoader;

        $parameters = [];
        $objectList = $objects;
        foreach ($files as $file) {
            $objectSet = $loader->loadFile($file, $parameters, $objectList);

            $parameters = $objectSet->getParameters();
            $objectList = $objectSet->getObjects();

            foreach ($objectList as $object) {
                /** @var class-string */
                $objectClass = get_class($object);

                // if object with forced id, check that the id is set
                if (in_array($objectClass, self::CLASS_WITH_FORCED_IDS) && !$object->getId()) {
                    throw new \InvalidArgumentException($objectClass . ' ids must be set in fixtures in file ' . $file);
                }

                $entityManager = $this->doctrine->getManagerForClass($objectClass);

                if (!$entityManager) {
                    throw new \RuntimeException('Unable to find manager for class ' . $objectClass);
                }

                $entityManager->persist($object);
            }

            $entityManager->flush();
        }

        return $objectList;
    }

    /**
     * This function makes sur that there is no key present in several files loaded in the same time both files will be loaded in the same array
     * 
     * @SuppressWarnings(PHPMD.UnusedLocalVariable("localObject"))
     *
     * @param array<string> $files
     */
    private function preventObjectNameConflict(array $files): void
    {
        $objectList = [];
        foreach ($files as $file) {
            $parsedYaml = Yaml::parseFile($file);

            if ($parsedYaml) {
                foreach ($parsedYaml as $localObjectList) {
                    foreach ($localObjectList as $localKey => $localObject) {
                        if (array_key_exists($localKey, $objectList)) {
                            throw new \InvalidArgumentException(
                            sprintf(
                                'key %s of file %s already exist in previously loaded objects',
                                $localKey,
                                $file
                            )
                        );
                        }
                    }
                    $objectList += $localObjectList;
                }
            }
        }
    }
}

If you want to continue working on this PR, feel free to do so.

[COil]

Hello @jdeniau , thanks for the answer and the code. I came with another solution where I decorate the ObjectManagerPersister service. In the persist() function I also use getManagerForClass the get the good entity manager to use, then I force the ObjectManagerPersister.objectManager property with reflection (the persistableClasses property must also be reinitialized when the entity manager is changed).