Implement caching types #26

acrobat · 2017-02-22T07:35:35Z

Q	A
Bug fix?	yes and no
New feature?	yes
BC breaks?	no
Deprecations?	no
Related tickets	fixes #3
Documentation	php-http/documentation#178
License	MIT

This is a first simple version to get the discussion going and see what changes are needed here.

What's in this PR?

As discussed in #3 this PR introduces 2 factory methods and an option to specify which cache control directives should be checked

Checklist

Updated CHANGELOG.md to describe BC| new feature / bugfix
Documentation pull request created [cache-plugin] document configuring respect_response_cache_headers documentation#178
Add tests

dbu

i find the name excluded_directives confusing. we seem to not exclude those directives, but use them.

i would call the option respect_response_cache_directives and deprecate respect_cache_headers.

dbu · 2017-02-22T10:32:19Z

src/CachePlugin.php

+    public static function clientCache(CacheItemPoolInterface $pool, StreamFactory $streamFactory, array $config = [])
+    {
+        // Allow caching of private requests
+        $config['excluded_directives'] = ['no-cache'];


we should check if the option already exists and add it to the existing list

Or maybe just allow respect_cache_headers to be either boolean or array. If boolean true then assume it is all directives. If false then assume it is empty array.

the things like no-cache are not headers, but directives. and some can be both on request and/or response, and we should treat that separately to allow the user full control. maybe they want no-cache on request to be respected, but on the response they still want to cache...

I'm also more in favor of separate options. Because I would avoid mixing option types bool/array

acrobat · 2017-02-22T17:16:41Z

@dbu thanks for the feedback! I've updated the code!

acrobat · 2017-02-23T07:44:07Z

I've tested my patch on a project using the github api (which sends the private cache directive with each request). And I can confirm it works when you construct the cache plugin with the clientCache method.

The phpspec tests fail after the patch, but I will look into this fail later tonight!

dbu

thanks, i think we move in the right direction with this. this will end up being quite flexible, while still being reasonably understandable.

dbu · 2017-02-23T07:46:16Z

src/CachePlugin.php

@@ -242,7 +268,7 @@ private function createCacheKey(RequestInterface $request)
     */
    private function getMaxAge(ResponseInterface $response)
    {
-        if (!$this->config['respect_cache_headers']) {
+        if (!$this->config['respect_cache_headers'] || !in_array('max-age', $this->config['respect_response_cache_directives'], true)) {


i would do the cleanup (create an empty array in respect_response_cache_directives) of this in the constructor and issue a deprecation warning if respect_cache_headers is defined. if both are defined in the constructor, throw an exception as its wrong to do that.

dbu · 2017-02-23T07:49:51Z

src/CachePlugin.php

-            return false;
+
+        foreach ($this->config['respect_response_cache_directives'] as $cacheDirective) {
+            if ($this->getCacheControlDirective($response, $cacheDirective)) {


this means that if max-age is set to be respected and defined, we do not cache. i think you need a constant for NO_CACHE_FLAGS and array intersect that with respect_response_cache_directives

Yes you are right, I will look into a way to do this. We can't use a constant with an array value as the cacheplugin supports php >=5.4

ah, right. then private property is fine too

dbu · 2017-02-23T07:50:49Z

src/CachePlugin.php

@@ -58,6 +59,28 @@ public function __construct(CacheItemPoolInterface $pool, StreamFactory $streamF
        $this->config = $optionsResolver->resolve($config);
    }

+    public static function clientCache(CacheItemPoolInterface $pool, StreamFactory $streamFactory, array $config = [])


lets add a phpdoc here that says this sets up the cache plugin in a client mode. it will cache even with cache-control private or no-store.

dbu · 2017-02-23T07:52:00Z

src/CachePlugin.php

            'respect_cache_headers' => true,
            'hash_algo' => 'sha1',
            'methods' => ['GET', 'HEAD'],
+            'respect_response_cache_directives' => ['no-cache', 'private', 'max-age'],


to stay BC, this should include no-store

acrobat · 2017-02-23T17:42:40Z

@dbu I've incorporated the feedback and I've added a phpspec test for this new option/behavior

Nyholm

Thank you for this PR. I really like the idea of these factory methods.

I had some comments on some things I did not understand.

Nyholm · 2017-02-23T17:57:45Z

src/CachePlugin.php

@@ -34,31 +35,83 @@
    private $config;

    /**
+     * Cache directives indicating if a response can be cached.


"can be cached"? "noCacheFlags"?

You mean cannot be cached?

Nyholm · 2017-02-23T17:59:08Z

src/CachePlugin.php

     * }
     */
    public function __construct(CacheItemPoolInterface $pool, StreamFactory $streamFactory, array $config = [])
    {
        $this->pool = $pool;
        $this->streamFactory = $streamFactory;

+        if (isset($config['respect_cache_headers']) && $config['respect_response_cache_directives']) {
+            throw new \InvalidArgumentException('You can\'t provide config option "respect_cache_headers" and "respect_response_cache_directives". Use "respect_response_cache_directives" instead.');


Generally. Make lines shorter than 100 chars. (PSR-2)

Nyholm · 2017-02-23T17:59:19Z

src/CachePlugin.php

        $optionsResolver = new OptionsResolver();
        $this->configureOptions($optionsResolver);
        $this->config = $optionsResolver->resolve($config);
    }

    /**
+     * This method will setup the cachePlugin in client cache mode. When using the client cache mode the plugin will cache responses with `private` cache directive.


Same here.. Long lines.

Nyholm · 2017-02-23T18:00:01Z

src/CachePlugin.php

+     *
+     * @param CacheItemPoolInterface $pool
+     * @param StreamFactory          $streamFactory
+     * @param array                  $config


"See config for constructor" or something similar.

Nyholm · 2017-02-23T18:00:51Z

src/CachePlugin.php

+    }
+
+    /**
+     * This method will setup the cachePlugin in server cache mode. This is the default caching behavior (refuses to cache responses with the `private`or `no-cache` directives.


Missing space

Nyholm · 2017-02-23T18:03:23Z

src/CachePlugin.php

+        // Allow caching of private requests
+        if (isset($config['respect_response_cache_directives'])) {
+            if (!in_array('no-cache', $config['respect_response_cache_directives'], true)) {
+                $config['respect_response_cache_directives'][] = ['no-cache'];


Should you add an array here? ['no-cache'] vs 'no-cache'.

Nyholm · 2017-02-23T18:03:53Z

src/CachePlugin.php

+     *
+     * @param CacheItemPoolInterface $pool
+     * @param StreamFactory          $streamFactory
+     * @param array                  $config


Same here, document what parameters or refer to constructor

Nyholm · 2017-02-23T18:05:40Z

src/CachePlugin.php

@@ -183,11 +236,12 @@ protected function isCacheable(ResponseInterface $response)
        if (!in_array($response->getStatusCode(), [200, 203, 300, 301, 302, 404, 410])) {
            return false;
        }
-        if (!$this->config['respect_cache_headers']) {


This cannot be removed. That will break BC.

On line 361 I've added some BC logic. If this option is false we add an empty array so all the checks are skipped and original behavior is kept

Nyholm · 2017-02-23T18:09:30Z

src/CachePlugin.php

-        if (!$this->config['respect_cache_headers']) {
-            return true;
-        }
-        if ($this->getCacheControlDirective($response, 'no-store') || $this->getCacheControlDirective($response, 'private')) {


Removing this will also break BC, right?

Can we do something smart in the constructor? If respect_cache_headers is true then populate respect_response_cache_directives with "no-store", "private".

I don't think this breaks BC as the default options include the headers

agree with acrobat, this is handled below with the directives

Nyholm · 2017-02-23T18:10:53Z

src/CachePlugin.php

-        if ($this->getCacheControlDirective($response, 'no-store') || $this->getCacheControlDirective($response, 'private')) {
-            return false;
+
+        $cacheableDirectives = $this->extractDirectives($this->config['respect_response_cache_directives'], $this->noCacheFlags);


$cacheableDirectives can only be one of these four (no matter what). Is this really the intention?

['no-cache', 'private'] ['private'] ['no-cache'] []

The directive list contains other values like max-age, no-store and possibly other values in the future. So I've added this method to easily extract cache directives if they are provided in the config

i agree with the approach. but i would directly use array_intersect here. as you assign it to a variable, its clear enough without the method. i would call the variable $nocacheDirectives.

acrobat · 2017-02-23T19:59:42Z

Thanks @Nyholm for the review! I've fixed your comments

dbu · 2017-02-24T16:43:06Z

src/CachePlugin.php

+        if (isset($config['respect_response_cache_directives'])) {
+            $config['respect_response_cache_directives'][] = 'no-cache';
+            $config['respect_response_cache_directives'][] = 'max-age';
+            array_unique($config['respect_response_cache_directives']);


afaik array_unique does not modify the input array. you need to re-assign the value to the field.

yes you're right! Fixed!

dbu · 2017-02-24T16:45:46Z

src/CachePlugin.php

-        if ($this->getCacheControlDirective($response, 'no-store') || $this->getCacheControlDirective($response, 'private')) {
-            return false;
+
+        $cacheableDirectives = $this->extractDirectives($this->config['respect_response_cache_directives'], $this->noCacheFlags);


i agree with the approach. but i would directly use array_intersect here. as you assign it to a variable, its clear enough without the method. i would call the variable $nocacheDirectives.

dbu · 2017-02-24T16:46:08Z

src/CachePlugin.php

-        if (!$this->config['respect_cache_headers']) {
-            return true;
-        }
-        if ($this->getCacheControlDirective($response, 'no-store') || $this->getCacheControlDirective($response, 'private')) {


agree with acrobat, this is handled below with the directives

acrobat · 2017-02-24T20:44:08Z

@dbu Fixed your new comments!

dbu

we are almost there i think. no-cache was not handled previously, but i consider that a bugfix rather than a BC break.

dbu · 2017-02-25T09:53:39Z

src/CachePlugin.php

+     *             we have to store the cache for a longer time than the server originally says it is valid for.
+     *             We store a cache item for $cache_lifetime + max age of the response.
+     * @var array  $methods list of request methods which can be cached
+     * @var array  $respect_response_cache_directives list of cache directives this plugin will respect while caching responses.


this was indented on purpose, because its the list of fields in the $config array.

Yeah I think this was fix for styleCi but indeed I've reverted the indent changes!

dbu · 2017-02-25T09:53:59Z

src/CachePlugin.php

+     *             We store a cache item for $cache_lifetime + max age of the response.
+     * @var array  $methods list of request methods which can be cached
+     * @var array  $respect_response_cache_directives list of cache directives this plugin will respect while caching responses.
+     *             }.


and this was left becaues it ends the list of options

See comment above

dbu · 2017-02-25T09:56:22Z

src/CachePlugin.php

+     *
+     * @var array
+     */
+    private $noCacheFlags = ['no-cache', 'private'];


i think this list should also have no-store

AFAIK you can actually cache the result if response has no-cache directive. It just means you must always revalidate before actually using the cached result. If origin server returns 304 Not Modified you can use the cached result.

That said the easy implementation is just not to cache. Only difference is some bandwith might be wasted, but implementation is much easier.

https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.9.1

right. lets open an issue to implement the full no-cache behaviour in the future. but rather than completely ignoring, completely not caching is at least correct according to the protocol.

Ok, I've added the no-store directive and left the no-cache for now

created #27 for the full no-cache support

dbu · 2017-02-25T09:58:03Z

src/CachePlugin.php

            'respect_cache_headers' => true,
            'hash_algo' => 'sha1',
            'methods' => ['GET', 'HEAD'],
+            'respect_response_cache_directives' => ['no-cache', 'private', 'max-age', 'no-store'],


looks like adding no-cache is a change over current behaviour. i think we can consider that a bugfix. but please add a note to the changelog so people have a chance of seeing the change.

I've added a line about this change in the changelog!

acrobat · 2017-02-26T08:51:19Z

I've did some changes according to the feedback and updated the changelog. @dbu & @Nyholm do you guys have time for a last review as I think we are almost ready here?

dbu

thanks! almost there. i noticed one glitch in the constructor sanity check.

dbu · 2017-02-27T07:21:48Z

src/CachePlugin.php

     * }
     */
    public function __construct(CacheItemPoolInterface $pool, StreamFactory $streamFactory, array $config = [])
    {
        $this->pool = $pool;
        $this->streamFactory = $streamFactory;

+        if (isset($config['respect_cache_headers']) && $config['respect_response_cache_directives']) {


the second check also needs to have an isset. as is, this will be a fatal error if respect_response_cache_directives is not set. (and throw no exception if respect_cache_headers is set and the other is [])

good catch! Fixed!

acrobat · 2017-02-27T07:29:35Z

Fixed comment from @dbu and squashed/rebase the PR

dbu

looks good to me now. what do the other maintainers think?

Nyholm

Nice. I really like this PR now =)

Thank you @acrobat

dbu · 2017-02-27T08:44:27Z

styleci is a bit weird. i guess we should remove that dot (i would be ok with it, but styleci should be green and i guess we can't easily make it allow just that one place)

acrobat · 2017-02-27T08:47:10Z

@dbu Yeah it was indeed a bit weird that's why I left it there. But removed the dot and now is styleci passing again!

dbu · 2017-02-27T09:26:05Z

thanks a lot! created php-http/documentation#178 about the documentation. needs not a lot, but would be great to have it mentioned in the doc

acrobat · 2017-02-27T09:26:38Z

Yes indeed, that was still a todo for after the PR was merged. I will try to do that tonight!

acrobat · 2017-02-27T11:36:34Z

Is there an ETA for the 1.3.0 release? As I would like to implement this feature in KnpLabs/php-github-api

Nyholm · 2017-02-27T11:48:13Z

There is no ETA yet. But I guess it will be sooner rather than later.

Feel free to start a PR on KnpLabs/php-github-api and I'll be happy to review that one. I'll make sure both libraries release a new tag with this feature.

acrobat · 2017-02-27T11:54:22Z

Thanks for the info @Nyholm! I will start a PR to change the cachePlugin usage

acrobat · 2017-03-03T10:35:22Z

@sagikazarmark Do you maybe have some time to create a new release? Would be awesome to use this feature and the method caching feature from #24! Thanks!

tuupola · 2017-03-08T17:00:23Z

Wait a bit with a release. I just noticed that the regexp in #24 allowes / character which should not be there. I will make a bugfix PR tomorrow.

acrobat · 2017-03-19T13:33:06Z

ping @tuupola any time to fix this so we can have a release anytime soon?

sagikazarmark · 2017-03-19T23:51:50Z

Do you maybe have some time to create a new release?

Sorry totally missed this one. My mailbox is quite full lately.

@tuupola ping me if you are done or need anything from me.

tuupola · 2017-03-20T08:18:35Z

Sorry for the delay. I will do it today.

acrobat mentioned this pull request Feb 22, 2017

Cache-Control header handling #3

Closed

dbu reviewed Feb 22, 2017

View reviewed changes

dbu requested changes Feb 23, 2017

View reviewed changes

Nyholm requested changes Feb 23, 2017

View reviewed changes

dbu reviewed Feb 24, 2017

View reviewed changes

dbu requested changes Feb 25, 2017

View reviewed changes

dbu requested changes Feb 27, 2017

View reviewed changes

acrobat force-pushed the caching-types branch from 2cccec7 to 42613cc Compare February 27, 2017 07:29

dbu approved these changes Feb 27, 2017

View reviewed changes

Nyholm approved these changes Feb 27, 2017

View reviewed changes

acrobat changed the title ~~[WIP] Implement caching types~~ Implement caching types Feb 27, 2017

Implement caching types for client and server caching. Fixes php-http#3

1568bea

acrobat force-pushed the caching-types branch from 42613cc to 1568bea Compare February 27, 2017 08:46

dbu merged commit 08ccb67 into php-http:master Feb 27, 2017

dbu mentioned this pull request Feb 27, 2017

[cache-plugin] document configuring respect_response_cache_headers php-http/documentation#178

Closed

acrobat deleted the caching-types branch February 27, 2017 09:26

dbu mentioned this pull request Feb 27, 2017

full support of no-cache #27

Open

acrobat mentioned this pull request Feb 27, 2017

Docs for new cache plugin option php-http/documentation#179

Merged

acrobat mentioned this pull request Feb 27, 2017

Use the new cache plugin factory method KnpLabs/php-github-api#531

Merged

Nyholm mentioned this pull request Mar 23, 2017

Prepare for 1.3 #32

Merged

Implement caching types #26

Implement caching types #26

Conversation

acrobat commented Feb 22, 2017 • edited by dbu Loading

What's in this PR?

Checklist

dbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acrobat commented Feb 22, 2017

acrobat commented Feb 23, 2017

dbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acrobat commented Feb 23, 2017

Nyholm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acrobat commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acrobat commented Feb 24, 2017

dbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuupola Feb 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acrobat commented Feb 26, 2017 • edited Loading

dbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acrobat commented Feb 27, 2017

dbu left a comment

Choose a reason for hiding this comment

Nyholm left a comment

Choose a reason for hiding this comment

dbu commented Feb 27, 2017

acrobat commented Feb 27, 2017

dbu commented Feb 27, 2017

acrobat commented Feb 27, 2017

acrobat commented Feb 27, 2017

Nyholm commented Feb 27, 2017

acrobat commented Feb 27, 2017

acrobat commented Mar 3, 2017

tuupola commented Mar 8, 2017

acrobat commented Feb 22, 2017 •

edited by dbu

Loading

tuupola Feb 25, 2017 •

edited

Loading

acrobat commented Feb 26, 2017 •

edited

Loading