MaxMindBinaryProvider, GeoIP and GeoIP2

[willdurand]

As mentioned in #301, the current implementation of MaxMindBinaryProvider is far from perfect. This is mostly because of the geoip/geoip package we use under the hood.

Obviously there is a newer lib, named geoip2/geoip2, which would be better. I'd like to switch to this package, but it would imply a BC break, so I suggest the introduction of a new provider (GeoIP2Provider? MaxMindBinary2Provider?) leveraging the geoip2/geoip2 lib. Also, we should mark MaxMindBinaryProvider as deprecated.

WDYT?

[toin0u]

Agreed. I think it's a good idea to do so (as we need before).

[jenswiese]

Hey William,

first of all: great library! I really appreciate your work.
We want to use exactly what is mentioned in this issue. And I consider your proposal to introduce a new provider as perfect.

My question: Do you plan to implement the MaxMindBinary2Provider in the near future? Do you need support? I guess I would implement a provider on my own anyways, because we need this. Maybe I could contribute something. ;)

Best Regards, Jens.

[toin0u]

Hi @jenswiese, I think @willdurand will appreciate a PR :) No doubt about it !

[willdurand]

Yup, feel free to implement this new provider! <3

[jenswiese]

Hey @willdurand,

I'm actually on it. :)

But I'm struggling with two approaches right now.

Unlike the other providers, the MaxMind provider operates on a database file, that has to be passed to the provider. As I noticed the existing MaxMind provider gets a string via the constructor - fine, it works with the current PHP version. But it might break with the following, since the signature of the concrete implementation differs from the abstract (HttpAdapterInterface vs. string).

How to solve this issue:

1.) Easy way, without harming the existing code

Decouple the new GeoIP2 provider from the existing AbstractProvider, and implement just the provider interface. Implement a GeoIPDatabaseAdapter, that encapsulates the database reading (provided by the Maxmind DatabaseReader) - the adapter is not implementing the HttpAdapterInterface, because it's not a Http request.

2.) Hard way, with harming a lot of existing code

Rename HttpAdapterInterface to AdapterInterface. Change the constructor signature of the AbstractProvider to something like this:

    public function __construct(AdapterInterface $adapter, $locale = null)

Change all existing adapters to implement the AdapterInterface, and finally rename the directory "HttpAdapter" to "Adapter". The GeoIP2Adapter would implement the same interface as the HttpAdapters then.

What do you think?

I didn't mean to break your whole library - sorry! :) I just wanted to share my thoughts. Maybe you have another great idea to solve this problem.

Thanks so far, Jens.

[jenswiese]

Hey @willdurand,

regarding my post above, I think I found a better way to combine the best of the two approaches - without messing with the existing code to much. (https://github.com/jenswiese/Geocoder/tree/geoip2)

I will buzz you, when I finished implementing it.

Thanks Jens.