doctrine / DoctrineModule

Doctrine Module for Laminas
http://www.doctrine-project.org/projects/doctrine-module.html
MIT License
398 stars 270 forks source link

Migration to v6 leads to cache issues #815

Closed boesing closed 11 months ago

boesing commented 1 year ago

Hey there,

I am currently migrating to v6 of this module. In prior versions (and AFAIR its doctrine default), array was a proper cache name which used some kind of in-memory cache adapter.

Since that worked out of the box, a service named array was never provided by our configuration which now leads to a type-error as LaminasCacheStorage is invoked with string type due to src/Service/CacheFactory.php:63.

IMHO, this module should somehow detect doctrine.cache.array and properly maps this to Memory adapter or WDYT? Would love to get some feedback here. Thanks in advance.

RodriAndreotti commented 1 year ago

Same here!

image

Could you provide a updated config example?

driehle commented 1 year ago

It should normally work as before. Did you remember to load the respective modules in your Laminas setup?

Here is a working example for a basic filesystem cache:

config/application.config.php:

return [
    'modules' => [
        'Laminas\Cache',
        'Laminas\Cache\Storage\Adapter\Memory',
        'Laminas\Cache\Storage\Adapter\Filesystem',
        'Laminas\Serializer',
        // [...]
    ],
    // [...]
];

config/autoload/doctrine.global.php:

return [
    'caches' => [
        'doctrinemodule.cache.filesystem' => [
            'options' => [
                'cache_dir' => './data/cache/doctrine-cache/',
            ],
        ],
    ],
    'doctrine' => [
        'configuration' => [
            'orm_default' => [
                'proxy_dir' => './data/cache/doctrine-proxy/',
                'metadata_cache' => 'filesystem',
                'query_cache' => 'filesystem',
                'result_cache' => 'array',
                'hydration_cache' => 'array',
                'generate_proxies' => false,
            ],
        ],
        // [...]
    ],
];

As you can see I have set up specific folders, one for doctrine cache and one for the doctrine proxies files.

datasage commented 1 year ago

This seems to be conflicting with the Di module. If its enabled, this error can occur, if disabled, it seems to go away.

boesing commented 11 months ago

I simply added a factory for doctrine.cache.array in the meantime. From what I can say is that I simply updated composer dependencies in a project which worked with v5 and stopped working with v6.

I remember that it was related to array. I can see that your example configuration does not contain array anywhere.

This is CacheFactory in v5: image

This is CacheFactory in v6: image

Due to the switch statement, we do end up in new $class($instance) but $instance was null in v5 and is now doctrinemodule.cache.array in v6.

I'd expect v6 to provide a service named doctrinemodule.cache.array to resolve to the Memory adapter which is required anyway.

driehle commented 11 months ago

I'd expect v6 to provide a service named doctrinemodule.cache.array to resolve to the Memory adapter which is required anyway.

Indeed, this is what should happen and I am surprised we don't to so, to be honest. So this is a bug the the compatibility layer we introduced in v6.

juizmill commented 11 months ago

It should be an array adapter and not an in-memory adapter

https://github.com/doctrine/DoctrineModule/blob/2dc978ef6a7b4d33c49c3f182af86ac98dd52693/src/ConfigProvider.php#L120

image

boesing commented 11 months ago

Okay, I think the main problem is, that the whole cache logic has changed to laminas-cache without providing module specific factories. Unless there is the StorageCacheAbstractServiceFactory is registered as an abstract_factories entry to service_manager/dependencies, projects wont be able to use backed cache backends provided by this project.

Maybe I missed this specific part in the documentation. I'd rather encourage you to avoid using caches for the module-specific caches and instead provide dedicated factories. As you can see, laminas-cache does not register that abstract factory by-default to the configuration and therefore, thats an essential part of the whole setup of this project.

Imho, this project should have dedicated factory for each cache backend (i.e. apcu, memory, filesystem, memcached and redis). So instead of using caches array, you simply add a factory which then uses StorageAdapterFactoryInterface service to create storage adapters. That won't rely on the StorageCacheAbstractServiceFactory (which is not registered to the service manager - on purpose) and still can use this "configuration" approach.

TomHAnderson commented 11 months ago

@boesing thanks for that summary. From your explanation, shouldn't adding to the documentation the need to register StorageCacheAbstractServiceFactory as an abstract factory solve this issue?

I can't imagine the headache of figuring that out.

boesing commented 11 months ago

It would be a solution, yes. Not the best one but a solution. 😅

driehle commented 11 months ago

I think we should prefer the solution with custom factories, as Maximilian suggested. Might look into it in about two weeks. Let's not rush with a badish solution.

boesing commented 11 months ago

Hm, it seems that we do actually expose abstract_factories entry for StorageCacheAbstractServiceFactory, so I guess I can close here as this is actually not a problem of this component but from the library I ran my tests in.

Anyways, I usually try to stick with StorageAdapterFactoryInterface but since laminas-cache is exposing the abstract factory anyways, I think there is no TODO here. Sorry for bothering you guys.

You might want to drop this from 6.0.5 milestone.