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

Add cache bypass header #406

Merged
merged 7 commits into from
Aug 1, 2022
Merged

Add cache bypass header #406

merged 7 commits into from
Aug 1, 2022

Conversation

fgilio
Copy link
Contributor

@fgilio fgilio commented Jul 20, 2022

👋 hi!

What do you think of adding a way of bypassing the cache?

Our use case is that we want to monitor the performance of parts of our app, and we want to get data for both cached and non cached responses. We'd actually add this header in Oh Dear.

I was working on adding this functionality to our custom ResponseCacheProfile and wondered if others might find it useful too.

@freekmurze
Copy link
Member

It seems like this breaks the tests. Could you take a look at that?

@fgilio
Copy link
Contributor Author

fgilio commented Jul 29, 2022

Sure!
I mostly wanted to know if you'd be interested in the feature

@fgilio
Copy link
Contributor Author

fgilio commented Jul 30, 2022

I just saw what was causing the tests to fail haha I don't know if it's funny or stupid 🤦 .
In any case, it's fixed now!
I'll work on adding tests specific to the feature

@fgilio fgilio marked this pull request as ready for review July 31, 2022 22:16
@fgilio
Copy link
Contributor Author

fgilio commented Jul 31, 2022

I've added a test and documented the feature, let me know what you think!

@freekmurze freekmurze merged commit 5bb27cd into spatie:main Aug 1, 2022
@freekmurze
Copy link
Member

Very nice, thank you!

@fgilio
Copy link
Contributor Author

fgilio commented Aug 1, 2022

🎉 thanks for merging!
I'm eager to use it in Oh Dear

@fgilio
Copy link
Contributor Author

fgilio commented Aug 1, 2022

@freekmurze we might have an issue here that I oversaw at first, please let me know what you think:
The way it's implemented it'll prevent a response from being cached, but if the response is already cached then it'll still return the cached version.
I think a better way would be to do the check here https://github.com/spatie/laravel-responsecache/blob/main/src/Middlewares/CacheResponse.php#L30, instead of doing it in the ResponseCacheProfile.

I'll wait for your reply and create a new PR with the changes, if you agree.

@freekmurze
Copy link
Member

Ok let's add it 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.

2 participants