Port GeoIP API to D7

Thanks for posting this. I've got a couple of small tweaks I want to make before I fork it for DRUPAL-6--1. Then HEAD will be all about D7.

Subscribe

Note that Drupal 7 core includes a function to return an array of countries. Call the following code:

include_once DRUPAL_ROOT . '/includes/locale.inc';
$countries = country_get_list();

It makes sense to use this data in Drupal 7 rather than have geoip.values.inc.

Note that this array will need to have "Anonymous Proxy" and "Satellite Provider" added.

Subscribing...

@das-peter - Excellent. Would you be able to post your work as a patch so that this can become the official D7 branch?

I only got half way through this but here's what I came across.

I'd like to make sure this gets applied to 6.x:

 function geoip_ip_address() {
-  if (variable_get('geoip_debug', FALSE) && $_GET['geoip_debug']) {
+  if (variable_get('geoip_debug', FALSE) && isset($_GET['geoip_debug'])) {
 

Typo/I'm not sure that's the Drupal way of specifying types:

+ * @return ressource|false

Incorrect indenting on the initial * and we don't declare @return if there's no value:

+*   @return void

This doesn't look like the Doxygen standards unless they've changed them for D7:

+ *
+ * @return geoiprecord|false
+ *  City name
  */

Comments need to be complete sentences. Also with is misspelled.

 /**
- * Return a list of country codes as specified by http://www.maxmind.com/app/iso3166
+ * Return a list of country codes
+ *
+ * Codes as specified by http://www.maxmind.com/app/iso3166
+ *
+ * @return array
+ *  Associative array wit the country code as key, and the country name as value
  */

Have we checked to see which labels have changed now that _geoip_country_values() is calling country_get_list()? We'll need to document it if any have.

geoip_language_mappings() Should be using the new drupal_static() rather than its own static variable.

I'll give this a longer review this afternoon. Lets put the token stuff into another feature because I'd like to see if it's feasible to backport it to the D6 branch.

Gently pointing everyone to: http://api.drupal.org/api/function/hook_countries_alter/7

...subscribing. Can we get these (#11 and #12) in a dev release please? That way we'll be getting update notifications through the sites' 'Available updates' section when there's something new to test. Thanx in advance ;)

Looking forward to a dev release.

...patching against latest 6.x dev 29-Oct-2010:

patching file geoip.admin.inc
patching file geoip.info
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file geoip.info.rej
patching file geoip.install
patching file geoip.module
patching file geoip.rules.inc
patching file geoip.tokens.inc
patching file geoip.values.inc
patching file geoip_language/geoip_language.admin.inc
patching file geoip_language/geoip_language.fastpath.inc
patching file geoip_language/geoip_language.info
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file geoip_language/geoip_language.info.rej
patching file geoip_language/geoip_language.install
patching file geoip_language/geoip_language.module
patching file geoip_language/geoip_language.test
patching file lib/geoipcity.inc

So I went on and patched it manually (the two failing files actually). Just thought I'd let you know. Will report back after testing.

...here it is for those testers out there that have trouble patching (rename from .tar_.gz to .tar.gz):

Patch from #17 updated to apply to current HEAD.

In the patch,

+  // MaxMind specific shortcuts
+  $countries += array(
+    'A1' => 'Anonymous Proxy',
+    'A2' => 'Satellite Provider',
+    'AP' => 'Asia/Pacific Region',
+    'EU' => 'Europe',
+    'O1' => 'Other Country',

The names should be wrapped in t(). That's what country_get_list() does for all the pre-defined country names.

Good point, thanks.

...again, here is the patched module for those testers out there that have trouble patching (rename from .tar_.gz to .tar.gz):

Note: the patch has a couple of hiccups when applied either against the latest 6.x-1.x-dev (2011-Feb-17) or against 6.x-1.3:

patching file geoip.admin.inc
patching file geoip.info
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file geoip.info.rej
patching file geoip.install
patching file geoip.module
patching file geoip.values.inc
patching file geoip_language/geoip_language.admin.inc
patching file geoip_language/geoip_language.fastpath.inc
patching file geoip_language/geoip_language.info
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file geoip_language/geoip_language.info.rej
patching file geoip_language/geoip_language.install
patching file geoip_language/geoip_language.module
patching file geoip_language/geoip_language.test
patching file lib/geoipcity.inc

...so I patched geoip.info and geoip_language.info manually.

sub

Including the module name makes it obvious which issue is on your dashboard :)

re-rolled the patch. i've created a 7.x branch that we can commit this to once it's sorted.

I committed a bunch of the simple stuff to the 7.x branch. Here's the more complicated stuff that I want to be able review closely.

...proper version then ;)

subbing

subscribe

Subscribe. I will potentially need something like this in D7 for an upcoming project, so if it all works out, I would be happy to help where needed on the D7 version.

So to get this to a full release we need to verify that the patch applies to the git head, run it through coder and see that it's ok, what else?

@drewish thanks for the patch in March. I haven't used this module yet, but would like to soon.

subscribing.

subscribing

Hi,

I tested the 7.x-1.x-dev branch and it works fine for me.

The 7.x dev has been around since past March and usage stats report ~25 sites using it. I guess what remains to be done is make the 7.x-1.x-dev available in the project's front page and we should close this issue here. This was about an initial port and now we can file other specific problems in their own separate issues.

Agree with klonos, #37. Please make a dev or alpha version available, so that it's more obvious that a decent D7 version exists.

I'm using the "hidden" 7.x.dev version of GeoIP API in combination with IP Geolocation and it works wonderfully. So it would be great if we can make the synergy between these two modules more apparent for the community at large.

Sorry for re-opening, but there still is no 7.x version available in the project's front page and no statement that there is a D7 port in the project's description either.

I created new geoip_language module, based on quova_locale. It is using hook_language_negotiation_info().
It is just a starter and yet to be reworked or improved - language mapping must be added. Maybe it will help someone.
Also had to change geoip_country_code() function a little.

In order for it to work place it in the second place after URL in "Detection and Selection". Also English must have a prefix - 'en'. So when there is no prefix in url it will fallback to the IP detection.

Patch applies to the current 7.x-1.x release.

Improved patch from #41, added mapping, also no changes needed to the geoip_country_code() function.

@das-peter: pushed to the sandbox, i hope i didn't break something - 1st time committing something to Drupal. :)

Patch applies to the current 7.x-1.x release.

subscribing, thanks for your good work!

Changing this to needs review for #43. @andyhu & @das-peter does it work?

@drewish / @zroger - could one of you get the 7.x dev release on the project page? This was by far the best module for my use case, but almost missed it as it didn't appear any 7.x work was happening. I've put the current dev release in place and it works perfectly (as also confirmed by #37 and #38 above).

There are a few D6 cleanup items (ie admin menu change) remaining, but we can get patches going for that.

I'm marking this as RTBC even though I know #43 hasn't been resolved, but IMO we first need to get 7.x up and then can start filing patches like that against it separately - otherwise we'll never have a 7.x release in the first place and this thread will get overrun.

The -dev snapshot should now show up. I've got no idea what the status of that branch is though. Is there a patch that needs go on?

Seems odd to be removing the schema stuff for geoip_language_schema... If we're killing off that functionality it seems like it should at least have a cleanup hook_update_N function. Yeah it's actually removing a lot of functionality and tests.

Thanks, drewish! Really great to have that dev branch up there!

the dev version shows the date of the dev version is still 2011-Mar-10 and it's not quite working.

Can someone try to fix the dev branch?

@yang_yi_cn, 7.x-1.x was last updated 16 months ago (march 2010), so that tarball is correct. You can now help get the dev branch fixed by filing detailed reports (or even patches) against 7.x-1.x.

I like others am confused... So no patch has actually been applied to the branch/dev.snap shot since March 2010?
What happened to dukem's patch (#43) ? Can we get that applied to get the ball rolling?

I commented on it already. It needs work. It's dropping a big chunk of functionality.

We were using the schema stuff when was at Sony to direct countries to languages... we were sort of mis-using the languages to act as regions. I think you're right that it's kind of wacky... I'm okay with dropping it for the limitations you'd outlined. If it needs to be in D7 it can get updated.

I do think the JS changes are useful and should be brought forward.

I added a property to the site entity to be able to do data comparisons in rules.

Until now I wasn't able to test the patch on a server. I am developing locally. As soon as I did so, I will provide feedback.

Any update on a stable d7 branch?

Until now the changes I made worked.

@HyperGlide: Please review the patch to get it committed.

@Lukas von Blarer: We have been looking to use drupal.org/project/smart_ip for our needs.

If there is a change I will test and revert back.

The config URL should be admin/config/regional/geoip, both in the module and in the .info file.

Here is a patch for a working version of the geoip_language sub module that allows mappings of IP country names to Drupal internal languages.

Please review the patch and add any changes or comments you think are necessary towards getting a stable release of the module rolled out for Drupal 7.

@zardoc, the changes in your patch, are out of scope. The language negotiation itself, shall do nothing but, get the language. There shall be no automatic redirect by the processor. It would be better to leave that redirect to a dedicated module that uses a generic approach (besides GeoIP, a redirect may be valid for Browser or session too).

Lukas' patch from #58 looks way better, as it focuses on the task.

Needs work, as we should change the URL, as mentioned in #62

Here's a reapplied patch from #58.

@derhasi That's awsome #67 works great so far, no errors on the admin pages at least or through install. I'll be trying this out some more and will report back but otherwise I'd like to say RTBC++

A few years later I repeat my comment in #38... Great news!
Thanks das-peter for all your work!
Rik

@das-peter cool!

I was playing with a custom module to implement MaxMind GeoIP2, xautoload made short work of it and was fun:)

/sites/all/composer.json

{
    "require": {
        "geoip2/geoip2": "~2.0"
    }
}

/**
 * Implements hook_xautoload().
 */
function MODULE_xautoload($api) {
  $api->absolute()->composerDir('sites/all/vendor/composer');
}

function MODULE_get_location($ip_address) {
  $result = array();
  try {
    $client = new GeoIp2\WebService\Client(123, 'ABC');
    $record = $client->city($ip_address);
    $result['country']      = $record->country->name;
    $result['country_code'] = $record->country->isoCode;
    $result['region']       = $record->mostSpecificSubdivision->name;
    $result['region_code']  = $record->mostSpecificSubdivision->isoCode;
    $result['city']         = $record->city->name;
    $result['zip']          = $record->postal->code;
    $result['latitude']     = $record->location->latitude;
    $result['longitude']    = $record->location->longitude;
    $result['time_zone']    = $record->location->timeZone;
    // $result['metro_code']   = $record->location->metroCode;
    // $result['area_code']    = $record->...;
    // $result['isp_name']     = $record->traits->isp;
    // $result['organization_name'] = $record->organization;
  } 
  catch (Exception $e) {
    // 
  }
}

Just mostly a demo of how xautoload and composer made short work of managing the dependent OOP libraries.

There are two D7 branches, safe to say a port was made :)

Might be worth switching up the recommended releases for D7 and the branches on the project page?