geocoder-php / Geocoder

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

feat: save chain exceptions #978

Closed atymic closed 1 year ago

atymic commented 5 years ago

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;
    })
}
georgecoca commented 5 years ago

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.

Nyholm commented 4 years ago

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.

atymic commented 4 years ago

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

Nyholm commented 4 years ago

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.

atymic commented 4 years ago

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)

Nyholm commented 4 years ago

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

Finesse commented 4 years ago

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.

alexsegura commented 3 years ago

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?