fix: correctly fetch file system cache entries

Method `\CuyZ\Valinor\Cache\FileSystemCache::get()` was not properly
looping on all delegates, leading to the values not being fetched from
the cache files and resulting in `null` (the default value) being
returned in some cases. Because of the following algorithm, the cache
entry was populated again, so the cache was not really working here.

```php
if ($this->cache->has($key)) {
    $entry = $this->cache->get($key);

    if ($entry) {
        return $entry;
    }
}

$class = $this->delegate->for($type);

$this->cache->set($key, $class);

return $class;
```
This commit is contained in:
Romain Canon 2022-09-01 12:21:41 +02:00
parent 11a7ea7252
commit 48208c1ed1
3 changed files with 61 additions and 19 deletions

View File

@ -15,6 +15,7 @@ use FilesystemIterator;
use Psr\SimpleCache\CacheInterface;
use Traversable;
use function assert;
use function file_exists;
use function file_put_contents;
use function is_dir;
@ -74,6 +75,10 @@ final class CompiledPhpFileCache implements CacheInterface
public function set($key, $value, $ttl = null): bool
{
$filename = $this->path($key);
assert(! file_exists($filename));
$code = $this->compile($value, $ttl);
$tmpDir = $this->cacheDir . DIRECTORY_SEPARATOR . '.valinor.tmp';
@ -84,7 +89,6 @@ final class CompiledPhpFileCache implements CacheInterface
/** @infection-ignore-all */
$tmpFilename = $tmpDir . DIRECTORY_SEPARATOR . uniqid('', true);
$filename = $this->path($key);
try {
if (! @file_put_contents($tmpFilename, $code)) {

View File

@ -13,10 +13,8 @@ use CuyZ\Valinor\Definition\Repository\Cache\Compiler\FunctionDefinitionCompiler
use Psr\SimpleCache\CacheInterface;
use Traversable;
use function current;
use function get_class;
use function is_object;
use function next;
use function sys_get_temp_dir;
/**
@ -55,13 +53,10 @@ final class FileSystemCache implements CacheInterface
public function get($key, $default = null)
{
while ($delegate = current($this->delegates)) {
foreach ($this->delegates as $delegate) {
if ($delegate->has($key)) {
return $delegate->get($key, $default);
}
// @infection-ignore-all
next($this->delegates);
}
return $default;

View File

@ -6,34 +6,77 @@ namespace CuyZ\Valinor\Tests\Integration\Cache;
use CuyZ\Valinor\Cache\FileSystemCache;
use CuyZ\Valinor\Cache\FileWatchingCache;
use CuyZ\Valinor\Mapper\TreeMapper;
use CuyZ\Valinor\MapperBuilder;
use CuyZ\Valinor\Tests\Integration\IntegrationTest;
use CuyZ\Valinor\Tests\Integration\Mapping\Fixture\SimpleObject;
use CuyZ\Valinor\Utility\Polyfill;
use org\bovigo\vfs\vfsStream;
use org\bovigo\vfs\vfsStreamDirectory;
use org\bovigo\vfs\vfsStreamFile;
use Psr\SimpleCache\CacheInterface;
use stdClass;
use function strtoupper;
use function file_get_contents;
final class CacheInjectionTest extends IntegrationTest
{
public function test_cache_entries_are_written_during_mapping(): void
public function test_cache_entries_are_written_once_during_mapping(): void
{
$files = vfsStream::setup('cache-dir');
$cacheDirectory = vfsStream::setup('cache-dir');
$cache = new FileSystemCache($files->url());
$cache = new FileSystemCache($cacheDirectory->url());
$cache = new FileWatchingCache($cache);
self::assertFalse($files->hasChildren());
// Calling the mapper a first time to populate the cache entries…
$this->createMapper($cache)->map(SimpleObject::class, 'foo');
$object = (new MapperBuilder())
$files = $this->recursivelyFindPhpFiles($cacheDirectory);
self::assertCount(6, $files);
foreach ($files as $file) {
$file->setContent($file->getContent() . "\n// generated value 1661895014");
}
// Calling the mapper a second time: checking that the cache entries
// have not been overridden.
$this->createMapper($cache)->map(SimpleObject::class, 'foo');
foreach ($files as $file) {
self::assertStringContainsString('// generated value 1661895014', file_get_contents($file->url()) ?: '');
}
}
/**
* @param CacheInterface<mixed> $cache
*/
private function createMapper(CacheInterface $cache): TreeMapper
{
return (new MapperBuilder())
->withCache($cache)
// The cache should be able to cache function definitions…
->alter(fn (string $value): string => strtoupper($value))
->mapper()
// …as well as class definitions.
->map(SimpleObject::class, 'foo');
->registerConstructor(fn (): stdClass => new stdClass())
->mapper();
}
self::assertSame('FOO', $object->value);
/**
* @return vfsStreamFile[]
*/
private function recursivelyFindPhpFiles(vfsStreamDirectory $directory): array
{
$files = [];
self::assertTrue($files->hasChildren());
foreach ($directory->getChildren() as $child) {
if ($child instanceof vfsStreamFile && Polyfill::str_ends_with($child->getName(), '.php')) {
$files[] = $child;
}
if ($child instanceof vfsStreamDirectory) {
$files = [...$files, ...$this->recursivelyFindPhpFiles($child)];
}
}
return $files;
}
}