Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve ProviderCache #904

Open
eugenekurasov opened this issue Nov 22, 2018 · 5 comments
Open

Improve ProviderCache #904

eugenekurasov opened this issue Nov 22, 2018 · 5 comments
Labels

Comments

@eugenekurasov
Copy link
Contributor

eugenekurasov commented Nov 22, 2018

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.

$result = $this->realProvider->geocodeQuery($query);
$this->cache->set($cacheKey, $result, $this->lifetime);

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
Copy link
Contributor Author

Other solution for empty result is to add lifetimeForEmptyResult

@Nyholm
Copy link
Member

Nyholm commented Dec 22, 2018

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
Copy link
Contributor Author

@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
Copy link
Member

Nyholm commented Dec 24, 2018

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
Copy link
Member

atymic commented Jul 1, 2019

@Nyholm

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants