[Cache] Handle unserialize() failures gracefully #19567
Conversation
nicolas-grekas
force-pushed
the
cache-fix-throwing-generator
branch
from
2915a08
to
1e151a6
Compare
nicolas-grekas
force-pushed
the
cache-fix-throwing-generator
branch
7 times, most recently
from
b18a56f
to
448de93
Compare
| $isHit = false; | ||
| } | ||
| } catch (\Exception $e) { | ||
| CacheItem::log($this->logger, 'Failed to unserialize key "{key}"', array('key' => $key, 'exception' => $e)); |
There was a problem hiding this comment.
It should not happen as this adapter is not cross requests (so something not unserializable won't be cached as it is not serializable as well).
If you want to keep this anyway then i guess you should use parent::unserialize below ?
There was a problem hiding this comment.
Ok got it it might break in case of a custom unserializer.
At least using parent::unserialize would allow you to remove the case managing false.
There was a problem hiding this comment.
There is no parent here, and calling the same implementation would call ini_set twice for something that can't happen because as you said, this is not cross reqs
There was a problem hiding this comment.
You're right sorry, i should definitely stop reviewing on my phone...
nicolas-grekas
force-pushed
the
cache-fix-throwing-generator
branch
from
448de93
to
5a8dbbe
Compare
| } | ||
| } catch (\Exception $e) { |
There was a problem hiding this comment.
Shouldn't this be moved inside the foreach to fetch as many values as possible ?
There was a problem hiding this comment.
right, done: try/catch moved inside the foreach
|
LGTM 👍 |
nicolas-grekas
force-pushed
the
cache-fix-throwing-generator
branch
2 times, most recently
from
20184d0
to
69cfa83
Compare
nicolas-grekas
force-pushed
the
cache-fix-throwing-generator
branch
from
69cfa83
to
47db638
Compare
|
Thank you @nicolas-grekas. |
…grekas) This PR was merged into the 3.1 branch. Discussion ---------- [Cache] Handle unserialize() failures gracefully | Q | A | ------------- | --- | Branch? | 3.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This makes fetching cached items more resilient: `__PHP_Incomplete_Class` "objects" and other errors triggered by unserialize should be turned to cache misses. Commits ------- 47db638 [Cache] Handle unserialize() failures gracefully
This makes fetching cached items more resilient:
__PHP_Incomplete_Class"objects" and other errors triggered by unserialize should be turned to cache misses.