[WIP] Extend #357

[willdurand]

This PR is based on @MattKetmo's work. His initial idea was a sort of eye-opener to me, hence this PR.

I removed useless methods in the AbstractProvider. I believe we could have a AbstractLocaleAwareProvider too. The LocaleAwareProvider interface does not make much sense at this point, I'd drop it.

The ProviderBasedGeocoder should be an aggregator or proxy.

The codebase is getting more and more readable and clean.

Edit: I chose to keep the LocaleAwareProvider interface as it is a pretty common use case.

[pyrech]

The FQCN is not needed here ;)

[willdurand]

More work in progress pushed.

[Baachi]

There is a dot to much.

[Baachi]

Really cool work! Looking forward!

[giosh94mhz]

You're doing a great job @willdurand . I have a minor suggestion.

The constructor of an AbstractProvider require an instance of HttpAdapterInterface, which doesn't really make sense when creating "local provider" (GeoIP db, Geonames imported in Doctrine, Apache local extensions and so on). Also, the adapter is never really used in the AbstractProvider class, so.. why not drop it? This will allow better subclassing.

Then, to avoid code duplication, you may just use a simple trait in the subclasses which add setAdapter/getAdapter.

[willdurand]

Thank you!

As for the AbstractProvider, it is a common class for most providers, as most of them depend on a HTTP API. It is not useful for all providers. I'll see how to fix this after this PR :-)

[Baachi]

@willdurand as you bump the php version to 5.4, why not add a HttpAdapterTrait to all providers which use the http protocol?

[willdurand]

To be honest, I don't like traits.

[giosh94mhz]

@willdurand I agree with you that traits are not that cool, but are a decent alternative of having a complex hierarchy (in this case by having two abstract class AbstractProvider and AbstractHttpProvider) or a lot of code getter/setter code-duplication. I like the idea of HttpAdapterTrait, but maybe I am biased since I love the "C++ way" :)

[Baachi]

@willdurand Yeah, traits are not the best OOP way, but it would be the easiest way. If you have a better option, let me know :)

[MattKetmo]

Traits are cool, but I neither don't like them for constructors.
IMO having AbstractProvider + AbstractHttpProvider extends AbstractProvider is 👍.

[giosh94mhz]

@MattKetmo I never meant for constructors: that would be really odd 8-/. Just for protected property + setter/getter...

[Baachi]

I agree with @giosh94mhz . The trait should be a client property with the needed accessors.

[MattKetmo]

Indeed, I didn't read your last phrase about the setAdapter/getAdapter.
But then I'm -1 on this since non-nullable attributes should be passed through the constructor.

[giosh94mhz]

@MattKetmo maybe you're right. Since this is geocoder (not geocoder-bundle, which has DI), using setter/getter imply that the dependency is optional; this doesn't really make sense for remote-providers (I smell "if-null-throw" sanity checks). Do you agree @Baachi ?
So, I also 👍 to AbstractProvider + AbstractHttpProvider extends AbstractProvider.

[Baachi]

I would prefer to throw an exception, if getLimit returns a value lower than 0.

[Baachi]

@giosh94mhz Yeah i agree with you. So lets create an AbstractHttpProvider if @willdurand agrees.

[willdurand]

Ok so I finished most of the work here, but some tests are failing because we have to rewrite them. I am going to merge it right now.

Then, @Baachi @toin0u I'd love your help on fixing the test suite. It is a matter of rewriting test cases, because results were array-based, and now we have an Address class.

[willdurand]

Anyone willing to contribute the AbstractHttpProvider class?