'respect_response_cache_directives' is ignored

[nunoplopes]

We have the following code in CachePlugin.php:

    public static function clientCache(CacheItemPoolInterface $pool, StreamFactoryInterface $streamFactory, array $config = [])
    {
        // Allow caching of private requests
        if (\array_key_exists('respect_response_cache_directives', $config)) {
            $config['respect_response_cache_directives'][] = 'no-cache';
            $config['respect_response_cache_directives'][] = 'max-age';
            $config['respect_response_cache_directives'] = array_unique($config['respect_response_cache_directives']);
        } else {
            $config['respect_response_cache_directives'] = ['no-cache', 'max-age'];
        }

        return new self($pool, $streamFactory, $config);
    }

If I pass a config with 'respect_response_cache_directives' => [], it is ignored and replaced with ['no-cache', 'max-age'].
I wanted it to always cache any page regardless of the server directives. It's impossible to do that right now. I'm not sure what's the intention of the code above.

[dbu]

hm, good question indeed. it looks like one can only add additional directives to respect, but not remove no-cache and max-age. this had been added like this with the original clientCache factory method in 1568bea, so it was always like this.

i see that the serverCache factory method does not tamper with that option, so maybe you could use that? (you still need to set the option to empty array, otherwise the OptionsResolver will add a bunch of directives to respect.

[nunoplopes]

The issue is that I'm not using php-http directly but through the php github API (https://github.com/KnpLabs/php-github-api).
But not respecting the respect directive seems wrong to me.

[dbu]

oh i see. you could extend the Builder and overwrite the addCache method.

while i agree that the current behaviour is unexpected and strange, i am afraid to change it as a bugfix - it would be significant behaviour change

[dbu]

unfortunate timing btw, we just tagged 2.0 2 weeks ago. otherwise we could have declared it one of the important BC breaks of 2.0.

we could add a new factory method that does not force any of the options and deprecate this one, to have a sane upgrade path. and once that is released, we could change the php-github-api to use the new method. do you want to work on that?

[nunoplopes]

Sorry for the delay; just got back to this. Unfortunately overriding the addCache method doesn't work because the fields are private.
Would you be open to a patch to add a $config['force_respect'] or similar name to skip the current code that adds no-cache & max-age?

[dbu]

indeed. i see 2 options:

1. you work with the knplabs github api client to change the builder to use the CachePlugin constructor directly
2. we do the BC break here in the plugin and release a new major version. actually i think a new major version sounds alright to me.

do you want to do a pull request with the change? i just created the 3.x branch now - the change would be against 3.x
and if you have the time and energy for it, implementing #31 and #47 would be awesome too (not in the same pull request though)

[dbu]

fixed in #93