Support for doctrine cache 2 #72

mrVrAlex · 2022-05-04T20:08:54Z

Feature #71

Ocramius

Check CI failures:

composer.lock needs to be updated
need to check that lowest dependency ranges are tested
potentially worth simply dropping psr/cache:^1 and doctrine/persistence:^2.3 entirely

mrVrAlex · 2022-05-05T19:44:56Z

potentially worth simply dropping psr/cache:^1 and doctrine/persistence:^2.3 entirel

Yeah, I think about it too. But drop persistence 2.3 means drop also ORM 2.11.
I planed for release 3.6 drop only doctrine/cache:^1 (allow only 2.0) and in future release (3.7?) can drop doctrine/persistence:^2.3

What you think?

Ocramius · 2022-05-05T20:04:02Z

IMO just bump aggressively 🤔

Ocramius

CI failures indicate some trouble with PHP 7.4. Should we drop it?

As for the rest: lots of breaking changes 😱

Can we somehow isolate this to a smaller change set, without affecting caching and similar?

composer.json

Ocramius · 2022-05-25T15:20:51Z

src/CacheFactory.php

-        $instance = null;
-
-        if (array_key_exists('instance', $config)) {
-            $instance = is_string($config['instance']) ? $container->get($config['instance']) : $config['instance'];
-        }
-
-        switch ($config['class']) {
-            case FilesystemCache::class:
-            case PhpFileCache::class:
-                $cache = new $config['class']($config['directory']);
-                break;
-
-            case PredisCache::class:
-                assert($instance !== null);
-                $cache = new PredisCache($instance);
-                break;
-
-            case ChainCache::class:
-                $providers = array_map(
-                    function ($provider) use ($container): CacheProvider {
-                        return $this->createWithConfig($container, $provider);
-                    },
-                    is_array($config['providers']) ? $config['providers'] : []
-                );
-                $cache     = new ChainCache($providers);
-                break;
-
-            default:
-                $cache = $container->has($config['class']) ? $container->get($config['class']) : new $config['class']();
-        }
-
-        if ($cache instanceof MemcachedCache) {
-            assert($instance !== null);
-            $cache->setMemcached($instance);
-        } elseif ($cache instanceof RedisCache) {
-            assert($instance !== null);
-            $cache->setRedis($instance);
-        }


Hmm, this removal will break a lot of stuff 🤔

If we want stay this code, then need return support doctrine/cache 1.x version to composer.json. But in 2.x anyway these such classes do not exists, so update to 2.x will break cache configuration for doctrine and need write own PSR compatibility factory for cache providers.

Ocramius · 2022-05-25T15:21:40Z

src/ConfigurationFactory.php

+            //'metadata_cache' => 'array',
+            //'query_cache' => 'array',
+            //'result_cache' => 'array',
+            //'hydration_cache' => 'array',


Ocramius · 2022-05-25T15:22:02Z

src/ConfigurationFactory.php

