Deprecate using doctrine/cache in favour of PSR-6 #144

alcaeus · 2020-11-27T13:21:10Z

This PR deprecates the getCacheDriver and setCacheDriver methods in the abstract metadata factory in favour of setCache and getCache which work with PSR-6 cache instances.

To make the transition easier, this PR provides a full backward and forward compatibility layer. When using setCache to use a PSR-6 cache, getCacheDriver will expose a doctrine/cache instance that wraps the PSR-6 cache. Similarly, when using setCacheDriver with a doctrine/cache instance, getCache will expose a PSR-6 cache that wraps the original cache.

The new protected getCacheKey method is used to avoid hardcoding cache keys in tests and can also be used by extending implementations to work with cache items directly (although they shouldn't).

alcaeus

I've left some feedback below.

@greg0ire what settings do I need to pass to update the lock file? Looks like psalm does not like something about this change, but I assume it's due to composer.lock changes, not the cache change itself.

alcaeus · 2020-11-27T13:21:43Z

composer.json

-        "doctrine/event-manager": "^1.0"
+        "doctrine/event-manager": "^1.0",
+        "psr/cache": "^1.0",
+        "symfony/cache": "^4.4|^5.0"


This is used to provide the forward/compatibility layer. Once we drop that in 3.0, this becomes a dev dependency.

symfony/cache has a dev-dependency on doctrine/cache, so this means a circular dependency (not for us, but for them when they are in dev mode). I think this might be OK though since it's not meant to last, and since symfony seems to be resorting to COMPOSER_ROOT_VERSION in its CI already: https://github.com/symfony/symfony/search?q=COMPOSER_ROOT_VERSION

Still, we might want to check if we don't break their pipeline after releasing this (or merging it? I think they have builds that use untagged versions).

How is there a circular dependency? This is doctrine/persistence, not doctrine/cache.

How is there a circular dependency?

symfony/symfony depends on doctrine/persistence and replaces symfony/cache. Anyway, we should be good here. 😃

Right, nevermind my comment, not sure what I meant but I must have been confused.

alcaeus · 2020-11-27T13:23:27Z

lib/Doctrine/Persistence/Mapping/AbstractClassMetadataFactory.php

@@ -400,6 +445,11 @@ public function getReflectionService()
        return $this->reflectionService;
    }

