doctrine / DoctrineBundle

Symfony Bundle for Doctrine ORM and DBAL
https://www.doctrine-project.org/projects/doctrine-bundle.html
MIT License
4.72k stars 454 forks source link

Support using custom service via configuration for SLC factory #1814

Open petrisorciprian-vitals opened 3 months ago

petrisorciprian-vitals commented 3 months ago

Hello, would it be possible to enable autowiring/autoconfiguration when a custom factory is specified for the second level cache?

I attempted enabling autowiring/autoconfiguring for my custom factory in services.yaml, as such:

App\Doctrine\CacheFactory:
   autowire: true
   autoconfigure: true

However this does not work, only the two arguments specified in the DoctrineExtension.php are passed.

My only solution to fix this was to enable them via a compiler pass:

    public function process(ContainerBuilder $container): void
    {
        $serviceIds = $container->getServiceIds();
        foreach ($serviceIds as $serviceId) {
            if (str_ends_with($serviceId, 'second_level_cache.default_cache_factory')) {
                $definition = $container->getDefinition($serviceId);
                if (CacheFactory::class === $definition->getClass()) {
                    $definition->setAutoconfigured(true);
                    $definition->setAutowired(true);
                }
            }
        }
    }

I probably could get away by adding the service ID itself in my services.yaml and enabling autowiring/autoconfiguring that way, but it needs to be done for each factory ( the IDs the bundle generate are naturally different), which is very repetitive when you have multiple connections/ems/2nd level caches, which is why I am proposing this change in the bundle itself.

I believe this should be as simple as calling ->autowire(true) and ->autoconfigure(true), as shown above, in the DoctrineExtension.php:

https://github.com/doctrine/DoctrineBundle/blob/1ea2b5bc483622367a3bfbf60de2be972536cd9d/src/DependencyInjection/DoctrineExtension.php#L931-L936

I can make this change, if you agree with it and do not see any downsides.

ostrolucky commented 3 months ago

Autowiring in bundles is discouraged. However, I think you were on the right track, it's just that it seems you are confused about service ids. Keys in your services.yaml file are service IDs, not class names. It just happens that some of the service IDs equal their class names, which is not the case here. Have you tried

- App\Doctrine\CacheFactory:
+ doctrine.orm.default_second_level_cache.default_cache_factory:
+  class: App\Doctrine\CacheFactory:
   autowire: true
   autoconfigure: true

?

petrisorciprian-vitals commented 3 months ago

Thank you for the quick answer!

I have tried the suggestion above, and, while it works, it means that I have to define the factory based on this internal ID (which requires knowledge of how this bundle formats it in the first place). Also to make your suggestion work, I have to remove the orm.entity_managers.default.second_level_cache.factory definition (it did not work with it in place and your suggestion).

petrisorciprian-vitals commented 3 months ago

@ostrolucky Sorry about the confusion, it looks like your suggestion does not work either, actually.

So:

services:
    doctrine.orm.default_second_level_cache.default_cache_factory:
        class: App\Doctrine\Caching\CacheFactory
        autowire: true
        autoconfigure: true

Does not work on its own, it just uses the DefaultCacheFactory implementation instead of my own. If I specify the cache factory in the doctrine.yaml and keep the service declaration above, like so:

services:
    doctrine.orm.default_second_level_cache.default_cache_factory:
        class: App\Doctrine\Caching\CacheFactory
        autowire: true
        autoconfigure: true
doctrine:
    orm:
           entity_managers:
                  default:
                      second_level_cache:
                           enabled: true
                           factory: App\Doctrine\Caching\CacheFactory

I get an error when generating the cache: Too few arguments to function App\Doctrine\Caching\CacheFactory::__construct(), 2 passed in /home/user/Projects/project/var/cache/dev/ContainerSnOQ4fQ/App_KernelDevDebugContainer.php on line 4646 and exactly 3 expected"

So, it seems the only viable option is the kernel pass.

ostrolucky commented 3 months ago

doctrine.orm.entity_managers.*.second_level_cache.factory value at the moment represents a FQCN, not a service/service id. We use it to create new service. We could perhaps extend it to support service id too. PR welcome, but I think it can be tricky, because DoctrineExtension wouldn't know about app services.