Added VaryGenerator #36
Conversation
src/Generator/VaryGenerator.php
Outdated
| * | ||
| * @author Tobias Nyholm <[email protected]> | ||
| */ | ||
| class VaryGenerator implements CacheKeyGenerator |
There was a problem hiding this comment.
Thank you for the review.
I cannot make it final because PHP spec need to extend this class to be able to run the tests.
|
not sure about this one. the "proper" use case would be to work around a broken server that is not sending the correct vary header for its content. as i understand, you want it to use the github api which likely sends correct headers. i think what i don't like about VaryGenerator that this is suggesting a relation with the Vary response header, which is not the case. could we call it HeaderHashGenerator? building the hash on Cookie and Authorization sounds ok. the proxy mode of the cache plugin could then use this generator by default.
i think for the client mode, we don't need to differentiate on credentials, as its assuming we send requests for the same client, and we cache things with the private cache-control. the problem comes when you are in a multiple users mode, where the http client acts as some sort of proxy between the user and the service that is cached. |
|
Thank you. I've renamed the client.
Don't we? How would we solve the Gitbub issue? // Request 1
$gitbub->setAuthenticationToken('foo');
$response = $github->me(); // MISS
// This will be stored in PSR6 cache
// Request 2, 2 days later
$gitbub->setAuthenticationToken('bar');
$response = $github->me(); // HIT
// This will fetch the cached response.
// Github servers will never see this request. Should this be considered "Client mode" or "Server mode"? |
|
to me, that would be server mode. but i realize that the naming could be confusing... my idea was that client mode would set up things like when you use a web browser, you are one single user and don't change authentication for example. do you have an idea for better names than client and server? single and shared maybe? |
|
You are correct. This is server mode. We need some good docs for this. Do we want to merge a generator like this now, or do we just one the ServerCacheGenerator and ClientCacheGenerator? |
|
i actually quite like my idea to move from the words client and server to single and shared. it might be more intuitive what this is about, or at least avoid assumptions that people get when we talk about server and client. then this could be a SharedCacheKeyGenerator and default to differentiate on cookie and authentication header. wdyt? |
|
I like it. I'll make some updates now |
| { | ||
| $concatenatedHeaders = []; | ||
| foreach ($this->headerNames as $headerName) { | ||
| $concatenatedHeaders[] = sprintf(' %s:"%s"', $headerName, $request->getHeaderLine($headerName)); |
There was a problem hiding this comment.
calling getHeaderLine for a header that is not defined simply returns an empty string. just double-checked in the psr-7 spec. so this is fine
| use Psr\Http\Message\RequestInterface; | ||
|
|
||
| /** | ||
| * Generate a cache key by specify what headers you want to vary on. |
There was a problem hiding this comment.
i'd like to use a different word than "vary" here to avoid confusion with the Vary response header.
| $concatenatedHeaders[] = sprintf(' %s:"%s"', $headerName, $request->getHeaderLine($headerName)); | ||
| } | ||
|
|
||
| return $request->getMethod().' '.$request->getUri().implode('', $concatenatedHeaders); |
There was a problem hiding this comment.
to be consistent with the simple generator, we should also add the body. we might just extend the simple generator and concat parent::generate with the headers
There was a problem hiding this comment.
I believe in cohesion over inheritance here. We should create some kind of chaining or use a separate class for chaining
There was a problem hiding this comment.
we can also just add the body from simple generator. its just one line of code, i think we don't need to get too complicated with this. but we should have the body to not have inconsistent behaviour between the two generators
There was a problem hiding this comment.
Okey, done.
We do not need the if(empty()) check because it was added in the SimpleGenerator because of BC.
|
great, thanks a lot! can you update the doc for this as well please? and should we start using this in the plugin when in server mode? |
Yes!
That would be a BC break, right? |
|
That would be a BC break, right?
i would consider it a bugfix. it will lead to cache misses after a
deployment, but i think its worth it because it avoids major accidents.
|
|
actually as i want to rename the methods to single and shared anways
(for better understanding what the two modes are about), we could add
those and just leave client and server untouched and deprecate them.
|
|
👍 |
What's in this PR?
This is an other generator. This PR is blocked by #35
Why?
This generator will be used by KNP Github API. Related to KnpLabs/php-github-api#557