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

[11.x] Add remaining method to get the expiration timestamp of a cache key #52017

Draft
wants to merge 25 commits into
base: 11.x
Choose a base branch
from

Conversation

Markshall
Copy link
Contributor

Synopsis

I'm running a large(ish) query and caching the data for a set period of time, and wanting to display to the user how long is remaining until the data is refreshed in the cache. To test this, I manually modified the FileStore and DatabaseStore classes within my project's vendor/ directory to add a method that will return the expiry time as a human readable string (or optionally the UNIX timestamp).

This is only a draft PR because protocols such as Memcached do not provide an efficient API to retrieve the expiry time of a cached item, unless you store the expiry time alongside the actual data being cached, but that would likely be a breaking change within the Laravel codebase if this was to happen. I have also not provided methods for other stores such as RedisStore or DynamoDbStore etc. as I was quickly testing solutions before drafting this PR.

I am open to seeing suggestions as to what should happen if this were to be a feature within the Cache system in Laravel. For example, as mentioned, protocols such as Memcached do not provide a way to retrieve the expiry time, so should the method exist, but return null? Or should it not exist at all?

Creating the method, but returning null, would mean you don't get an error when calling via the cache helper function:

cache()->remaining('my_key');

Not having the method would of course mean you receive an error.

Proposed usage

Get the expiry time as a human readable string

$expiry = cache()->remaining('my_key'); // "7 minutes from now"

Get the UNIX timestamp

$expiry = cache()->remaining('my_key', true); // 1720099976

@Markshall Markshall marked this pull request as draft July 4, 2024 14:13
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might need to leave out of this PR all changes made to the Contract, and add those to a different PR targeting master instead of 11.x since any changes made to a Contract are considered breaking changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have restored the Contract to its original state to reduce overcomplication. I originally just wanted this to be a change to FileStore for testing purposes and suggestions/discussion and got a little carried away, haha.

@henzeb
Copy link
Contributor

henzeb commented Jul 4, 2024

What is wrong with two cache keys, one containing the data, the other the ttl?

@antonkomarev
Copy link
Contributor

antonkomarev commented Jul 4, 2024

Maybe it's better to return \DateIntrval object? We will be able to convert it to what format we want

@Markshall
Copy link
Contributor Author

What is wrong with two cache keys, one containing the data, the other the ttl?

I like it. But I feel like this is only necessary on protocols such as Memcached and Redis. Storing 2 keys for FileStore and DatabaseStore seems unnecessary to me as we can easily retrieve the expiration with these protocols for example, and would also make the expiration column somewhat redundant in the DatabaseStore cache table.

Retrieving an item from the cache could also have an optional parameter which enables the developer to also retrieve the remaining TTL for the given item in an array, such as:

[
  'data' => ...,
  'ttl' => 1720099976
]

So we could have 2 options to get the remaining TTL:

[$data, $ttl] = cache('my_data', ttl: true);
$ttl = cache()->remaining('my_data');

This is just quick drafting off the top of my head, so will test some implementations.

@Markshall Markshall changed the title Add remaining method to get the expiration timestamp of a cache key [11.x] Add remaining method to get the expiration timestamp of a cache key Jul 9, 2024
@driesvints
Copy link
Member

@Markshall please rebase this PR and mark it as ready if you need a new review.

@henzeb
Copy link
Contributor

henzeb commented Oct 1, 2024

Can we please fix tests on the local machine before pushing it?

@Markshall
Copy link
Contributor Author

Markshall commented Oct 2, 2024

Can we please fix tests on the local machine before pushing it?

Was having trouble replicating the tests on my local machine. I don't have memcached PHP extension installed so it's skipping the tests and trying to install it is proving impossible. Windows is saying the extension isn't found in my ext dir even though it clearly is there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants