@joostdekeijzer opened this Pull Request on November 1st 2014

I think this resolves #3581.

Am I correct to see that only the GeoIp\Php class needs modification? GeoIP\Pecl still doesn't seem to work with IPv6 and GeoIP\ServerBased seems to handle IPv6 already?

Running console tests:run --file tests/PHPUnit/System/ManyVisitorsOneWebsiteTest.php fails on some assertions (not mine). For now, I kept those as they are.

@joostdekeijzer commented on November 1st 2014

Oops, did not investigate failed tests good enough: they are the result of my coding.
Sorry, working on improved PR...

@joostdekeijzer commented on November 1st 2014

Updated PR

  • bugfix :-/
  • updated expected test results: IPv6 visits were previously "unknown", current test IPv6 addres is US.
@mattab commented on November 3rd 2014 Owner

Kuddos for the Pull request adding this awesome new feature!

GeoIP\Pecl still doesn't seem to work with IPv6

good point, I've posted a comment in the PECL feature request, hopefully they add it sometime soon :) https://bugs.php.net/bug.php?id=59124

GeoIP\ServerBased seems to handle IPv6 already

wondering if you had a chance to test it?

Glad to see you added a new test too! as you can see some test are failing in "AutoSuggest" system test which is the test that generate values for the "Segment value suggest dropdown". because now the visitor have a location some test fail. if you can't fix those it's ok i can do it.

I put some other feedback inline. Cheers

@joostdekeijzer commented on November 4th 2014

Fixed the tests.

I'm afraid I can't test the ServerModule

About ISP and ORG: isn't this already tested with the MockLocationProvider?

@mattab commented on November 5th 2014 Owner

About ISP and ORG: isn't this already tested with the MockLocationProvider?

Maybe (didn't look) but it would not beat a test with actual database files :-)

@mattab commented on November 7th 2014 Owner

I can merge it on Monday for our next beta - will be an extra treat for 2.9.0! or we'll make it in 2.10.0 otherwise which works well too

@mattab commented on November 9th 2014 Owner

@joostdekeijzer kuddos for this great PR! We hope you will consider again contributing to Piwik :+1:

@mattab commented on November 13th 2014 Owner

We had to revert the Pull request as it was not working on PHP 5.3

@joostdekeijzer see #6647 #6636

@joostdekeijzer commented on November 13th 2014

That's to bad.

Could it be the problem is not PHP5.3 but the availability of the GeoIP PECL extension?

@joostdekeijzer commented on November 13th 2014

@mattab @tsteur It's not PHP5.3 (travis tests with 5.3) but the availability of the GeoIP PECL extension.

I've updated my original branch https://github.com/joostdekeijzer/piwik/tree/3581_IPv6_support with some function_exists and defined checks and tested the code with the PECL extension installed (no errors).

I can create a new PR if you like (I don't think it's possible to revive this one).

@tsteur commented on November 13th 2014 Owner

Yes, I do geoip extension version => 1.0.7. I reckon a new PR sounds good @mattab ?

@mattab commented on November 13th 2014 Owner

:+1:

This Pull Request was closed on November 9th 2014
Powered by GitHub Issue Mirror