ClearObjectManagerStrategy is mostly not useful; add better strategy for clearing entity manager

[iwyg]

Hi there

Fetching the ObjectManager in onClear can never be negated since ObjectManagerAwareInterface->getObjectManager() can't return null.

This should be wrapped in a try/catch

ClearObjectManagerStrategy

[roelvanduijnhoven]

Fetching the ObjectManager in onClear can never be negated since ObjectManagerAwareInterface->getObjectManager() can't return null.

Makes sense! Although I don't follow why we should try/catch here?

---

But TBH @iwyg; the ClearObjectManagerStrategy implementation kinda sucks if you ask me. We use a strategy in which we inject the EntitytManager. And then we can reset, regardless of the job that ran. Namely:

class ClearEntityManagerStrategy extends AbstractStrategy
{
    private EntityManager $entityManager;

    public function __construct(EntityManager $entityManager)
    {
        parent::__construct();
        $this->entityManager = $entityManager;
    }

    public function attach(EventManagerInterface $events, $priority = 1)
    {
        $this->listeners[] = $events->attach(
            AbstractWorkerEvent::EVENT_PROCESS_JOB,
            [$this, 'onClear'],
            1000
        );
    }

    public function onClear(ProcessJobEvent $event): void
    {
        $this->entityManager->clear();
    }
}

I feel that would be a good thing to add to this repo. And deprecate this strategy. Do you agree @iwyg?

[iwyg]

@roelvanduijnhoven ClearObjectManagerStrategy strategy will work as log as you only expect one EntityManager instance in the whole application (which in most cases is a sane guess), however you can easily deal with multiple EM instances for different DB connections. etc.

I think the real culprit here is the ObjectManagerAwareInterface interface for defining both getters and setters but don't allow an optional getter return value.

Makes sense! Although I don't follow why we should try/catch here?

Something like this maybe?

try {
  $job->getObjectManager()->clear();
} catch (\Throwable $err) {
 // report error, etc
}

[roelvanduijnhoven]

@iwyg To be able to use the current strategy, you will need to inject the ObjectManager into every job. And write some boiler plate. Is that what you currently do?

My proposed strategy seems way simpler! You are correct that my proposed strategy won't work for multiple entity managers. But.. for 99% of the cases it will help. And it will be far easier. Right? :)

As with regards to your suggestion. For me it does not make sense to make a nullable return type. Why would you expose the interface, if you don't have an object manager attached? In that case, why not simply omit the interface altogether? Probably something I am missing here; so please do let me know what works for you.

Still: I feel like the best path forward, is to expose a new strategy, and promote that as the default one. And keep this one, as is (although we should drop the unnecessary if condition if (!$manager = $job->getObjectManager())).

[iwyg]

Is that what you currently do?

In deed. I use a custom abstract job factory that handles OM injection

<?php
namespace Queue\Job\Factory;

use Doctrine\ORM\EntityManager;
use Doctrine\Persistence\ObjectManager;
use DoctrineModule\Persistence\ObjectManagerAwareInterface;
use Interop\Container\ContainerInterface;
use Queue\Job\AbstractJob;
use SlmQueue\Job\JobInterface;
use Laminas\ServiceManager\Exception\ServiceNotCreatedException;
use Laminas\ServiceManager\Factory\FactoryInterface;

/**
 * @psalm-template T of JobInterface
 */
abstract class AbstractJobFactory implements FactoryInterface {
    /** @psalm-var class-string<T> */
    private string $jobClass = JobInterface::class;
    /** @psalm-var class-string<ObjectManager>|string */
    protected string $objectManagerClass = EntityManager::class;

    final public function __construct() {
        $this->jobClass = $this->jobClass();
    }

    /** @psalm-return class-string<T> */
    abstract protected function jobClass(): string;

    /**
     * @param ContainerInterface $container
     * @param string             $requestedName
     * @param array|null         $options
     *
     * @return T
     */
    public function __invoke(ContainerInterface $container, $requestedName, array $options = null) : JobInterface {
        $class = $this->jobClass;
        /** @var JobInterface|AbstractJob $job */
        $job =  new $class(...$this->getDependencies(
            $container,
            $requestedName,
            $options
        ));

        /**
         * @psalm-var JobInterface|ObjectManagerAwareInterface|\SlmQueueDoctrine\Persistence\ObjectManagerAwareInterface $job
         */
        if ($job instanceof ObjectManagerAwareInterface || $job instanceof \SlmQueueDoctrine\Persistence\ObjectManagerAwareInterface) {
            $job->setObjectManager($container->get($this->objectManagerClass));
        }

        return $job;
    }

    /**
     * @param ContainerInterface $container
     * @param string             $requestedName
     * @param array|null         $options
     *
     * @return array<array-key, mixed>
     */
    abstract protected function getDependencies(ContainerInterface $container, string $requestedName, array $options = null) : array;


}

I feel like the best path forward, is to expose a new strategy, and promote that as the default one

Alright then. I'd suggest you provide some upgrade path then? ClearObjectManagerStrategy is part of the default cli config and sudden changes may break stuff :)

[roelvanduijnhoven]

@iwyg Nice solution :). But the new solution will remove the need for this abstract factory 👏.

I don't have the time currently to add such a thing currently. But.. you can do this easily as follows. This is wat we at our company have been doing for ages: create and configure the following new strategy. And that should remove the need for your custom factory.

class ClearEntityManagerStrategy extends AbstractStrategy
{
    private EntityManager $entityManager;

    public function __construct(EntityManager $entityManager)
    {
        parent::__construct();
        $this->entityManager = $entityManager;
    }

    public function attach(EventManagerInterface $events, $priority = 1)
    {
        $this->listeners[] = $events->attach(
            AbstractWorkerEvent::EVENT_PROCESS_JOB,
            [$this, 'onClear'],
            1000
        );
    }

    public function onClear(ProcessJobEvent $event): void
    {
        $this->entityManager->clear();
    }
}

And.. obviously feel free to add a PR if you have time for it :).