Use Countries module to improve geocoding results

unfortunately this no longer applies

I manually applied the country part of the patch and it solved my problem with El Salvador showing up in Switzerland. =^)

Rerolled the patch, works like a charm.

I can confirm that this patch successfully uses the countries module to distinguish between countries.

If I understand correctly, with this patch, the country is added two times to the query (one using the current code, one at the end coming from the patch). Am I right?

@Simon Georges I believe you are right regarding #9.

Maybe it should replace it? @ptmkenny or @nagy.balint

I stumbled upon this issue because it looks like I am working on the same project that led @michaelmol to open this issue. The patch applied in that project has evolved beyond the version in this ticket to solve a few more problems we ran into, basically to solve the problem noted by Simon Georges. I'm attaching that version here.

Also, I am planning to improve integration between Countries module and Geocoder in another way; it is currently not possible to have a location field on a country representing the location of that country (at least, while using Geocoder to automatically fill that field). Since that really also constitutes "Integration with Countries module" (the title of this issue), but really is in a completely different area, I guess I'll rename this issue to "Use Countries module to improve goecoding results" and open a new "Allow geocoded field on Countries entity" issue. Edit: Done, see #2376447: Allow geocoded field on Countries entity.

I like the solution of this last patch, less intrusive. I'm just waiting for some people to confirm it works before committing. Thanks!

I've taken another look at this... the patch works. But, the functionality overlaps with #2245189: Make address field country unambiguous. Difference being that this needs countries module to be installed. 2245189 just uses the country list which is also used in the installer and is part of Drupal core. We could include both because the countries module is bound to be more up to date than core /includes/iso.inc.

I've included a patch which implements both.

I added another patch because using the official country name confuses the Google geocoder.
The new patch uses the name instead of the official name.

===
"Luxembourg" was passed as "Grand Duchy de Luxembourg" (and leads to a result in downtown Los Angeles)
"France" was passed as "French Republic" (and leads to the french embassy in Budapest, Hungary)
"Netherlands" was passed as "Kingdom of the Netherlands" (and leads to a place near Almaty, Kazakhstan)

This looks good:

#17 looks great!

+++ b/geocoder.widget.inc
@@ -415,7 +414,21 @@ function geocoder_widget_parse_addressfield($field_item) {
+      $field_item['country'] = $country->official;

Isn't this 'official_name' property not 'official'?

Bork, my bad. It should be $country->name, not $country->official_name nor $country->official. Good catch.

Hi all,

Instead of using 'if (function_exists('country_load')) {', shouldn't we use entity_load('country', ... ) and remove the if condition ?
We could try to find the results in cascade... first using entity_load('country', ... ) then using the default Drupal system ?

What do you think ?

Let me know what you think of this patch.

The patch looks great though I'd really advise against using a private function(denoted by the _), thought it's likely not going to change it's not part of the API. Can you switch it back to country_get_list()?

To be more clear: the fallback should be locale.inc + country_get_list().

+++ b/geocoder.widget.inc
@@ -473,26 +473,32 @@ function geocoder_widget_values_from_geometry($geometry, $target_info) {
-    include_once DRUPAL_ROOT . '/includes/locale.inc';
-    $countries = country_get_list();
...
+      include_once DRUPAL_ROOT . '/includes/iso.inc';
+      $countries = _country_get_predefined_list();

It gets the countries alter hook and is the proper API to be calling instead of the private function in iso

You're absolutely right.

Can you let me know when you've tested this patch (especially with the entity_load() stuff) so I can commit it ?

Thanks!

Thanks @Pol for changing that up.

Well one thing that get's lost in translation with the removal of country_load() is that it uppercase the ISO code for comparison to normalize it using drupal_strtoupper() which is a UTF safe string uppercase(which is a bit overkill if we expect a 2 character iso code.

Even though I like the idea, if countries is not installed it will be much slower to do a SQL/cache query with no results than it would to do a function_exists() so I'm back peddling on that idea. The string to array clean up is still good in my books though. Does that logic seem correct to you @Pol?

I don't know I would prefer cleaner code even if it's a bit slower... So, what do you think we should do ?

I really think we should use the function_exists() or module_exists() code. I'm bias as I'm big on performance but also I think it's cleaner because it's clear what the dependency is on that call being made. Sorry should have looked harder in #22

Ok let's do this then.

Ok it's done, I just replace function_exists() with module_exists().

Awesome, thanks @Pol