fix: 🐛 Self-heal cache when corrupted non-Collection value is detected.#207
Merged
Conversation
|
Hi @mikebronner! Do I read it properly that you Laravel 13 says it's not recommended and if used, the class must be added to exceptions: If this is indeed the case, maybe it would be good not to serialize the whole classes, but just to save the cached data as json/array? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #206 —
removeEmptyCacheEntry()throwsTypeErrorwhen a cached entry can't be unserialized cleanly.Why this was happening
Cached entries are stored as
["key" => $cacheKey, "value" => Collection]. On read, the package trusted Laravel's cache to return that shape back intact and passed$result["value"]straight toremoveEmptyCacheEntry(), which had a strictCollectiontype hint. When the cachedvaluecame back as anything else — most commonly__PHP_Incomplete_Class— the type assertion failed and the request blew up.PHP returns
__PHP_Incomplete_Classfromunserialize()when the serialized blob references a class it can't reify at read time. There are several realistic triggers:MySQL column truncation on the
cachetable. Laravel'scache:tablemigration usesmediumText(~16 MB), but older or hand-rolled tables may usetext(~64 KB). A Geocoder Collection holding multi-result Address objects (admin levels, bounds, coords, country, locality, etc.) can exceed 64 KB, particularly for chained providers. MySQL silently truncates the row, the blob comes back malformed, and unserialize produces__PHP_Incomplete_Classfor partially-reconstructed items. This is consistent with the reporter's symptoms in Started receiving errors fromremoveEmptyCacheEntry()#206: worked for several days post-upgrade (until a large enough result was cached), surfaced "randomly" when cache cleaning ran (expired big entries → next read of a refilled-and-truncated row trips it), and disappeared on Redis (no column-size constraint).Class-definition drift across upgrades. Old serialized blobs reference class shapes (private property sets, namespaces) that no longer match current definitions. Unserialize falls back to
__PHP_Incomplete_Class. Switching cache stores looks like a fix because the new store starts empty — nothing from before the upgrade is in there.Partial/concurrent writes in the database driver. Theoretically possible, much less likely than the above.
In all three cases the package's job isn't to prevent the corruption — that's outside our control — it's to recover gracefully.
What this PR changes
src/ProviderAndDumperAggregator.php:preventCacheKeyHashCollision()now validates the full shape of the cached entry: must be an array withkeyandvaluekeys, key must match the expected cache key, ANDvaluemust be aCollection. Any validation failure routes through the existing recovery path:forget()the bad entry and re-fetch. The parameter type is relaxed fromarraytomixedso a non-array cache return doesn'tTypeErrorbefore validation can run.removeEmptyCacheEntry()parameter type is relaxed fromCollectiontomixedas belt-and-suspenders, with an explicitinstanceof Collectionearly-return. The upstream validation should mean this is never reached with a non-Collection, but a future code path that misses the validation can't reintroduce the originalTypeError.Effect for users
php artisan cache:clearrequired.Test coverage
New Pest test in
tests/Feature/Providers/GeocoderServiceTest.php:it self-heals when a cached entry has a non-Collection value— pre-poke an entry whosevalueis a string instead of a Collection (simulating truncation / unserialize drift /__PHP_Incomplete_Class), callgeocode(), assert no exception is thrown, a realCollectionis returned, and the cache entry is replaced with a healthy one.Without the fix this test reproduces issue #206 verbatim (same exception class, same parameter, same call site). All 30 existing tests continue to pass.