+    protected function getCacheKey(string $realClassName): string
+    {
+        return str_replace('\\', '__', $realClassName) . $this->cacheSalt;


This (along with the changed cache salt) will invalidate existing caches. We'll have to check if existing implementations manually mess with cache keys, in which case we'll have to come back and check the impact of this change.

alcaeus · 2020-11-27T13:24:05Z

tests/Doctrine/Tests/Persistence/Mapping/ClassMetadataFactoryTest.php


-        $metadata = $this->createMock(ClassMetadata::class);
-        assert($metadata instanceof ClassMetadata);
+    public function testWillNotCacheFallbackMetadata(): void


I duplicated that test, as I realised that using the onMetadataNotFound fallback does not cache that metadata. Since I assume this is intended, I haven't made changes and instead opted to codify that behaviour in tests.

greg0ire · 2020-11-27T18:43:22Z

I guess you will need to fix the conflicts for a new build to be triggered and the SA to be fixed (I merged up 2.1.x into 2.2.x, it contains a fix for your issue.)

lib/Doctrine/Persistence/Mapping/AbstractClassMetadataFactory.php

greg0ire · 2020-11-28T19:00:08Z

My experience with doctrine/cache in paid projects is that it has bad framework integration, so I usually create a Symfony cache pool and wrap it in an adapter so that I can use it as a result cache for Doctrine and also see if the cache is used in the profiler. IMO this is the right move because it will mean we will not have to worry about things such as integrating doctrine/cache and a framework. Or maintaining that package. This is a great move IMO.

greg0ire · 2020-11-29T11:31:14Z

Not sure why there are this many PHPStan errors, but a lot of them look like they could be fixed by setting up phpstan/phpstan-phpunit.

alcaeus · 2020-11-29T11:33:51Z

Not sure why there are this many PHPStan errors, but a lot of them look like they could be fixed by setting up phpstan/phpstan-phpunit.

Not sure where those are coming from. The problem may be that I need to update the lock file due to dependency changes and it installs a newer version than it did before. We should probably document this in our contribution docs.

lib/Doctrine/Persistence/Mapping/AbstractClassMetadataFactory.php

tests/Doctrine/Tests/Persistence/Mapping/ClassMetadataFactoryTest.php

composer.json

alcaeus · 2021-02-10T08:38:24Z

@greg0ire any idea why psalm suddenly complains about AbstractClassMetadataFactory::getMetadataFor? Nothing really changed in between, so I suspect that this is a psalm-related issue 🤔

greg0ire · 2021-02-13T10:09:28Z

Woops sorry I'm only noticing your ping now, and I think you are right, see #156

greg0ire · 2021-02-13T10:18:10Z

Ah I need to merge up

Co-authored-by: Grégoire Paris <[email protected]>

Co-authored-by: Alexander M. Turek <[email protected]>

greg0ire · 2021-02-13T10:27:03Z

Thanks @alcaeus !

stof · 2021-03-31T15:55:15Z

lib/Doctrine/Persistence/Mapping/AbstractClassMetadataFactory.php

+    public function setCache(CacheItemPoolInterface $cache): void
+    {
+        $this->cache       = $cache;
+        $this->cacheDriver = new DoctrineProvider($cache);


this should use the doctrine/cache PSR-6 provider instead, which is available in the latest version

The DoctrineAdapter could be replaced. I've created doctrine/cache#366 to also include this class in doctrine/cache so we no longer have external dependencies for the forward and backward compatibility layers. I'll follow up with a PR here once this is done.

alcaeus added Improvement Deprecation labels Nov 27, 2020

alcaeus added this to the 2.2.0 milestone Nov 27, 2020

alcaeus requested review from beberlei, greg0ire and malarzm November 27, 2020 13:21

alcaeus force-pushed the support-psr-6 branch from 8e33555 to d062763 Compare November 27, 2020 13:22

alcaeus commented Nov 27, 2020

View reviewed changes

alcaeus mentioned this pull request Nov 27, 2020

Deprecate using doctrine/cache for metadata caching doctrine/mongodb-odm#2253

Merged

greg0ire closed this Nov 27, 2020

greg0ire reopened this Nov 27, 2020

greg0ire reviewed Nov 27, 2020

View reviewed changes

lib/Doctrine/Persistence/Mapping/AbstractClassMetadataFactory.php Show resolved Hide resolved

alcaeus requested a review from morozov November 29, 2020 10:28

alcaeus force-pushed the support-psr-6 branch 3 times, most recently from 4c21399 to 9b17158 Compare November 29, 2020 11:03

alcaeus requested a review from greg0ire November 29, 2020 11:03

malarzm approved these changes Nov 29, 2020

View reviewed changes

lib/Doctrine/Persistence/Mapping/AbstractClassMetadataFactory.php Outdated Show resolved Hide resolved

lib/Doctrine/Persistence/Mapping/AbstractClassMetadataFactory.php Show resolved Hide resolved

nicolas-grekas reviewed Nov 29, 2020

View reviewed changes

lib/Doctrine/Persistence/Mapping/AbstractClassMetadataFactory.php Show resolved Hide resolved

nicolas-grekas reviewed Nov 29, 2020

View reviewed changes

lib/Doctrine/Persistence/Mapping/AbstractClassMetadataFactory.php Outdated Show resolved Hide resolved

alcaeus force-pushed the support-psr-6 branch 2 times, most recently from ee2df30 to c2e4044 Compare December 12, 2020 12:06

derrabus approved these changes Dec 12, 2020

View reviewed changes

greg0ire reviewed Dec 12, 2020

View reviewed changes

alcaeus requested a review from greg0ire December 12, 2020 14:21

greg0ire approved these changes Dec 12, 2020

View reviewed changes

derrabus reviewed Feb 5, 2021

View reviewed changes

composer.json Outdated Show resolved Hide resolved

franmomu mentioned this pull request Feb 12, 2021

Fix psalm issue #156

Merged

greg0ire closed this Feb 13, 2021

greg0ire reopened this Feb 13, 2021

alcaeus and others added 4 commits February 13, 2021 11:21

Deprecate using doctrine/cache in favour of PSR-6

23869a2

Optimise storing metadata in cache

9d16e4b

Remove unnecessary invocation count matchers

86f51b2

Co-authored-by: Grégoire Paris <[email protected]>

Accept all major versions of psr/cache

3ee8ca7

Co-authored-by: Alexander M. Turek <[email protected]>

greg0ire force-pushed the support-psr-6 branch from ac52449 to 3ee8ca7 Compare February 13, 2021 10:22

greg0ire merged commit cdfca25 into doctrine:2.2.x Feb 13, 2021

alcaeus deleted the support-psr-6 branch February 15, 2021 15:13

stof reviewed Mar 31, 2021

View reviewed changes

greg0ire mentioned this pull request Apr 29, 2021

Introduce PSR-6 for metadata caching doctrine/orm#8651

Merged

github-actions bot mentioned this pull request May 15, 2021

Merge release 2.2.0 into 3.0.x #178

Merged

greg0ire mentioned this pull request May 16, 2021

Cannot convert cache to PSR-6 cache #179

Closed

This was referenced Dec 6, 2021

Use our cache pool to store configurations doctrine-extensions/DoctrineExtensions#2365

Closed

Remove unused cache doctrine-extensions/DoctrineExtensions#2370

Closed

franmomu mentioned this pull request Jan 13, 2022

Closure: mirror Doctrine cache key doctrine-extensions/DoctrineExtensions#2416

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate using doctrine/cache in favour of PSR-6 #144

Deprecate using doctrine/cache in favour of PSR-6 #144

alcaeus commented Nov 27, 2020

alcaeus left a comment

alcaeus Nov 27, 2020

greg0ire Dec 12, 2020 •

edited

Loading

alcaeus Dec 12, 2020

derrabus Dec 12, 2020

greg0ire Feb 13, 2021

alcaeus Nov 27, 2020

alcaeus Nov 27, 2020

greg0ire commented Nov 27, 2020 •

edited

Loading

greg0ire commented Nov 28, 2020 •

edited

Loading

greg0ire commented Nov 29, 2020

alcaeus commented Nov 29, 2020

alcaeus commented Feb 10, 2021

greg0ire commented Feb 13, 2021

greg0ire commented Feb 13, 2021

greg0ire commented Feb 13, 2021

stof Mar 31, 2021

alcaeus Apr 13, 2021

Deprecate using doctrine/cache in favour of PSR-6 #144

Deprecate using doctrine/cache in favour of PSR-6 #144

Conversation

alcaeus commented Nov 27, 2020

alcaeus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire Dec 12, 2020 • edited Loading

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

greg0ire commented Nov 27, 2020 • edited Loading

greg0ire commented Nov 28, 2020 • edited Loading

greg0ire commented Nov 29, 2020

alcaeus commented Nov 29, 2020

alcaeus commented Feb 10, 2021

greg0ire commented Feb 13, 2021

greg0ire commented Feb 13, 2021

greg0ire commented Feb 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire Dec 12, 2020 •

edited

Loading

greg0ire commented Nov 27, 2020 •

edited

Loading

greg0ire commented Nov 28, 2020 •

edited

Loading