geocoder-php / Geocoder

The most featured Geocoder library written in PHP.
https://geocoder-php.org
MIT License
3.95k stars 517 forks source link

Improve ProviderCache #904

Open eugenekurasov opened 5 years ago

eugenekurasov commented 5 years ago

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 commented 5 years ago

Other solution for empty result is to add lifetimeForEmptyResult

Nyholm commented 5 years ago

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.

eugenekurasov commented 5 years ago

@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.

Nyholm commented 5 years ago

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.

atymic commented 5 years ago

@Nyholm

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