Allow service repositories to be used with more than one manager #1044

ipernet · 2019-11-04T20:43:23Z

Hi,

This is related to symfony/symfony-docs#9878 and may be as an answer to this very informative comment: symfony/symfony-docs#9878 (comment)

Different entity managers can be defined to manage a common set of entities (ref: https://symfony.com/doc/current/doctrine/multiple_entity_managers.html). It is a valid configuration set but this is not well supported by the boilerplate code generated by standard utilities such as make:entity or even by the internal ManagerRegistry class (see issue below).

However due to doctrine/persistence#68, it is currently not possible to have service repositories that can use an EntityManager different than "the first one defined to manage a given entity".

Speaking with code, these are the issues encountered with standard boilerplate code:

<?php

// A simple repository as defined with make:entity
namespace App\Repository;

use App\Entity\BadKitty;
use Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository;
use Doctrine\Common\Persistence\ManagerRegistry;

/**
 * @method BadKitty|null find($id, $lockMode = null, $lockVersion = null)
 * @method BadKitty|null findOneBy(array $criteria, array $orderBy = null)
 * @method BadKitty[]    findAll()
 * @method BadKitty[]    findBy(array $criteria, array $orderBy = null, $limit = null, $offset = null)
 */
class BadKittyRepository extends ServiceEntityRepository
{
    public function __construct(ManagerRegistry $registry)
    {
        parent::__construct($registry, BadKitty::class);
    }
}

// The BadKitty entity is manageable by both em1 and em2.
class LuckyController
{
    /**
     * @Route("/", name="home")
     */
    public function test(ManagerRegistry $registry)
    {
        $a = $registry->getManager('em1')->getRepository(BadKitty::class)->find(1);
        $b = $registry->getManager('em2')->getRepository(BadKitty::class)->find(1);

        return new Response(
            '<html><body></body></html>'
        );
    }

The issue is: $a === $b, when $a should be !== $b

However, when the repository is defined as a non-serviceable repository (the "old way"), this works:

<?php

namespace App\Repository;

use App\Entity\BadKitty;
use Doctrine\ORM\EntityRepository;

class BadKittyRepository extends EntityRepository
{
   
}

// Now $a === $b

But this legacy definition of a repository does not allow service repositories.

Again, the issue is doctrine/persistence#68 and cannot really be solved because there is no way to know what entity manager should be used at autowiring when multiple entity managers can manage a given entity. The first defined takes it all.

As a solution to this issue I'd like to propose the following approach:

Allow the repository to let the user select another valid entity manager to use at call time.

Example:

// The GentlePuppy entity is manageable by both em1 and em2.
class LuckyController
{
    /**
     * @Route("/test3", name="test3")
     */
    public function test3(GentlePuppyRepository $gentlePuppyRepository, ManagerRegistry $registry)
    {
        // First-defined em for the entity is used (no-change)
        $a = $gentlePuppyRepository->find(1);

        // But the em can be set explicitly or changed as follow:
        $b = $gentlePuppyRepository->withManager('em1')->find(1);
        $c = $gentlePuppyRepository->withManager('em2')->find(1);

        $d = $registry->getManager('em1')->getRepository(GentlePuppy::class)->find(1);
        $e = $registry->getManager('em2')->getRepository(GentlePuppy::class)->find(1);

        // $a === $b === $d
        // $c === $e and $d !== $e

        return new Response(
            '<html><body></body></html>'
        );
   }
}

This way:

1 - Current behavior does not change. It's also backward compatible.
2 - Unexpected behavior is fixed, the correct em is used accordingly ($d !== $e).
3 - Service repositories can be used in any configuration with no specific limitation to know about first or custom glue code.

How is it implemented?

Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository is rewritten to automatically generate a new (lazy) instance of the entity repository class when a different manager than the default autowired-one is specified by the user with the withManager method.

On the user side, it requires a minimal change in the constructor of the (service) repository (BC):

<?php

namespace App\Repository;

use App\Entity\GentlePuppy;
use App\Helper\MyCustomService;

use App\DependencyInjection\ServiceEntityRepository;
use Doctrine\Common\Persistence\ManagerRegistry;
use Doctrine\ORM\EntityRepository;

class GentlePuppyRepository extends ServiceEntityRepository
{
   /*
   public function __construct(MyCustomService $myService, ManagerRegistry $registry)
    {
        parent::__construct($registry, GentlePuppy::class);
    }
    */

    public function __construct(MyCustomService $myService, ManagerRegistry $registry, ...$extra)
    {
        parent::__construct($registry, GentlePuppy::class, func_get_args());
    }
}

This constructor definition allows the new ServiceEntityRepository class to create behind-the-scenes new (lazy) instances of GentlePuppyRepository but with a specific EntityManager instance (contained in the $extra args) and with the very same services instances than initially injected by the user.

Implementation details:

<?php

namespace App\DependencyInjection;

use Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepositoryInterface;
use Doctrine\Common\Persistence\ManagerRegistry;
use Doctrine\ORM\EntityRepository;
use Doctrine\ORM\EntityManager;
use LogicException;
use Doctrine\Bundle\DoctrineBundle\Registry;

/**
 * 
 */
class ServiceEntityRepository extends EntityRepository implements ServiceEntityRepositoryInterface
{
    /**
     * All the arguments used by the constructor of the user-defined service repository
     *
     * @var mixed[]
     */
    private $args;

    /** @var Registry */
    private $registry;

    /** @var EntityManager */
    private $manager;

    /** @var string[] */
    private static $nonDefaultUserRepositoriesIds = [];

    /**
     * @param string $entityClass The class name of the entity this repository manages
     */
    public function __construct(ManagerRegistry $registry, string $entityClass, ...$args)
    {
        $userSpecifiedManager = null;

        // BC
        if (isset($args[0])) {
            $this->args = $args[0];

            // Is there a specific entity manager to use here? To be searched in the extra args we internally manage.
            foreach ($this->args as $arg) {
                if ($arg instanceof EntityManager) {
                    $userSpecifiedManager = $arg;
                    break;
                }
            }  
        }

        // Default manager: "first one defined for the entity"
        $manager = $userSpecifiedManager ? : $registry->getManagerForClass($entityClass);

        if ($manager === null) {
            throw new LogicException(sprintf(
                'Could not find the entity manager for class "%s". Check your Doctrine configuration to make sure it is configured to load this entity’s metadata.',
                $entityClass
            ));
        }

        $this->registry = $registry;
        $this->manager  = $manager;

        parent::__construct($manager, $manager->getClassMetadata($entityClass));
    }

    /**
     * @param string|EntityManager $entityManagerRef
     *
     * @return self
     */
    public function withManager($entityManagerRef)
    {
        if (! $this->supportsMultipleManagers()) {
            // BC: Fails silently with the old behavior
            return $this;
        }

        // getFrom: instance
        if ($entityManagerRef instanceof EntityManager) {
            $userSpecifiedManager = $entityManagerRef;
        } else { // getFrom: name
            $userSpecifiedManager = $this->registry->getManager($entityManagerRef);
        }

        // If a different manager than the autowired-one is required, instantiate a new user's service-repository with the right one.
        if ($userSpecifiedManager !== $this->manager) {
            $managerInstanceId = static::class . ':' . spl_object_hash($userSpecifiedManager);

            if (! isset(self::$nonDefaultUserRepositoriesIds[$managerInstanceId])) {
                // Use the very-same arguments for the new instance but the manager name is added to be used as the default manager.
                $args   = $this->args;
                $args[] = $userSpecifiedManager;

                self::$nonDefaultUserRepositoriesIds[$managerInstanceId] = new static(...$args);
            }

            return self::$nonDefaultUserRepositoriesIds[$managerInstanceId];
        }

        return $this;
    }

    /**
     * @return bool
     */
    public function supportsMultipleManagers(): bool
    {
        return $this->args !== null;
    }
}

And to fix the unexpected current behavior reported above, a fix in Repository\ContainerRepositoryFactory can thus be done as follow:

    /**
     * {@inheritdoc}
     */
    public function getRepository(EntityManagerInterface $entityManager, $entityName)
    {
        /*
        $metadata            = $entityManager->getClassMetadata($entityName);
        $repositoryServiceId = $metadata->customRepositoryClassName;

        $customRepositoryName = $metadata->customRepositoryClassName;

        if ($customRepositoryName !== null) {
            // fetch from the container
            if ($this->container && $this->container->has($customRepositoryName)) {
                $repository = $this->container->get($customRepositoryName);

                if (! $repository instanceof ObjectRepository) {
                    throw new RuntimeException(sprintf('The service "%s" must implement ObjectRepository (or extend a base class, like ServiceEntityRepository).', $repositoryServiceId));
                }
*/
                // Use the correct manager (BC)
                if(method_exists($repository, 'withManager')) {
                    return $repository->withManager($entityManager);
                }
/*
                return $repository;
            }

            // if not in the container but the class/id implements the interface, throw an error
            if (is_a($customRepositoryName, ServiceEntityRepositoryInterface::class, true)) {
                throw new RuntimeException(sprintf('The "%s" entity repository implements "%s", but its service could not be found. Make sure the service exists and is tagged with "%s".', $customRepositoryName, ServiceEntityRepositoryInterface::class, ServiceRepositoryCompilerPass::REPOSITORY_SERVICE_TAG));
            }

            if (! class_exists($customRepositoryName)) {
                throw new RuntimeException(sprintf('The "%s" entity has a repositoryClass set to "%s", but this is not a valid class. Check your class naming. If this is meant to be a service id, make sure this service exists and is tagged with "%s".', $metadata->name, $customRepositoryName, ServiceRepositoryCompilerPass::REPOSITORY_SERVICE_TAG));
            }

            // allow the repository to be created below
        }

        return $this->getOrCreateRepository($entityManager, $metadata);
*/
    }

Any toughts?

The text was updated successfully, but these errors were encountered:

…rine#1044

stof · 2019-11-05T10:35:52Z

I'm -1 for this solution, for several reasons:

first, registering the same entity in multiple entity managers is already an unsupported use case in DoctrineBundle (btw, this can lead to many weird cases because an entity manager could think that it should deal with this object, while it is managed by the other one)
your proposal would not work for auto-registration of repositories, as $extra won't ever be autowired
this logic in getRepository will precisely break things, as the entity manager received here will never be the one managed in the container, due to the usage of a lazy proxy to wrap the entity manager to be resettable (while still allowing to inject the entity manager as a service). This is precisely the reason why "normal" doctrine repositories should not be defined as services.

IMO, the correct way to support setups with multiple entity managers would be to make sure that we define separate repository factories for each entity manager (I think we might be using the same one right now), and providing a way to register different entity repository services in each of them (which of course won't work with auto-registration of services as it will require multiple instance, but that's fine to me). Also, it would avoid adding extra complexity in repository classes at runtime.

ipernet · 2019-11-05T11:07:39Z

Hi @stof

First, registering the same entity in multiple entity managers is already an unsupported use case...

To me it's the responsibility of the developer to ensure a proper isolation when using many entity managers so of course many bad things could happen if not done properly, but like with anything else.

your proposal would not work for auto-registration of repositories, as $extra won't ever be autowired

$extra won't be autowired but it does not prevent the repository to be currently auto-registered like any others service repositories (else it would be pointless). Let me know if I'm missing something here, but the auto-injection of any services in such repo is not impacted and the repo auto-registers just fine.

Anyway, thanks for the feedback, I'll dig into your 3rd point so if you can link to the code you're talking about that would be nice!

alcaeus · 2019-11-05T20:22:20Z

I agree with @stof here. We discourage managing one entity with multiple managers, but are aware that there may be specific use-cases to do so. As I've said previously, service repositories aim to cover 90% of all use-cases, while keeping the entire system flexible enough to allow users to build a solution for their problem.

In this case, I don't see how your solution would make it any easier, as you are now essentially just injecting a repository factory into a controller, except that you call it a repository.

Please note that we may change the concept of multiple entity managers in the future: it can certainly solve some problems, but it can also cause a lot of problems if abused. Service repositories is one of those features that breaks if the multiple manager concept is abused, and I believe it's best if people figure out a solution for their specific problem rather than us trying to come up with a generic one-size-fits-all type solution.

ipernet · 2019-11-05T22:40:29Z

Thanks for your comments! Closing the issue till there is more to talk about on the subject!

ipernet mentioned this issue Nov 4, 2019

With multiple Entity Managers (EM) the Repository uses only the mapped EM for the Entity symfony/symfony-docs#9878

Closed

ipernet added a commit to ipernet/DoctrineBundle that referenced this issue Nov 4, 2019

Allow service repositories to be used with more than one manager doct…

aefd94c

…rine#1044

ipernet added a commit to ipernet/DoctrineBundle that referenced this issue Nov 4, 2019

Allow service repositories to be used with more than one manager doct…

5c7015d

…rine#1044

ipernet added a commit to ipernet/DoctrineBundle that referenced this issue Nov 4, 2019

Allow service repositories to be used with more than one manager doct…

fce68a1

…rine#1044

ipernet mentioned this issue Nov 4, 2019

Allow service repositories to be used with more than one manager #1045

Closed

ipernet closed this as completed Nov 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow service repositories to be used with more than one manager #1044

Allow service repositories to be used with more than one manager #1044

ipernet commented Nov 4, 2019 •

edited

Loading

stof commented Nov 5, 2019

ipernet commented Nov 5, 2019 •

edited

Loading

alcaeus commented Nov 5, 2019

ipernet commented Nov 5, 2019 •

edited

Loading

Allow service repositories to be used with more than one manager #1044

Allow service repositories to be used with more than one manager #1044

Comments

ipernet commented Nov 4, 2019 • edited Loading

stof commented Nov 5, 2019

ipernet commented Nov 5, 2019 • edited Loading

alcaeus commented Nov 5, 2019

ipernet commented Nov 5, 2019 • edited Loading

ipernet commented Nov 4, 2019 •

edited

Loading

ipernet commented Nov 5, 2019 •

edited

Loading

ipernet commented Nov 5, 2019 •

edited

Loading