+        foreach (['metadata_cache', 'query_cache', 'result_cache', 'hydration_cache'] as $cacheKey) {
+            if (isset($config[$cacheKey])) {
+                $cache = $this->retrieveDependency(
+                    $container,
+                    $config[$cacheKey],
+                    'cache',
+                    CacheFactory::class
+                );
+
+                $this->processCacheImplementation(
+                    $configuration,
+                    $cache,
+                    \Closure::fromCallable([$configuration, str_replace('_', '', ucwords('set_' . $cacheKey, '_'))])
+                );
+            }


Don't think this should be refactored this way - makes it much harder to understand.

Ocramius · 2022-05-25T15:22:50Z

src/DriverFactory.php

@@ -49,7 +49,7 @@ protected function createWithConfig(ContainerInterface $container, string $confi
        if (
            $config['class'] !== AttributeDriver::class
            && ! is_subclass_of($config['class'], AttributeDriver::class)
-            && is_subclass_of($config['class'], AnnotationDriver::class)
+            && $config['class'] === AnnotationDriver::class


is_subclass_of() should probably stay? We're checking if $config['class'] operates like an AnnotationDriver, not if it IS the AnnotationDriver 🤔

PowerKiKi · 2023-01-02T04:24:10Z

@mrVrAlex are you still willing to work on this PR ? or should the community step up to finish it ?

mrVrAlex · 2023-01-02T08:22:56Z

@mrVrAlex are you still willing to work on this PR ? or should the community step up to finish it ?

Yes, I will rebase this and will drop support doctrine persistence 2.x & cache 1.x as renovate bot want it)

PowerKiKi · 2023-01-13T00:58:37Z

I created opsway#2 to simplify this PR by not-adding StubCache when we can easily mock it with PHPUnit.

Let me know if you'd rather have in another format.

Slamdunk · 2023-01-31T15:03:40Z

@mrVrAlex #95 has been merged and released, can you rebase this PR on top of the new 3.10.x branch please?

mrVrAlex · 2023-01-31T17:46:32Z

@mrVrAlex #95 has been merged and released, can you rebase this PR on top of the new 3.10.x branch please?

Yes, will do.

test/CacheFactoryTest.php

composer.json

example/full-config.php

Ocramius · 2023-02-06T09:21:45Z

psalm.xml.dist

+        <UndefinedClass>
+            <errorLevel type="suppress">
+                <referencedClass name="Doctrine\Common\Cache\Cache"/>
+                <referencedClass name="Doctrine\Common\Cache\CacheProvider"/>
+                <referencedClass name="Doctrine\Common\Cache\Psr6\CacheAdapter"/>
+            </errorLevel>
+        </UndefinedClass>
+        <UndefinedDocblockClass>
+            <errorLevel type="suppress">
+                <referencedClass name="Doctrine\Common\Cache\Cache"/>
+            </errorLevel>
+        </UndefinedDocblockClass>


These should go away before merging: the fact that we reference these classes, yet they are not explicit dependencies, is a problem.

those class referencesses are in instanceof-operator to check package availability (before replace cache with bundled CacheAdapter) and special ::class consts in InvalidArgumentException::fromUnsupportedCache(). So as for me its safe for optional dependency.

Bytheway, until doctrine/orm will not bump doctrine/dbal:^4.0 all project will have doctrine/cache, but whats the reason to declare it as that package dependency.

As less error-prone approach I could inline suppression:

@psalm-suppress UndefinedClass, UnusedPsalmSuppress

If we say that we go with doctrine/cache:^2, then we should most certainly not have any undefined classes at all.

If the support for doctrine/cache requires referencing classes from doctrine/cache, then the dependency should be declared in composer.json.

This applies to instanceof and ::class references alike, but I'll check the patch in more detail when I have more time :)

no its not about doctrine/cache:^2 anymore. We allow v1 - #72 (comment) - but drop logic specific for this version (i mean instantiation support in CacheFactory). But stays logic for both versions:

wrap into PSR-6 interface adapter required by ORM

set namespace from $config['doctrine']['cache'][$configKey]['namespace'] in case when no conventional cache service id "doctrine.cache.{$configKey}" (assume its fully build for usage with adapter).

With knowledge that doctrine/cache:^2 is an intermediate solution for migrating to the PSR-6 cache interface. It would be great to make package optional dependency.

doctrine/cache should be moved from require to require-dev then, to satisfy psalm, and not impose it on users.

yes, this will solve problem with ugly psalm suppression. It looks like ORM cache types are stable (metadata, query, result, hydration) so no problem that contributors may not aware about disappearing doctrine/cache package.

But there is other blocker for optional dependency for newbies - composer-require-checker - which require add composer-require-checker.json with list optional class lists:

{ "symbol-whitelist" : [ "Doctrine\\Common\\Cache\\Cache", "Doctrine\\Common\\Cache\\CacheProvider", "Doctrine\\Common\\Cache\\Psr6\\CacheAdapter" ] }

looks like composer-require-checker struggle against complexity caused by optional dependencies, so I will return doctrine/cache to required dependencies

s/confused/correct.

The idea of composer-require-checker is precisely to disallow the existence of symbols that aren't part of the dependency set.

Ocramius · 2023-02-06T09:26:48Z

src/CacheFactory.php

@@ -42,126 +29,28 @@ protected function createWithConfig(ContainerInterface $container, string $confi
            throw OutOfBoundsException::forMissingConfigKey('class');
        }

-        $instance = null;
+        $cache = $container->has($config['class']) ? $container->get($config['class']) : new $config['class']();


Since we are no longer supporting the various cache instance types, new $config['class'] only remains useful/relevant for something like a NullCache: we can probably always use $container->get() for all scenarios.

also for old default cache implementation - 'array'. So its usefull to simplify migration by add config:

'doctrine' => [ 'cache' => [ 'array' => ['class' => \Symfony\Component\Cache\Adapter\ArrayAdapter::class], ], ],

without forcing adding useless services into container

This would be a sufficient approach, but a void cache is preferable.

While it may be tempting to use array as a default, it is risky to do so for caches other than DQL and metadata caches, because array can break in long-running processes.

For result caches, a void/null cache would be best as a default.

I would let clients decide which caches to use for which purposes. Also caches are optional, so we can avoid create own implementation of void/null/blackhole cache or be locked to another vendor (e.g. symfony/cache).

Above in #72 (comment) its just example how simple could be migration for client who use our default config with 'array' cache and if we will left constructor call in CacheFactory. We have no dependency on symfony/cache.

Ocramius · 2023-02-06T09:28:44Z

src/ConfigurationFactory.php

+        if (isset($config['metadata_cache'])) {
+            $metadataCache = $this->retrieveDependency(
+                $container,
+                $config['metadata_cache'],
+                'cache',
+                CacheFactory::class
+            );
+
+            $this->processCacheImplementation(
+                $metadataCache,
+                $configuration->setMetadataCache(...),
+            );
+        }
+
+        if (isset($config['query_cache'])) {
+            $queryCache = $this->retrieveDependency(
+                $container,
+                $config['query_cache'],
+                'cache',
+                CacheFactory::class
+            );
+
+            $this->processCacheImplementation(
+                $queryCache,
+                $configuration->setQueryCache(...),
+            );
+        }
+
+        if (isset($config['result_cache'])) {
+            $resultCache = $this->retrieveDependency(
+                $container,
+                $config['result_cache'],
+                'cache',
+                CacheFactory::class
+            );

-        $this->processCacheImplementation(
-            $configuration,
-            $hydrationCache,
-            [$configuration, 'setHydrationCache'],
-        );
+            $this->processCacheImplementation(
+                $resultCache,
+                $configuration->setResultCache(...),
+            );
+        }
+
+        if (isset($config['hydration_cache'])) {
+            $hydrationCache = $this->retrieveDependency(
+                $container,
+                $config['hydration_cache'],
+                'cache',
+                CacheFactory::class
+            );
+
+            $this->processCacheImplementation(
+                $hydrationCache,
+                $configuration->setHydrationCache(...),
+            );
+        }


Would probably be best to not have these conditionals, and always fallback to a null/void cache for the basic setup?

I believe that setter-injection is for optional dependencies, so I think no need to force clients to provide a default cache implementation

Yes, but we can provide a default array or void cache for most of these, as stated below in #72 (comment)

our dependencies now have no guaranteed cache implementation, so we can't

🤔 It's a shame psr/cache v2 doesn't come with a null/void cache implementation since that's no doubt going to be the same for all implementations. Long shot but could it be added there?

ok, I will try add null cache implementation to activate optional cache and to simplify ConfigurationFactory. I do not think that psr/cache may get any implementation even if it useful in real world.

Ocramius · 2023-02-06T09:29:07Z

src/ConfigurationFactory.php

-            'metadata_cache' => 'array',
-            'query_cache' => 'array',
-            'result_cache' => 'array',
-            'hydration_cache' => 'array',
+            'metadata_cache' => null,
+            'query_cache' => null,
+            'result_cache' => null,
+            'hydration_cache' => null,


We should probably declare a null/void/blackhole cache default

1. Doctrine (orm, dbal etc.) going to use PSR-6|PSR-16, so doctrine/cache drop own cache implementations which previously could be instantiated in CacheFactory according to config in "doctrine.cache.{$cacheType}". Now only 'class' key supported to either get as service from container or className to be instantiated. 2. ConfigurationFactory lost usage of cache by default, see "doctrine.configuration.{$entityManagerName}" keys: - 'metadata_cache' - 'query_cache' - 'result_cache' - 'hydration_cache' because no 'array' implementation 3. Left 'array' and 'filesystem' caches in full-config.php as example how to migrate to symfony/cache 4. Remove unused $configuration argument from ConfigurationFactory::processCacheImplementation()

…ctory 1. doctrine/orm Configuration class and its parent from doctrine/dbal does not support null in cache setters 2. To avoid `if` in ConfigurationFactory create NullCache implementation of PSR-6 interface. Avoid Doctrine\Common\Cache\Psr6\CacheItem (create own NullCacheItem) because someday we can remove "doctrine/cache" dependency 3. No type-hinting in NullCache and item to be compatible with "psr/cache:^1.0.1"

mrVrAlex · 2023-03-20T12:19:20Z

@Ocramius Any further remarks after last fixes? Soon in near future need add support for DBAL v4.0 ))

