Refactor providers to leverage ResultFactories - fix #232 #233

toin0u · 2013-05-14T21:46:32Z

willdurand · 2013-05-14T22:01:07Z

Looks good to me.

One question though, should I consider this as a BC break? Or people don't use providers in a standalone way?
Depending on the answer, it may imply a new major release.

toin0u · 2013-05-15T06:44:32Z

When all providers will be rewritten, they will return all results so DefaultResultFactory with returns the first one and MultilpleResultFactory all of them. It will not be a BC break because the DefaultResultFactory is the one by default. But I guess it can be a major release anyway :)

willdurand · 2013-05-15T07:19:32Z

well, I won't bump a new major release just for fun. I was talking about using providers without the result factories, which could be a BC break in that case.

toin0u · 2013-05-15T08:28:13Z

Oh yes I see now. I never used providers without factories. Well, in this case, it will be a BC break !

toin0u · 2013-05-20T16:05:04Z

@willdurand @baashi All providers are rewitten (except YahooProvider see #230). Tests are also updated.

I just ran the test suite. Some tests are skipped or incomplete:

GeocoderUsProvider: Timeout too long
GeoipProvider: You have to install GeoIP (see conflict issue here https://github.com/maxromanovsky/php-maxmind-geoip/issues/5#issuecomment-18153090)
YahooProvider: You need to configure the YAHOO_API_KEY value in phpunit.xml

willdurand · 2013-05-20T21:09:07Z

Alright, so let's remove the Yahoo provider first.

The maxromanovsky/php-maxmind-geoip lib is annoying. If they don't fix their stuff by using function_exists() to define functions only if they don't exist, we will drop the provider that integrates this awful lib.

willdurand · 2013-05-20T21:09:31Z

I think one commit would be enough btw.

toin0u · 2013-05-20T21:14:20Z

I agree it's annoying :(

Ok I'll remove YahooProvider and squash commits.

willdurand · 2013-05-20T21:15:30Z

@toin0u make a commit for the providers, and another one for the Yahoo provider removal

toin0u · 2013-05-20T21:44:22Z

@willdurand Rebased and squashed :) And there is an other PR for Yahoo provider.

toin0u · 2013-05-20T22:08:35Z

Test is fixed by #237

Baachi · 2013-05-21T06:09:54Z

src/Geocoder/Provider/GeonamesProvider.php

-        } else {
+        $data = $json->geonames;
+
+        if (!isset($data) || empty($data)) {


isset isn't needed here.

toin0u · 2013-05-21T07:48:40Z

Thanks for the review :) I'm not able to fix it right now, I'll do it tonight I think.

Baachi · 2013-05-21T09:43:31Z

No problem. Thanks for the PR and your awesome work 👍

toin0u · 2013-05-21T20:42:17Z

Thanks for your comment :) 👍

willdurand · 2013-05-22T09:04:38Z

Looks good to me. WDYT?

stof · 2013-05-22T09:11:13Z

src/Geocoder/Provider/TomTomProvider.php

+     *
+     * @return array
+     */
+    protected function getResultArray(\Traversable $data)


The typehint looks weird. You are never traversing the data (so Traversable on it is useless) but you are accessing properties (not part of the interface)

Mmm you're right. So do you recommend to not use any typehint here ?

I would typehint \SimpleXmlElement

btw @willdurand, is it intended to use \SimpleXmlElement in some providers and DOMDocument in some others ?

No it's not. Just a matter of choice from many contributors.

@stof Thanks I'll update this.

toin0u · 2013-05-22T09:34:00Z

I'll make the last updates suggested by @stof and @Baachi asap :)

toin0u · 2013-05-22T15:52:42Z

It should be ok now :)

willdurand · 2013-05-22T15:56:56Z

What are the 13 skipped tests and the incomplete one?

toin0u · 2013-05-22T16:16:38Z

It's like this comment #233 (comment)

Maybe you want me to rebase ? I can do it but it will be little bit later.

willdurand · 2013-05-22T17:06:20Z

No. I want to know if the maxmind-lib issue is fixed.

toin0u · 2013-05-22T21:04:57Z

I understand.

Just for info, I ran the test suite with the last version of php-maxming-geoip which removed the autoloading from composer https://github.com/maxromanovsky/php-maxmind-geoip/issues/5#issuecomment-18193859

GeocoderUsProvider: timeout too long
MaxMindBinaryProvider: skipped because functions geoip_open and/or GeoIP_record_by_addr are not existing.
YahooProvider

willdurand · 2013-05-22T21:11:14Z

this is annoying. I'm thinking about removing this lib..

toin0u · 2013-05-22T21:16:11Z

I understand..

toin0u · 2013-05-22T21:34:42Z

Mmm the problem is not only with redeclaring existing functions but also the set of define statements in geoip.inc file... https://github.com/maxromanovsky/php-maxmind-geoip/blob/master/geoip.inc#L23-L71

The origin of the lib is here I guess: https://github.com/maxmind/geoip-api-php

willdurand · 2013-05-23T22:52:23Z

IMO we a need a better lib to include this provider.

+1000

toin0u · 2013-05-24T16:23:40Z

@willdurand :)
@Baachi @makasim What do you guys think about that ?

makasim · 2013-05-25T03:51:15Z

I hoped it could be posible to keep their "not so good code" out of Geocoder. As I see it is not true.

I would try to open issues on maxmind official repo. I would even do some PRs with fixes If they be really active and positve on required changes (function_exist\defined const checks or maybe renaming functions). I dont think I will be able to do it till monday.

toin0u · 2013-05-26T21:16:51Z

@makasim I understand you as well.. I think your provider is good but the library which your provider depends on should be improved to avoid problems. I'm pretty sure that @willdurand @Baachi agree with me. Maybe you can keep us in touch about your PRs and so on..

makasim · 2013-05-27T04:56:24Z

Maybe you can keep us in touch about your PRs and so on.

sure

Baachi · 2013-05-27T06:08:49Z

Why we don't use the offical library?

makasim · 2013-05-27T06:15:15Z

Why we don't use the offical library?

I haven't found it

Baachi · 2013-05-27T06:31:21Z

I prefer the offical driver. I found this driver http://maxmind.github.io/GeoIP2-php/ . The driver looks really good, he supports the webclient and local database. But the driver is for geoIP2, i don't know if there is a big difference between geoIP and geoIP2.

makasim · 2013-05-27T07:06:56Z

I prefer the offical driver. I found this driver http://maxmind.github.io/GeoIP2-php/ . The driver looks really good, he supports the webclient and local database. But the driver is for geoIP2, i don't know if there is a big difference between geoIP and geoIP2.

Indeed it looks much more better, but as stated on this page they do not add support of dowloadable databases, they dont even published them.

In the future, this distribution will also provide the same API for the GeoIP2 downloadable databases. These databases have not yet been released as a downloadable product.

Downloadable databases for GeoIP2 are not yet available. However, when they are, they will use our new MaxMind DB format. Once the databases are released, we will provide additional details on their use on this dev site.

makasim · 2013-05-27T07:31:10Z

Apperantly GeoipProvider and MaxMindBinaryProvider are both use maxmind's database as source. I would prefer to go with MaxMindBinaryProvider, since the extension is not supported any more (since 2011). As the extension is not updated more then a year it could not match the php lib of current version.

This should be done:

Remove GeoipProvider
Add composer support to official lib version one 1 for now add composer support. maxmind/geoip-api-php#5
add a conflicts note to composer official lib conflicts: ext-geoip

makasim · 2013-05-27T07:31:39Z

http://pecl.php.net/package/geoip

Baachi · 2013-05-27T07:44:20Z

Ah i don't saw these sentences, sorry.
But i see that this library is currently alpha, include this library can cause some issues. So we should not require this library.
And yes we should drop the support for the pecl extensions and use the official library instead.

Remove the GeoIPProvider is a BC break, we can only mark it as deprecated.

@willdurand WDYT?

willdurand · 2013-05-27T07:50:42Z

it's ok to remove it as of 2.0.0.

havvg · 2013-05-27T12:35:00Z

src/Geocoder/Provider/ArcGISOnlineProvider.php

@@ -20,6 +20,11 @@
 class ArcGISOnlineProvider extends AbstractProvider implements ProviderInterface
 {
    /**
+     * @var integer
+     */
+    const MAX_RESULTS = 5;


Is there a special reason why this value is immutable (for this and other providers)? Should be a mutable property, no?

@havvg There isn't a special reason. I just thought it's "enough" to have 5 results as maximum.. We can make this property mutable if everybody agree.

willdurand · 2013-05-27T12:45:19Z

Maybe it could be a new argument in the geocoder() method, WDYT?

toin0u · 2013-05-27T13:05:58Z

I agree - it's indeed the best place even if some providers don't return many results.

havvg · 2013-05-27T13:55:51Z

Hmm.. I'm not sure on this. It would require adding a parameter to the ProviderInterface::getGeocodedData and ProviderInterface::getReversedData for a behavior that is not supported by all providers. So I would rather go for a simple setter/getter (should be distinguished by an interface) and maybe a Geocoder::limit(?) method wrapping it if available:

$geocoder = new \Geocoder\Geocoder($provider);
// add some providers ..

$results = $geocoder
    ->using('bing_maps')
    ->limit(5)
    ->reverse([ 48.8630462, 2.3882487 ])
;

toin0u · 2013-05-27T14:56:15Z

@willdurand @havvg I think we almost agree ;) Maybe Geocoder::limit is better than Geocoder::setMaxResults though..

