Implement MaxData GeoIP2 provider and database adapter

[jenswiese]

Update README.md, add requirements/suggest to composer.json.

[jenswiese]

Tests are running on my machine. ;) No, seriously I'm on fixing it.

[willdurand]

Ok cool, really cool!

One thing though, while I thought it could have been a good idea to have a dedicated Adapter, it totally breaks the library's philosophy. For instance, you can't use this provider with the chain provider.

[jenswiese]

Hey @willdurand,

how should we proceed with this PR?

I guess I didn't get the point. Do you consider this provider breaking the library's philosophy, or what I mentioned before in #302 ?

Please let me know, whether I can adapt/fix/change something to meet your requirements better.

Thanks Jens.

[willdurand]

I'd like @toin0u and @Baachi POVs first :)

[Baachi]

Why not change the interface to GeoIP2DatabaseAdapter and remove the instanceof call?

[jenswiese]

Good point!
This is exactly what I was struggling with in #302 . The main problem is that the AbstractProvider defines the constructor signature. Thus I was about to propose to change the typehint to something like AdapterInterface in order to have the ability to use a different adapter. But that would mean that we have to adapt all the HttpAdapters, as well as the directory structure/namespaces to unify that.

But as @willdurand mentioned that would break the library's philosophy.

[toin0u]

I think we should keep BC and improve the adapter for the next major release.

[toin0u]

@jenswiese I think it's a really good work :) 👍

[willdurand]

Yeah, I like such contribution too! It just needs some more work to be mergeable :-)

[jenswiese]

Hey guys,

thank you very much for your feedback!

Let me try to wrap it up:

• Remove the trimming of the URL
• You proposed to inject the GeoIP2\Database\Reader into the GeoIP2DatabaseAdapter. I would rather stick to passing on the database path. As I mentioned, I want to hide the dependency to the Reader for the user of the Geocoder library. Because he has to deal with the Adapter and the Provider, and that's it.

When it's okay for you, I will prepare the PR. Otherwise we have to discuss a little. ;)

Cheers Jens.

[toin0u]

Hi @jenswiese,

First of all thanks for you work ! It's really great :)

According to the PR, I see your point but I think like a special provider, a special configuration is ok if we can remove the complexity by using DI. But it's only my POV. WDYT @willdurand @Baachi ?

[oschwald]

I only skimmed this, but if you go the DI route, you could allow the injection of a GeoIp2\ProviderInterface, which will support both the GeoIP2 database and web service.

[jenswiese]

I consider what @oschwald mentioned as a good point and pro injecting the GeoIP2 dependency. When nobody has any concerns I would go that way, and go with the more generic approach.

Cheers Jens.

[toin0u]

@jenswiese @oschwald I think it's a good idea :)

[willdurand]

Looks promising!

[toin0u]

Yay! I will be a great improvement :)

[willdurand]

@jenswiese what's the status of your PR?

[jenswiese]

Hey @willdurand, I considered this as finished. ;)

[toin0u]

@jenswiese What about injecting the GeoIP2 dependency ?

[jenswiese]

@toin0u, it's in 6b5bbd8

[toin0u]

👍

[willdurand]

thank you @jenswiese!