@sgiehl opened this Pull Request on September 10th 2017 Member

Our geoip library wasn't updated a long time. Meanwhile it is also available using composer. So why don't use that one :)

If we merge this, we should also update our build script to exclude all files besides those in src directory of the library

@mattab commented on September 18th 2017 Owner

Feedback:

  • In the commits there are big arrays of values which are not available in the latest version. Is this expected that these arrays of Region names, etc. are not anymore in the library?

  • maybe i'm missing something but not seeing the function geoip_country_code_by_addr_v6 and the other geoip_* functions in the library anymore, although we use them in core.

  • I don't think our tests cover all of GeoIP use cases, right? likely we need to manually test carefully the change
@sgiehl commented on September 20th 2017 Member
  • In the commits there are big arrays of values which are not available in the latest version. Is this expected that these arrays of Region names, etc. are not anymore in the library?
  • maybe i'm missing something but not seeing the function geoip_country_code_by_addrv6 and the other geoip* functions in the library anymore, although we use them in core.
  • I don't think our tests cover all of GeoIP use cases, right? likely we need to manually test carefully the change

The library is only removed from libs. running a composer install should install it in vendor. And there are those geoip methods and big arrays still included.

@sgiehl commented on October 12th 2017 Member

I'll do a rebase of this branch to check if tests still passes. But imho it should be save to merge

Powered by GitHub Issue Mirror