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

fix cache and generate short cache key #971

Merged
merged 6 commits into from
Dec 8, 2024

Conversation

ilaoniu
Copy link
Contributor

@ilaoniu ilaoniu commented Dec 1, 2024

Fix cache

The $cacheItem can only be null or boolean value, so $cacheItem->get() method call will throw an exception and be caught by the outer try catch code block, the cache will not work normally.

Shorter cache key

When use database as cache driver, the cache key is too long(500 chars), with md5 it can be limited to 50 chars.

Cache TTL

When use Redis or Memcached as cache driver, the cache can automatically expire, otherwise the memory usage will continue to increase.

@serbanghita serbanghita self-assigned this Dec 5, 2024
@serbanghita
Copy link
Owner

The $cacheItem can only be null or boolean value

$cacheItem can only be CacheItem|null because $cacheKey is always a base64 string.
I don't understand what do you mean.

so $cacheItem->get() method call will throw an exception and be caught by the outer try catch code block, the cache will not work normally.

Indeed get() and set() can throw Unhandled \Psr\SimpleCache\InvalidArgumentException.
I think I need to handle and ignore the exception IF hypothetically the cache key is invalid.

When use database as cache driver, the cache key is too long(500 chars), with md5 it can be limited to 50 chars.

this is a good point, I guess I made it base64 because I wanted the debug aspect of it. I'd make this configurable, def I'd not go for md5 because of the collisions, but a sha1

When use Redis or Memcached as cache driver, the cache can automatically expire, otherwise the memory usage will continue to increase.

Agree on the cache TTL, good catch!

@serbanghita
Copy link
Owner

serbanghita commented Dec 5, 2024

What I can do is:

  1. add TTL config
  2. make the encoding of the cache key configurable (so I can keep backwards compat)
  3. address Unhandled \Psr\SimpleCache\InvalidArgumentException. I guess nobody wants their detection script to break because of faulty cache

I'll fork your PR and make these changes

@serbanghita
Copy link
Owner

@ilaoniu tests are failing

@ilaoniu
Copy link
Contributor Author

ilaoniu commented Dec 6, 2024

Oh, you are right. There is no problem with calling $cacheItem->get().

@serbanghita serbanghita merged commit 177f891 into serbanghita:4.8.x Dec 8, 2024
5 checks passed
@serbanghita
Copy link
Owner

@ilaoniu thank you!
I've merged it

I've added tests and made the config option cacheKeyFn, by default it's base64

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.

2 participants