Geocoder icon indicating copy to clipboard operation
Geocoder copied to clipboard

Improve ProviderCache

Open eugenekurasov opened this issue 5 years ago • 5 comments

What are you think about add to ProviderCache flag isAllowEmptyResult?

For example:

$chain = new \Geocoder\Provider\Chain\Chain([
    // ...
]);
$cache = new \Geocoder\Provider\Cache\ProviderCache($chain, $cache, 86400);

When have exceptions from providers the chain catch and return empty ArrayCollection. This result is saved for 24h. I think will be good point if I don't save empty results.

https://github.com/geocoder-php/Geocoder/blob/35650985d8b69ead88340ca5b75546f7117fad06/src/Provider/Cache/ProviderCache.php#L65-L66

Here is example how I think can be:

         $result = $this->realProvider->geocodeQuery($query); 
         if (!$result->isEmpty() || $this->isAllowEmptyResult)) {
             $this->cache->set($cacheKey, $result, $this->lifetime);
         }

What are you think about it?

eugenekurasov avatar Nov 22 '18 10:11 eugenekurasov

Other solution for empty result is to add lifetimeForEmptyResult

eugenekurasov avatar Nov 22 '18 11:11 eugenekurasov

Oh, Hm.

So the cache store empty responses if there is a chain and you got some exceptions, is that correct?

I believe it is always valid to store empty responses. However, we should never store responses if an exception is thrown.

Nyholm avatar Dec 22 '18 11:12 Nyholm

@Nyholm

Yes we save empty response on error when use chain.

I am agree that empty response is valid response, but with chain we hide exceptions and this is problem. lifetimeForEmptyResult and lifetime is like strategy for save for empty and full response.

When lifetimeForEmptyResult null then behavior will not changed. This is just my suggestions for improve Cache provider for more customize. If you don't like this idea I can close issue. Thanks for your any opinion.

eugenekurasov avatar Dec 24 '18 09:12 eugenekurasov

I really think it is an issue so I will not close it.

Im not a super fan of doing a workaround the problem. I would be much happier trying to fix the real issue.

Could we change the Chain provider somehow? Say that we allow it to throw exception if (A) an exception is thrown and (B) no result is found.

Nyholm avatar Dec 24 '18 09:12 Nyholm

@Nyholm

I'd be happy to create a PR for your solution if you want 😄

atymic avatar Jul 01 '19 01:07 atymic