Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use composer version of GeoIP library #12021

Merged
merged 2 commits into from Nov 19, 2017
Merged

Use composer version of GeoIP library #12021

merged 2 commits into from Nov 19, 2017

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Sep 10, 2017

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

@sgiehl sgiehl added c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Needs Review PRs that need a code review labels Sep 10, 2017
@sgiehl sgiehl added this to the 3.2.0 milestone Sep 10, 2017
@sgiehl sgiehl force-pushed the maxmindgeoip branch 2 times, most recently from 87654ca to 38de70c Compare September 11, 2017 15:19
@mattab
Copy link
Member

mattab commented Sep 18, 2017

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
Copy link
Member Author

sgiehl commented Sep 20, 2017

  • 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

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.

@mattab mattab modified the milestones: 3.3.0, 3.2.1 Oct 10, 2017
@sgiehl
Copy link
Member Author

sgiehl commented Oct 12, 2017

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

@mattab mattab merged commit 23249e9 into 3.x-dev Nov 19, 2017
@mattab mattab deleted the maxmindgeoip branch November 19, 2017 21:52
@sgiehl sgiehl restored the maxmindgeoip branch November 21, 2017 18:24
@sgiehl sgiehl deleted the maxmindgeoip branch November 21, 2017 18:25
sgiehl added a commit that referenced this pull request Nov 21, 2017
sgiehl added a commit that referenced this pull request Nov 22, 2017
@matomo-org matomo-org deleted a comment from MatomoForumBot Dec 4, 2017
@mattab mattab added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Dec 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants