@carsonbot could you find me a reviewer please?

I'm sorry. I could not find any suitable reviewer.

Oh. I guess it was too long ago@dmaicher wrote this class =)

David, do you mind having a look at this PR?

Thanks for the quick fix :)

If I understand correctly though, this means using lambdas like that will prevent caching of the resolved config and it'll reload on every request needing it. I guess it's thus worth avoiding lambda callbacks..

Yes, that is correct.

I updated the docs with this observation:symfony/symfony-docs#15172

Storing misses is an important part of the performance provided by system caches.
We should look for another solution if possible.

Yes, but these "misses" are interpreted as "hits". A "hit" with null is not the same as a miss. There should never be "null" stored inthis cache.

I have two alternative solutions. The first would be to find a way to serialize a\Closure. The second one is to throw an exception on cache warmup when you fail to save a value in cache.

The test case passes even without the change if I didn't mess up :)

Yes, but these "misses" are interpreted as "hits". A "hit" with null is not the same as a miss. There should never be "null" stored in this cache.

For system caches, aka the ones sourced by the code itself, we can do this - cache misses.
We built this logic because it saves CPU cycles, eg you don't want to read the annotations again on methods that don't have any annotation.

I have two alternative solutions. The first would be to find a way to serialize a \Closure. The second one is to throw an exception on cache warmup when you fail to save a value in cache.

Why a closure ends up in the cache? That's the main question I have here indeed. An annotation should not lead to a closure because then of course it cannot be cached - while the source is a plain descriptive annotation.

The second solution would be a major regression actually.

Can you try this patch instead? It tries to not store misses for failures-to-save().

--- a/src/Symfony/Component/Cache/Traits/ArrayTrait.php+++ b/src/Symfony/Component/Cache/Traits/ArrayTrait.php@@ -145,6 +145,7 @@ trait ArrayTrait             try {                 $serialized = serialize($value);             } catch (\Exception $e) {+                unset($this->values[$key]);                 $type = \is_object($value) ? \get_class($value) : \gettype($value);                 $message = sprintf('Failed to save key "{key}" of type %s: ', $type).$e->getMessage();                 CacheItem::log($this->logger, $message, ['key' => $key, 'exception' => $e]);

The test case passes even without the change if I didn't mess up :)

You did mess up =)

❯ ./phpunit src/Symfony/Bundle/FrameworkBundle/Tests/CacheWarmer #!/usr/bin/env phpPHPUnit 9.5.3 by Sebastian Bergmann and contributors.Testing /src/Symfony/Bundle/FrameworkBundle/Tests/CacheWarmer....F..............                                               19 / 19 (100%)Time: 00:00.091, Memory: 16.00 MBThere was 1 failure:1) Symfony\Bundle\FrameworkBundle\Tests\CacheWarmer\AnnotationsCacheWarmerTest::testWarmupRemoveCacheMissesFailed asserting that actual size 3 matches expected size 2./src/Symfony/Bundle/FrameworkBundle/Tests/CacheWarmer/AnnotationsCacheWarmerTest.php:149

Why a closure ends up in the cache?

Because we cache an object ofSymfony\Component\Validator\Constraint\Callback. That object is given a closure by the user in its constructor.

Can you try this patch instead? It tries to not store misses for failures-to-save().

I think that would work. But the end result would be the same. We would reread the annotations at runtime.
Note that we still cache the empty array. Which means "we have read the annotations, and there weren't any".

You did mess up =)

I confirm :P

Because we cache an object of Symfony\Component\Validator\Constraint\Callback. That object is given a closure by the user in its constructor.

I just read the doc PR, nice one.

I think that would work. But the end result would be the same. We would reread the annotations at runtime.

True, but we would still store misses, and this is important for the other system caches.

Note that we still cache the empty array. Which means "we have read the annotations, and there weren't any".

OK great, then I suppose other system caches need this. Annotations are not the only system caches, but the current patch changes the behavior for all of them.

I was wrong patchingAbstractPhpFileCacheWarmer. What I really wanted to do is to only modify the behaviour of theAnnotationCacheWarmer.

Can you try this patch instead? It tries to not store misses for failures-to-save().

This is not reliable. If a user does something like this, the bug will occur again.

$i =$cache->getItem('foo');$i->set('data');$cache->save($i);// Save failed for some reason.// ...$cache->getItem('foo');

The PR is updated

Oh. I guess it was too long ago@dmaicher wrote this class =)

David, do you mind having a look at this PR?

I think my work on that was related to#23558

But now I see you changed the PR anyway 😊

Thank you@Nyholm.