Geocoder icon indicating copy to clipboard operation
Geocoder copied to clipboard

feat: save chain exceptions

Open atymic opened this issue 5 years ago • 8 comments

Relates to #881

Would be interested in your thoughts @geodeveloper @Nyholm @jbelien This allows the chain to execute as it does currently, but adds the ability to handle the exceptions if they occured.

For example, you could log QuotaExceeded exceptions and ignore UnsupportedOperation ones.

Example:

$chain->geocodeQuery($query);

if ($chain->getPreviousQueryExceptions()) {
    $exceptionsToLog = array_filter($chain->getPreviousQueryExceptions(), function (\Throwable $e) {
         return $e instanceof QuotaExceeded;
    })
}

atymic avatar Jul 01 '19 01:07 atymic

Hi @atymic,

Thanks for bringing up this solution. I guess that will work as a workaround. I think the class Chain needs a better approach to handle different scenarios and the final class should be reconsidered if it's really necessary.

georgecoca avatar Jul 04 '19 09:07 georgecoca

What if we collect exceptions and then if no result we throw either:

  1. one exception if count($exceptions) === 1
  2. A new ChainExceptionCollection($exceptions)

We do someting similar in Symfony/messanger.

Nyholm avatar Aug 14 '19 13:08 Nyholm

@Nyholm

I'm not sure that would work in this context. The issue is the chain provider only runs until we get a valid result (which it returns), so we'd never have a chance to throw the exceptions.

For example:

Provider 1 -> Exception
Provider 2 -> Exception
Provider 3 -> Success

Results in return of $result from the last provider

Unless we changed the behaviour to throw an exception even if we find a valid result, the only case where we would throw a ChainExceptionCollection would be when they all fail.

atymic avatar Aug 15 '19 05:08 atymic

We should not throw exception if we get a result. They should only be thrown if no result was found.

I like the idea of getting exceptions throw a getter even if we do find a result.

Nyholm avatar Aug 15 '19 06:08 Nyholm

We should not throw exception if we get a result. They should only be thrown if no result was found.

I agree. I can add a ChainExceptionCollection in the case that no results are found, but that would be a BC break hence why I went with this solution (which doesn't break BC)

atymic avatar Aug 15 '19 06:08 atymic

No it wouldn’t. The contract is from the interface. The interface states that an exception might be thrown, right?

Nyholm avatar Aug 15 '19 07:08 Nyholm

I can add a ChainExceptionCollection in the case that no results are found

I want to clarify. A provider interface says that a provider returns an empty collection when there is no result for the query and no error has happened (e.g. when the given address doesn't exist). Therefore empty result doesn't mean that an exception must be thrown. I think that the chain provider must throw an exception only when no results are retrieved and an inner provider has thrown an exception.

Finesse avatar Oct 08 '19 07:10 Finesse

I think that the chain provider must throw an exception only when no results are retrieved and an inner provider has thrown an exception.

I agree with @Finesse.

Well, actually, it depends on what a "chain" provider is intended to be used for. It is here to have a "winner takes all" behavior, or is it here to have a "fault tolerant" behavior?

alexsegura avatar Sep 22 '20 18:09 alexsegura