-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Cache] Propagate expiry when syncing items in ChainAdapter #34787
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.
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
[Cache] Propagate expiry when syncing items in ChainAdapter #34787
Conversation
$item->isHit = $sourceItem->isHit; | ||
$item->metadata = $sourceItem->metadata; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have preferred to keep reading from $sourceItem->metadata here, but this is cleared by
unset($metadata[CacheItem::METADATA_EXPIRY], $metadata[CacheItem::METADATA_CTIME]); |
CacheInterface
so have provided the metadata separately to work around this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is cleared only if the expiry is over, so not sure about this argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That didn't appear to be the case in my testing. The condition makes sense when setting metadata based on the item properties set by the cache interface build callback, however in the case of the chain adapter the wrapper callback will be returning an item fetched from the cache which does not have the expiry property set (see #34487). Thus the metadata is cleared.
I'm currently seeing test failures on the travis build for PHP 7.3. I'm not sure why this is as PHP 7.2 is fine and the test pass for me when run locally against 7.3.12. |
The penny has just dropped. The failures were a result of the use of the lowest possible dependencies for the 7.3 build for 4.2. Presumably the cache contracts. Of course, this is all moot as the real problem here is that I was incorrectly targeting 4.2 rather than 4.3. I've re-targeted this PR accordingly and all tests are now OK. |
If a lower adapter provides item metadata, propagate the expiry time when syncing the item to upper ones.
Good catch, thanks @trvrnrth. |
…r (trvrnrth) This PR was merged into the 4.3 branch. Discussion ---------- [Cache] Propagate expiry when syncing items in ChainAdapter | Q | A | ------------- | --- | Branch? | 4.3 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | n/a | License | MIT | Doc PR | n/a If a lower adapter provides item metadata, propagate the expiry time when syncing the item to upper ones. Commits ------- 77138ac [Cache] Propagate expiry when syncing items in ChainAdapter
If a lower adapter provides item metadata, propagate the expiry time when syncing the item to upper ones.