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] Fixed issue where Cache::forget() works differently if keys created with Cache::flexible when using Redis, DynamoDB, Memcached or Apc #52936

Closed
wants to merge 1 commit into from

Conversation

Tklaversma
Copy link

See #52891.

Currently when calling Cache::forget() on any store, it will delete the cache item. Since the introduction of Cache::flexible() two items are created, the cache item itself and a "created" key.

For example:

// This will create two keys, 'foo' and 'illuminate:cache:flexible:created:foo'
Cache::flexible('foo', 100, fn() => 'bar'); 

When using the store file or database, both cache items will be deleted. But when using Redis, the 'illuminate:cache:flexible:created:foo will not be deleted.

I've created this PR to address this issue and making sure all stores work the same way, deleting all created keys when Cache::forget() is being called.

Copy link

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

@Tklaversma Tklaversma marked this pull request as ready for review September 26, 2024 07:21
@timacdonald
Copy link
Member

As mentioned in the original PR conversation, we intentionally only targeted the drivers where the keys would remain, i.e., file and database.

This PR will introduce another network call for all cache drivers, which I feel is more detrimental to an application's performance than leaving the keys to expire, like we do with cache tags.

We have discussed this internally and I still feel we should not take this specific approach without first improving the cache drivers to better support it.

@Tklaversma
Copy link
Author

Tklaversma commented Sep 27, 2024

@timacdonald I understand your issue completely and is totally valid. But, the fact remains, different cache stores do different things now. Having the option to make them work equally, sounds to me like a temporary fix for now, rather than doing nothing until we have better support.

I'm not saying my PR is the way, but I come back to my first point of having a second parameter on forget, that does delete this ":created" key. This way, everything stays as it is right now, but people that do want to tackle this issue can now use it.

The current alternative for now, is calling forget twice everywhere when used...

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.

4 participants