Make sure we vary on the Authorization header #558
Conversation
lib/Github/HttpClient/Builder.php
Outdated
| @@ -181,6 +182,9 @@ public function addHeaderValue($header, $headerValue) | |||
| */ | |||
| public function addCache(CacheItemPoolInterface $cachePool, array $config = []) | |||
| { | |||
| if (!isset($config['cache_key_generator'])) { | |||
| $config['cache_key_generator'] = new HeaderCacheKeyGenerator(['Authorization', 'Cookie', 'Accept']); | |||
There was a problem hiding this comment.
Is there any more headers we should vary on?
There was a problem hiding this comment.
The content type of the request perhaps?
There was a problem hiding this comment.
I think this will be sufficient! Maybe @GrahamCampbell can test this pr to see if it fixes his problem from #557
EDIT: nvm I've missed his comment :p
lib/Github/HttpClient/Builder.php
Outdated
| @@ -181,6 +182,9 @@ public function addHeaderValue($header, $headerValue) | |||
| */ | |||
| public function addCache(CacheItemPoolInterface $cachePool, array $config = []) | |||
| { | |||
| if (!isset($config['cache_key_generator'])) { | |||
| $config['cache_key_generator'] = new HeaderCacheKeyGenerator(['Authorization', 'Cookie', 'Accept']); | |||
There was a problem hiding this comment.
The content type of the request perhaps?
|
I wonder if we could add an integration test to make sure that we don't reuse caches by mistake? |
|
I've added functional tests for this. Please review and/or test so I can merge and tag e new release |
| $this->assertEquals('nyholm', $userA['login']); | ||
|
|
||
| $userB = $github->currentUser()->show(); | ||
| $this->assertEquals('nyholm', $userB['login'], 'Two request following each other should be cached.'); |
There was a problem hiding this comment.
Client should always send a request, and then see if we get a "not modified" response. We shouldn't just randomly cache for 600 seconds.
There was a problem hiding this comment.
Client should always send a request, and then see if we get a "not modified" response
No, not always. If server says that it is okey to cache for X time, we do not need to send a request for that period.
But the logic you are referring to is in the php-http/cache-plugin.
We shouldn't just randomly cache for 600 seconds.
I set the default ttl to arbitrary number here to force the first the request to be cached (for a time longer then the tests run). Doing that I can easily verify if we make a second request or not. (Im using the MockClient, Im not actually making any HTTP calls. )
This will fix #557
It is blocked by php-http/cache-plugin#35 and php-http/cache-plugin#36