Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Metadata caching isn't compatible with prefixed cache keys #338

Open
wants to merge 7 commits into
base: 3.3.x
Choose a base branch
from
Open

Metadata caching isn't compatible with prefixed cache keys #338

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
afa61c8
fix: Refactor metadata generation and cache saving to be compatible w…
adambalint-srg Sep 27, 2023
f8c7ef1
fix: Stylecheck fixes
adambalint-srg Sep 28, 2023
46d44c5
Merge branch 'doctrine:3.3.x' into dev-prefix-cache-fix
adambalint-srg Jun 13, 2024
afd02aa
test: Write a test for prefixed cache key support
adambalint-srg Jun 13, 2024
c328f3c
fix: Fix test on PHP7.*
adambalint-srg Jun 14, 2024
2e24143
fix: Simpler solution for saving metadata to cache with prefixed key …
adambalint-srg Jun 21, 2024
247d327
Merge branch '3.3.x' into dev-prefix-cache-fix
adambalint-srg Feb 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions src/Persistence/Mapping/AbstractClassMetadataFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,15 @@
use Psr\Cache\CacheItemPoolInterface;
use ReflectionClass;
use ReflectionException;
use Traversable;

use function array_combine;
use function array_keys;
use function array_map;
use function array_reverse;
use function array_unshift;
use function assert;
use function class_exists;
use function iterator_to_array;
use function ltrim;
use function str_replace;
use function strpos;
Expand Down Expand Up @@ -205,17 +206,16 @@ public function getMetadataFor(string $className)
$this->wakeupReflection($cached, $this->getReflectionService());
} else {
$loadedMetadata = $this->loadMetadata($realClassName);
$classNames = array_combine(
array_map([$this, 'getCacheKey'], $loadedMetadata),
$loadedMetadata
);
$cacheItems = $this->cache->getItems(array_map([$this, 'getCacheKey'], $loadedMetadata));

foreach ($this->cache->getItems(array_keys($classNames)) as $item) {
if (! isset($classNames[$item->getKey()])) {
continue;
}
if ($cacheItems instanceof Traversable) {
$cacheItems = iterator_to_array($cacheItems);
}

$loadedMetadataItems = array_combine($loadedMetadata, $cacheItems);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this logic assumes that $cacheItems is in the same order than $loadedMetadataItems`. However, I don't think PSR-6 makes any guarantee about the order of items in the returned result.
The CacheItemPoolInterface phpdoc only says that the returned iterable is keyed by cache keys.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. This led me to an even simpler solution for this code part. Check my newer commit. It iterates over loaded metadata and get item for each, but I think it's not too frequent that $loadedMetadata's count is more than one, so it's maybe not a performance issue.


$item->set($this->loadedMetadata[$classNames[$item->getKey()]]);
foreach ($loadedMetadataItems as $loadedClassName => $item) {
$item->set($this->loadedMetadata[$loadedClassName]);
$this->cache->saveDeferred($item);
}

Expand Down
29 changes: 29 additions & 0 deletions tests/Persistence/Mapping/AbstractClassMetadataFactoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
use Doctrine\Persistence\Mapping\Driver\MappingDriver;
use Doctrine\Persistence\Mapping\MappingException;
use Doctrine\Tests\DoctrineTestCase;
use Psr\Cache\CacheItemInterface;
use Psr\Cache\CacheItemPoolInterface;

use function get_class;

Expand Down Expand Up @@ -92,6 +94,33 @@ public function testItGetsTheSameMetadataForBackslashedClassName(): void
/** @psalm-suppress ArgumentTypeCoercion */
self::assertSame($cmf->getMetadataFor(SomeOtherEntity::class), $cmf->getMetadataFor('\\' . SomeOtherEntity::class));
}

public function testCacheStoredWithPrefixedKeys(): void
{
$cmf = $this->getMockForAbstractClass(AbstractClassMetadataFactory::class);
$cmf
->method('newClassMetadataInstance')
->with(SomeOtherEntity::class)
->willReturn(
$this->createStub(ClassMetadata::class)
);

$cache = $this->createMock(CacheItemPoolInterface::class);
$cmf->setCache($cache);

$cache->method('getItem')->willReturn($this->createConfiguredMock(CacheItemInterface::class, ['get' => null]));

$cacheItem = $this->createMock(CacheItemInterface::class);
$cacheItem->method('getKey')->willReturn('prefix__Doctrine__Tests__Persistence__Mapping__SomeOtherEntity__CLASSMETADATA__'); //Cache item's key is prefixed
$cache->method('getItems')
->with(['Doctrine__Tests__Persistence__Mapping__SomeOtherEntity__CLASSMETADATA__']) //Key which is generated from class name is not prefixed
->willReturn([$cacheItem]);

$cacheItem->expects(self::once())->method('set');
$cache->expects(self::once())->method('saveDeferred');

$cmf->getMetadataFor(SomeOtherEntity::class);
}
}

class SomeGrandParentEntity
Expand Down