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 caching streams and cache lifetimes. #14

Merged
merged 2 commits into from Jul 20, 2015
Merged

Fix caching streams and cache lifetimes. #14

merged 2 commits into from Jul 20, 2015

Conversation

EAnushan
Copy link
Contributor

I found some issues with the way caching is implemented at the moment.

  1. Serializing the response stream causes the stream to seek to the end. You need to rewind the stream or the caller code will receive an empty body.
  2. All entries are being cached forever. I forwarded a lifetime through Doctrine's Cache interface.

@EAnushan
Copy link
Contributor Author

I think the tests may need to be updated.

@Kevinrob
Copy link
Owner

Thank for your contribution!
For the stream, it's alright!

But for the TTL, you miss something with 2 situations (the broken tests):

  • Cache-Control: stale-if-error=XXX (Kevinrob\GuzzleCache\HeaderExpireTest::testStaleIfErrorHeader)
  • HTTP re-validation

Can you please make a PR with only the stream issue for a rapid merging?
For the TTL, I suggest to add a method in CacheEntry like getTTL(). Like that, we can keep the logic of lifetime in it. (infinite TTL if some conditions (re-validation, etc.))

@Kevinrob
Copy link
Owner

Or if you are okay, I can do it myself after the merge. :)

@EAnushan
Copy link
Contributor Author

I think I will leave this up to you as I haven't read the entire RFC yet for HTTP caching if that's okay with you?

Kevinrob added a commit that referenced this pull request Jul 20, 2015
Fix caching streams and cache lifetimes (WIP).
@Kevinrob Kevinrob merged commit fccedf6 into Kevinrob:master Jul 20, 2015
@Kevinrob Kevinrob mentioned this pull request Jul 20, 2015
@Kevinrob
Copy link
Owner

It's done!
Thank again for your help!

@EAnushan
Copy link
Contributor Author

Thank you =)

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