search

geocoder-php / Geocoder

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

Making the `ProviderAggregator` final makes it harder to test #727

Closed ddinchev closed 7 years ago

ddinchev commented 7 years ago

In my application, I have a thin wrapper around geocoder, that would depend on Geocoder\ProviderAggregator. I'd test that by mocking it. However, mocking final classes is not possible (not with the defacto standard phpunit-mock-objects at least). So after updating to the latest master, my tests now break.

What is the benefit or goal of having the ProviderAggregator to be final? Could we also extract an Aggregator interface?

Nyholm commented 7 years ago

Thank you for this issue.

I usually make all classes final as it is much easier to remove a final than adding one. For this specific class there are no real reasons. You can send a PR to remove it if you want.

May I ask why you need to extend the ProviderAggregator?

ddinchev commented 7 years ago

I don't - just not having an interface makes it a requirement to mock the class itself. And final classes are not mockable.

One potential case for extending (or providing an alternative implementation of such aggregator) is such where you would have a certain logic to automatically select a provider based on your query. For example -> use geoip2 for valid IP address geocode() calls, use google maps for anything else. Given the popularity of the library, I think more options to extend the library are better. I'd be happy with an interface covering the public API of the class as well 👍

Nyholm commented 7 years ago

That is an cool idea. Let me make a PR for that.

Nyholm commented 7 years ago

Thank you @ddinchev.

I will close this PR. ProviderAggregator is not final any more and your specific use case is now supported in the library.