Delegating strategy

[bpolaszek]

Hi there,

I know it's my 3rd pull request for today (the last one, I promise :)), but this middleware is awesome and I'm trying to fit common needs.

Here's a DelegatingCacheStrategy.

A Guzzle client rarely connects to only 1 host.

Some hosts handle cache differently, and sometimes, depending on the API you connect to, you need to have more control on the cache.

The DelegatingCacheStrategy allows you to define a default caching strategy (defaults to none), and overriding it depending on the Request object.

Since I needed to create a test-specific class, I had to separate tests from the code and to create a namespace dedicated to tests. This does not break anything in phpunit / travis theorically.

I've updated the README.

Please let me know if there's something I can improve.

Ben

[Kevinrob]

Maybe it's better to use callable for accepting callable object too?

[bpolaszek]

Well, the Kevinrob\GuzzleCache\Tests\RequestMatcher\ClosureRequestMatcher class is actually only intended to unit tests (as its namespace implies). If PHP7 were the minimum requirement, I would have used an anonymous class for using it in PHPUnit. As a consequence, since this class should not be used outside of Kevinrob\GuzzleCache\Tests\DelegatingCacheTest, another type-hinting is pointless.

This class is indeed just an example of implementation of RequestMatcherInterface, but I don't think this example should be followed for production: since request matching targets specific use cases, I suggest letting developers implement RequestMatcherInterface by themselves instead of providing a default class. What's your opinion?

If you think a default implementation should be provided, then I suggest the removal of the RequestMatcherInterface and type-hint a callable in DelegatingCacheStrategy::registerRequestMatcher(), that should return a boolean.

This would lead to this:

final public function registerRequestMatcher(callable $requestMatcher, CacheStrategyInterface $cacheStrategy)
{
    $this->requestMatchers[] = [
        $requestMatcher,
        $cacheStrategy,
    ];
}

private function getStrategyFor(RequestInterface $request)
{
    foreach ($this->requestMatchers as $requestMatcher) {
        list($match, $cacheStrategy) = $requestMatcher;
        if ($match($request)) {
            return $cacheStrategy;
        }
    }

    return $this->defaultCacheStrategy;
}

What do you think?

Besides, since the default PHP version is 5.5 (so, no return type-hinting), do you think we should throw an InvalidArgumentException when the RequestMatcher|callable does not return a strict boolean? Or do we do this the lazy way?

[Kevinrob]

Ho right. I don't see that it's only for tests!
I am influenced by functional programming... So my thoughts are biased. I trust you.

[Kevinrob]

@bpolaszek Wow, it's a very good idea! I like the functionality.
I just left one comment about Closure vs callable, can you just tell me what you thing. And I will wait on the PR #84 (this one includes change of it...).

A big thanks for the time that you take for this project!

[bpolaszek]

Great, thanks for merging!