For instance:
Fatal error: Cannot redeclare geoip_country_code_by_name() in [...]/sites/all/modules/geoip/lib/geoip.inc on line 376

This happens when the geoip PECL extension is enabled on the server.

GeoIP API currently only supports the pure PHP lib that comes with the module and not the geoip PECL extension.
This is not stated on the project page nor in the INSTALL.txt.

Comments

Cool, I had no idea there was a pecl extension. Ideally we'd make them compatible but they seem to have different enough interfaces that it might be tricky. We should document the incompatibility and probably do a warning in hook_requirements.

By the way, I think that the pure PHP API (http://www.maxmind.com/download/geoip/api/php/) should not ship with the module (it is a 3rd party library) but be downloaded separately by user into sites/all/libraries.
This way, user could choose between using the pure PHP API or the PECL extension.

Including their library doesn't preclude supporting the pecl library. It's an include that could be conditional. But I think you must have missed the part about them being totally different APIs. I'm open to patches that would add support but that would really be a feature request. The real bug here is the undocumented incompatibility.

Sorry, I know this is a different issue. I was just too lazy to open a separate one :-)

The ad_geoip module (http://drupal.org/project/ad_geoip) uses the PECL extension.

Version:6.x-1.x-dev» 6.x-1.4
Priority:Major» Critical

When you use PHP's geoip extension, there is no need to use this Drupal module. Thus I have removed the module from our server. Completely breaking all sites that have this module enabled is quite critical.

StatusFileSize
new4.63 KB

I created a small patch to allow usage of geoip even if the geoip-extension is loaded. Note this is only a workaround to get this running. The geoip-extension should be used by geoip if possible and the PHP-implementation should only be a fallback.

Even with the geoip-extension I see the need for this module, as it allows to integrate geoip better into Drupal. Not sure how far the current implementation goas here, but I like having a module to define the API Drupal uses...

In case others are looking for anrikun's request to remove the library from the code, please see #1159090: Lib files should be removed from the module

With regards to adding compatibility with the geoip php extension, I've attached a patch that should accomplish this functionality. in the attached patch, if the geoip extension is installed, then it'll use that extension, otherwise it'll load the MaxMind geoip-api-php library and use it's calls.

This was written for the D7 version as patched in issue #887268: Port GeoIP API to D7 and then backported so there are three patch files included:

  • Compatibility_with_geoip_pecl_extension_d6-945852-8.patch - This should apply against the current D6 branch. This is untested.
  • Compatibility_with_geoip_pecl_extension_d7-945852-8.patch - This should apply against the current D7 branch. Since the D7 branch currently doesn't function, this patch is pretty useless and is untested.
  • Compatibility_with_geoip_pecl_extension_d7_patched-945852-8.patch - This should apply against the current D7 branch after applying the latest patch from #887268: Port GeoIP API to D7. This is tested and is working from what I can tell.

Status:Active» Needs review

Marking the ticket as "Needs review"