smsalert-mobi · 2023-09-03T10:39:30Z

Hello, what is the status of this PR ? when it will be released ?

Slamdunk · 2023-10-03T10:18:20Z

Ping 💯

Slamdunk · 2023-10-30T08:42:27Z

Merged in #110, thank you @mrVrAlex @e-vil-dev

Ocramius added the enhancement New feature or request label May 5, 2022

Ocramius assigned mrVrAlex May 5, 2022

Ocramius requested changes May 5, 2022

View reviewed changes

mrVrAlex requested a review from Ocramius May 5, 2022 22:02

Ocramius requested changes May 25, 2022

View reviewed changes

PowerKiKi mentioned this pull request Jan 2, 2023

Upgrade to doctrine/persistence v3 Ecodev/graphql-doctrine#67

Open

mrVrAlex changed the base branch from 3.6.x to 3.8.x January 11, 2023 10:24

mrVrAlex changed the base branch from 3.8.x to 3.9.x January 11, 2023 10:26

e-vil-dev force-pushed the support-persistence-3 branch 2 times, most recently from dccbbe3 to 61c69d6 Compare January 12, 2023 19:28

PowerKiKi mentioned this pull request Jan 13, 2023

Use mocks instead of creating new class from scratch opsway/psr-container-doctrine#2

