Added: ability to choose different entity manager

[balabis]

Here the discussion: #961 (comment)

[balabis]

Hey, @Steveb-p, would you mind checking this PR?

[Steveb-p]

I'll try to look it up more thoroughly in the evening.

[Steveb-p]

@balabis Have you tested this against your application?

I've looked through the extension and Enqueue\JobQueue\Tests\Functional\Job\JobStorageTest, and wasn't able to find the part that injects enqueue's job entity mapping into Doctrine. Are you sure it's made automatically?

enqueue:
    default:
        # plus basic bundle configuration

        job: true
        
        # by default bundle will add a default orm mapping configuration 
        # if you define custom mappings, you can specify which entity manager to use here 
        entity_manager_name: ~

This part from the original documentation was responsible for injecting Job metadata into Doctrine entity manager. I'm pretty sure it's required, since I haven't found any other references to mapping metadata in bundle files.

I'm pretty sure tests would fail the moment you'd start using a different doctrine entity manager than default - reason being that EntityManager would not contain metadata required to work with Job entities.

To test your branch against your application you can use composer's built-in functionality which allows using one's fork instead of a library.
https://getcomposer.org/doc/05-repositories.md#loading-a-package-from-a-vcs-repository

In your case it should look something like this:

{
    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/balabis/enqueue-dev.git"
        }
    ],
    "require": {
        "enqueue/job-queue": "dev-master"
    }
}

[balabis]

enqueue-bundle/DependencyInjection/EnqueueExtension lines 179-198 prepends the Doctrine mappings by default:

foreach ($container->getExtensionConfig('doctrine') as $config) {
            // do not register mappings if dbal not configured.
            if (!empty($config['dbal'])) {
                $rc = new \ReflectionClass(Job::class);
                $jobQueueRootDir = dirname($rc->getFileName());
                $container->prependExtensionConfig('doctrine', [
                    'orm' => [
                        'mappings' => [
                            'enqueue_job_queue' => [
                                'is_bundle' => false,
                                'type' => 'xml',
                                'dir' => $jobQueueRootDir.'/Doctrine/mapping',
                                'prefix' => 'Enqueue\JobQueue\Doctrine\Entity',
                            ],
                        ],
                    ],
                ]);
                break;
            }
        }

The part I removed from docs suggests the user that he should define the Doctrine mappings manually, but in reality that mapping is injected behind the scenes with the code above.

So this default mapping is always there, but the user, if he so choses, can define his own mappings (thats what we did). And now you can define as many mappings as you want, but JobStorage will always use the default one.

I have used this code with our application. If I specify entity_manager_name, JobStorage will use it and queue will be consumed using this entity manager.

[Steveb-p]

Well, I'm blind apparently. Thanks for finding it. Goes to show that I'm not really that much more familiar with enqueue-bundle itself than you 😄

However, instead of injecting an entity name into JobStorage and creating a mapping manually (which you still have to do, so that part in documentation about adding a mapping would be even more important ❕ ), I would make EnqueueExtension inject the mapping properly to specified entity manager.
This would prevent class metadata from existing in multiple managers and would allow doctrine's ManagerRegistry to properly resolve manager when getManagerByClass is called (in user code, perhaps).

Could you please see if this is possible there?

[balabis]

Ok, will see what I can do.

[balabis]

What do you think if instead of having:

foreach ($container->getExtensionConfig('doctrine') as $config) {
            // do not register mappings if dbal not configured.
            if (!empty($config['dbal'])) {
                $rc = new \ReflectionClass(Job::class);
                $jobQueueRootDir = dirname($rc->getFileName());
                $container->prependExtensionConfig('doctrine', [
                    'orm' => [
                        'mappings' => [
                            'enqueue_job_queue' => [
                                'is_bundle' => false,
                                'type' => 'xml',
                                'dir' => $jobQueueRootDir.'/Doctrine/mapping',
                                'prefix' => 'Enqueue\JobQueue\Doctrine\Entity',
                            ],
                        ],
                    ],
                ]);
                break;
            }
        }

We'd have something like this:

foreach ($container->getExtensionConfig('doctrine') as $config) {
            // do not register mappings if dbal not configured.
            if (!empty($config['dbal'])) {
                $rc = new \ReflectionClass(Job::class);
                $jobQueueRootDir = dirname($rc->getFileName());
                $mappingExists = false;

                array_walk_recursive($config['orm'], function ($item, $key) use (&$mappingExists) {
                   if ($key === 'prefix' && $item === 'Enqueue\JobQueue\Doctrine\Entity') {
                       $mappingExists = true;
                   }
                });

                if (!$mappingExists) {
                    $container->prependExtensionConfig('doctrine', [
                        'orm' => [
                            'mappings' => [
                                'enqueue_job_queue' => [
                                    'is_bundle' => false,
                                    'type' => 'xml',
                                    'dir' => $jobQueueRootDir.'/Doctrine/mapping',
                                    'prefix' => 'Enqueue\JobQueue\Doctrine\Entity',
                                ],
                            ],
                        ],
                    ]);
                }

                break;
            }
        }

The idea here would be to prepend the default mapping only if the user has not defined any mappings.

[Steveb-p]

The idea here would be to prepend the default mapping only if the user has not defined any mappings.

I would consider a different approach. Instead of trying to establish if there are any mappings provided by the user, create a configuration setting to explicitly inject - with the default being true, to maintain current behavior (which would nicely prevent BC breaks).

This is also important due to the fact that you can technically provide your own Job classes as far as I'm aware (might be wrong though - as I said, I'm not familiar with JobQueue component). Code proposed would inject mapping in this case anyway.

[balabis]

Hey, @Steveb-p, mind checking the PR again?

[Steveb-p]

Hey, @Steveb-p, mind checking the PR again?

Yeah, will do in the evening or tomorrow morning.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[balabis]

Hey, @Steveb-p , have you checked it?

[Steveb-p]

Sorry for taking so long.

There are two things I'd like you to look at at this point. Otherwise than that it look ok to me - I know that code style fixer might be a PITA that causes a lot of changes here (which can probably be avoided by squashing commits and amending/removing those changes) but I think personally it's ok to fix them right away.

[balabis]

Thanks @Steveb-p
I've edited these two things and created a new PR #1081 for it.