makasim · 2013-05-28T05:36:01Z

The PR with composer support was merged, it autoload's files (as it was in maxromanovsky's lib) but it also has conflict section with ext-geoip: "*". I think I should do the changes in a separate PR? v1?

willdurand · 2013-05-28T07:22:07Z

@makasim actually the lib should not conflict. It would be easy to declare functions/constants only if they don't exist.

makasim · 2013-05-28T09:14:18Z

@makasim actually the lib should not conflict. It would be easy to declare functions/constants only if they don't exist.

no it is not easy. The extension is not up to date and could be differ (last updated 2011) from current version of the lib (v1.13). So we cannot rely on the extension in all cases. Also if we declare function/constans check it would silently fallback to use the extension and if there is an error a developer would have to spent more time to investigate it. I dont see nothing against marking the ext as conflict. It is not official ext and it does seem like maintened.

willdurand · 2013-05-28T09:46:41Z

@makasim ok, I created a 1.7.0 version with a new dependency to geoip/geoip. We won't remove any MaxMind provider by now.

Refactor providers to leverage ResultFactories - fix #232

willdurand · 2013-05-28T10:03:18Z

So I merged this PR into the master (which is 2.0.0-dev). Let's see how to set the limit now, and we will ready for a 2.0.0 release.

willdurand · 2013-05-28T10:03:29Z

oh and thank you @toin0u for your awesome work!

toin0u · 2013-05-28T12:39:37Z

You're welcome!

Baachi reviewed May 21, 2013
View reviewed changes

stof reviewed May 22, 2013
View reviewed changes

havvg reviewed May 27, 2013
View reviewed changes

Refactor providers to leverage ResultFactories - fix geocoder-php#232

7dadc1c

willdurand added a commit that referenced this pull request May 28, 2013

Merge pull request #233 from toin0u/RefactorProvidersResultFactories

918776d

Refactor providers to leverage ResultFactories - fix #232

willdurand merged commit 7dadc1c into geocoder-php:master May 28, 2013

toin0u deleted the RefactorProvidersResultFactories branch May 28, 2013 12:46

Baachi mentioned this pull request May 29, 2013

Call to undefined method Geocoder\Provider\ChainProvider::setMaxResults() #241

Closed

Refactor providers to leverage ResultFactories - fix #232 #233

Refactor providers to leverage ResultFactories - fix #232 #233

Uh oh!

Conversation

toin0u commented May 14, 2013

Uh oh!

willdurand commented May 14, 2013

Uh oh!

toin0u commented May 15, 2013

Uh oh!

willdurand commented May 15, 2013

Uh oh!

toin0u commented May 15, 2013

Uh oh!

toin0u commented May 20, 2013

Uh oh!

willdurand commented May 20, 2013

Uh oh!

willdurand commented May 20, 2013

Uh oh!

toin0u commented May 20, 2013

Uh oh!

willdurand commented May 20, 2013

Uh oh!

toin0u commented May 20, 2013

Uh oh!

toin0u commented May 20, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toin0u commented May 21, 2013

Uh oh!

Baachi commented May 21, 2013

Uh oh!

toin0u commented May 21, 2013

Uh oh!

willdurand commented May 22, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toin0u commented May 22, 2013

Uh oh!

toin0u commented May 22, 2013

Uh oh!

willdurand commented May 22, 2013

Uh oh!

toin0u commented May 22, 2013

Uh oh!

willdurand commented May 22, 2013

Uh oh!

toin0u commented May 22, 2013

Uh oh!

willdurand commented May 22, 2013

Uh oh!

toin0u commented May 22, 2013

Uh oh!

toin0u commented May 22, 2013

Uh oh!

willdurand commented May 23, 2013

Uh oh!

toin0u commented May 24, 2013

Uh oh!

makasim commented May 25, 2013

Uh oh!

toin0u commented May 26, 2013

Uh oh!

makasim commented May 27, 2013

Uh oh!

Baachi commented May 27, 2013

Uh oh!

makasim commented May 27, 2013

Uh oh!