Skip to content
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

Merged
merged 1 commit into from Dec 10, 2019
Merged

[Cache] Propagate expiry when syncing items in ChainAdapter #34787

merged 1 commit into from Dec 10, 2019

Conversation

trvrnrth
Copy link

@trvrnrth trvrnrth commented Dec 3, 2019

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.

$item->isHit = $sourceItem->isHit;
$item->metadata = $sourceItem->metadata;
Copy link
Author

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]);
when the adapter is a CacheInterface so have provided the metadata separately to work around this.

Copy link
Member

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

Copy link
Author

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.

@trvrnrth
Copy link
Author

trvrnrth commented Dec 4, 2019

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.

@trvrnrth trvrnrth changed the base branch from 4.2 to 4.3 December 9, 2019 15:24
@trvrnrth
Copy link
Author

trvrnrth commented Dec 9, 2019

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.
@nicolas-grekas nicolas-grekas changed the title [Cache] [ChainAdapter] When syncing items propagate expiry [Cache] Propagate expiry when syncing items in ChainAdapter Dec 10, 2019
@nicolas-grekas
Copy link
Member

Good catch, thanks @trvrnrth.

nicolas-grekas added a commit that referenced this pull request Dec 10, 2019
…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
@nicolas-grekas nicolas-grekas merged commit 77138ac into symfony:4.3 Dec 10, 2019
This was referenced Dec 19, 2019
@fabpot fabpot mentioned this pull request Jan 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants