Added support for Last-Modified and ETag by Nyholm · Pull Request #8 · php-http/cache-plugin

Nyholm · 2016-07-29T12:43:44Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	yes
Deprecations?	no
Related tickets	fixes#2
Documentation
License	MIT

What's in this PR?

We should allow the server to answer 304 which allows us to use a response in the cache.
The BC break here is that all existing items in the cache will be invalid. Is that an issue? It is a cache and not a storage.

I introduced a new config parameter cache_lifetime that says how long the item should be in cache without it being used.

I did also add a few parameters to the array we store in the cache such as expiresAt, createdAt, etag.

To Do

Fix the tests

sagikazarmark · 2016-07-29T12:50:14Z

+ $cacheItem->set($data)->expiresAfter($this->config['cache_lifetime'] + $data['expiresAt']);
+ $this->pool->save($cacheItem);
+
+ return $this->createResponseFromCacheItem($cacheItem);


What if the server responds with 304 but there is no cache?

We do now that it is a cache. Or else the If-Modified-Since and If-None-Match would not have been added. And correct me if I'm wrong but I don't think the server will answer 304 without those headers in the request.

Yes, those headers may have been added before. I'll just make sure to pass the response then.

sagikazarmark · 2016-07-29T12:52:07Z

Why do we need cache lifetime? Normally the server tells us how long it should be cached, doesn't it?

Nyholm · 2016-07-29T12:57:30Z

Yes, but we would like to have the possibility to serve an expired response if the server answers 304.

Nyholm · 2016-07-29T13:08:57Z

T0: We make a request. Server responds saying the response is valid until T10. Return response form server
T5: We make a request. The response is in the cache and we know it is valid until T10. Return response from cache
T15: We make a request. The response is in the cache but is stale. We ask the server about the response and server answer 304 and that the response is valid until T25. Return response from cache
T20: We make a request. The response is in the cache and we know it is valid until T25. Return response from cache

Nyholm · 2016-07-31T14:37:27Z

 $item->isHit()->willReturn(false);
- $item->set(['response' => $response, 'body' => $httpBody])->willReturn($item)->shouldBeCalled();
- $item->expiresAfter(60)->willReturn($item)->shouldBeCalled();
+ $item->set()->willReturn($item)->shouldBeCalled();


I can't figure out a good way to write this line. I cant specify the exact data since it is dependent on time().
Can I say $item->set(<any argument>)->willRetu....?

Nyholm · 2016-07-31T16:00:01Z

I've figured out the tests and I found some bugs.

Im done with the code changes and this PR is ready for review.