Merged

mrVrAlex requested a review from Ocramius January 13, 2023 13:00

Slamdunk mentioned this pull request Jan 18, 2023

Add doctrine/annotations:v2, doctrine/event-manager:v2 and doctrine/persistence:v3 support #95

Merged

mrVrAlex changed the title ~~Support for doctrine persistence 3~~ Support for doctrine cache 2 Jan 31, 2023

mrVrAlex changed the base branch from 3.9.x to 3.10.x January 31, 2023 17:45

e-vil-dev force-pushed the support-persistence-3 branch from 633580f to 25c64cd Compare February 6, 2023 09:11

Ocramius requested changes Feb 6, 2023

View reviewed changes

Yevhen Vilkhovchenko and others added 2 commits February 7, 2023 14:49

Use mocks instead of creating new class from scratch

7d58ca0

e-vil-dev force-pushed the support-persistence-3 branch 2 times, most recently from 5f27c40 to 5d7bd85 Compare February 12, 2023 17:49

mrVrAlex requested a review from Ocramius February 13, 2023 13:57

e-vil-dev force-pushed the support-persistence-3 branch from 5d7bd85 to 46da607 Compare February 13, 2023 22:58

Slamdunk mentioned this pull request Oct 30, 2023

Add support for doctrine/cache:v2 #110

Merged

Slamdunk closed this Oct 30, 2023

Support for doctrine cache 2 #72

Support for doctrine cache 2 #72

Conversation

mrVrAlex commented May 4, 2022

Ocramius left a comment

Choose a reason for hiding this comment

mrVrAlex commented May 5, 2022

Ocramius commented May 5, 2022

Ocramius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PowerKiKi commented Jan 2, 2023

mrVrAlex commented Jan 2, 2023

PowerKiKi commented Jan 13, 2023

Slamdunk commented Jan 31, 2023

mrVrAlex commented Jan 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

internalsystemerror Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrVrAlex commented Mar 20, 2023

smsalert-mobi commented Sep 3, 2023

Slamdunk commented Oct 3, 2023

Slamdunk commented Oct 30, 2023

internalsystemerror Feb 9, 2023 •

edited

Loading