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] Changed getCacheKey to use the method name instead of hardcoding to get #94

Merged
merged 1 commit into from
Sep 29, 2021
Merged

[fix] Changed getCacheKey to use the method name instead of hardcoding to get #94

merged 1 commit into from
Sep 29, 2021

Conversation

streamingsystems
Copy link

Hi,

I implemented my own "exists" method using the examples you outline here:

https://leqc.renoki.org/advanced/implement-caching/implementing-cache-for-other-functions-than-get

My code:

public function exists() { if (! $this->shouldAvoidCache()) { return $this->getFromQueryCache('exists'); } return parent::exists(); }

However, the getFromQueryCache ignores the $method when creating the cache key and always uses "get".

What happened was I ran the exists() first, which cached the query in Redis as a bool.

Then, when I ran the get() it found the cache entry and returned that and then Laravel blew up as it was expecting a collection , but it got a bool.

I changed the code so that the key will use the $method variable and it solved my problem. Now exists and get are different entries in the cache.

I would think this would be a widespread problem for anyone trying to implement methods (other than the get) as they would always be sharing the same "get" key in the cache. Of course, if the method they were using ultimately retrieved the "get" data from the cache it would be fine.

However, with exists it's just storing the result (boolean) in the cache which is problematic for subsequent calls with the same key that expect a collection (or some result other than a boolean).

Thanks!

-Rob

@codecov
Copy link

codecov bot commented Sep 29, 2021

Codecov Report

Merging #94 (b25dd4a) into master (3168fb2) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #94   +/-   ##
=======================================
  Coverage   79.34%   79.34%           
=======================================
  Files           4        4           
  Lines         184      184           
=======================================
  Hits          146      146           
  Misses         38       38           
Impacted Files Coverage Δ
src/Traits/QueryCacheModule.php 91.48% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3168fb2...b25dd4a. Read the comment docs.

@rennokki rennokki merged commit e3a4501 into renoki-co:master Sep 29, 2021
@rennokki rennokki changed the title Changed getCacheKey to use the method name instead of hardcoding to get [fix] Changed getCacheKey to use the method name instead of hardcoding to get Sep 29, 2021
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.

None yet

2 participants