GrahamCampbell · 2016-08-02T09:07:06Z

 private function configureOptions(OptionsResolver $resolver)
 {
 $resolver->setDefaults([
+ 'cache_lifetime' => 2592000, // 30 days


might be clearer to actually have an arithmetic expression here since it's only executed once

Nyholm · 2016-08-02T09:28:14Z

Thank you @GrahamCampbell. I've update the PR according to your suggestions.

[ci skip] [skip ci]

GrahamCampbell · 2016-08-02T10:50:01Z

 function let(CacheItemPoolInterface $pool, StreamFactory $streamFactory)
 {
- $this->beConstructedWith($pool, $streamFactory, ['default_ttl'=>60]);
+ $this->beConstructedWith($pool, $streamFactory, ['default_ttl'=>60, 'cache_lifetime'=>1000]);


GrahamCampbell · 2016-08-02T11:03:40Z

- $this->beConstructedWith($pool, $streamFactory, ['default_ttl'=>60, 'cache_lifetime'=>1000]);
+ $this->beConstructedWith($pool, $streamFactory, [
+ 'default_ttl' => 60,
+ 'cache_lifetime'=>1000


missing spaces?

GrahamCampbell · 2016-08-02T11:06:07Z

+ }
+ }
+
+ return;


don't need this statement?

See #8 (comment)

yeh, i replied there

GrahamCampbell · 2016-08-02T11:09:53Z

+ $cacheItem
+ ->expiresAfter($this->config['cache_lifetime'] + $maxAge)
+ ->set([
+ 'response' => $response,


is this one level not enough indented?

Thank you. But shouldn't StyleCi pick this up?

Symfony has no rules about indentation like this I think?

GrahamCampbell · 2016-08-02T11:13:06Z

Looks good. I've not physically tested this in the real world yet though.

GrahamCampbell · 2016-08-02T11:22:21Z

+ $maxAge = $this->getMaxAge($response);
+ $currentTime = time();
+ $cacheItem
+ ->expiresAfter($this->config['cache_lifetime'] + $maxAge)


Why are we doing this please?

We would like to serve Responses that the server once said should expire.
See #8 (comment)

when doing cache validation, we keep outdated caches and send If-Modified-Since / If-None-Match requests to the server to check if the cache is ok to be used. we might add some comment in the code though, to make it clear.

dbu · 2016-08-02T13:15:41Z

went over the code, looks good to me.

for a moment i was wondering about stale-while-validate but this is just content validation and not advanced proxy operations. in async mode we theoretically could use stale-while-validate to return the expired response and re-fetch to our local cache. but this has nothing to do with this PR and anyways seems like a bad use case - one should use a dedicated caching proxy for such things, not implement this on client side.

Nyholm · 2016-08-02T14:20:58Z

TODO:

Decide on how to handle updates in the array that is stored in the cache. isset or cache busting Added support for Last-Modified and ETag #8 (comment)

Nyholm · 2016-08-02T15:49:11Z

We use issets.

This PR is ready to be merged

sagikazarmark · 2016-08-02T20:56:00Z

Thanks, will tag a release soon.

Nyholm · 2016-08-02T21:00:51Z

Awesome. Thank you for all the reviews and feedback.

sagikazarmark reviewed Jul 29, 2016
View reviewed changes

Nyholm reviewed Jul 31, 2016
View reviewed changes

GrahamCampbell reviewed Aug 2, 2016
View reviewed changes

Nyholm added 11 commits August 2, 2016 11:36

Added support for Last-Modified and ETag
824b9aa

style fix
3b2f720

Return the 304 response if we do not have a response in cache
3f36fb5

Bugfix and started on the tests
39c0cfe

Style fixes
33c6e1c

Tests are passing
83e22ce

Style fix
370b0a0

Added tests
d89d233

Updated comment
23d0cd4

Applied fixes from StyleCI
bf2e511
[ci skip] [skip ci]

Made the code easier to read
eb2cd58

Nyholm force-pushed the issue-2 branch from 8f8cc5e to eb2cd58Compare August 2, 2016 09:36

Updated hash
0e6792a

GrahamCampbell reviewed Aug 2, 2016
View reviewed changes

Minor
8c5047f

GrahamCampbell reviewed Aug 2, 2016
View reviewed changes

cs
68edac2

GrahamCampbell reviewed Aug 2, 2016
View reviewed changes

Removed return statement
4ffc484

GrahamCampbell reviewed Aug 2, 2016
View reviewed changes

cs
91a031a

GrahamCampbell reviewed Aug 2, 2016
View reviewed changes

Nyholm added 2 commits August 2, 2016 15:45

Added comment
75db148

Removed period
dfa5f17

Added docs about isset
cc8ac75

sagikazarmark merged commit 4bf191d into masterAug 2, 2016

sagikazarmark deleted the issue-2 branch August 2, 2016 20:55

This was referenced Aug 3, 2016
Bumped branch alias #15
Merged
changelog for cache validation #16
Merged

Nyholm mentioned this pull request Aug 5, 2016
Use sha1 to reduce the risk of collisions #12
Merged
3 tasks

+ }
+ }
+ return;

Uh oh!

Conversation

Nyholm commented Jul 29, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's in this PR?

To Do

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagikazarmark commented Jul 29, 2016

Uh oh!

Nyholm commented Jul 29, 2016

Uh oh!

Nyholm commented Jul 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nyholm commented Jul 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nyholm commented Aug 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GrahamCampbell commented Aug 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Aug 2, 2016

Uh oh!

Nyholm commented Aug 2, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nyholm commented Aug 2, 2016

Uh oh!

sagikazarmark commented Aug 2, 2016

Uh oh!

Nyholm commented Aug 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Nyholm commented Jul 29, 2016•
edited
Loading

Nyholm commented Aug 2, 2016•
edited
Loading