I hate to start a new issue on google.inc, but looking at the others I didn't see one that really related and I didn't want to hijack another issue.

Attached is a patch that alters the google_geocode_country_list() function to generate the available countries array from the google spreadsheet via its feed. (Another way to do it would be via javascript and the google maps api v3, but I don't know javascript so... php it is.)

Since the a few of names generated by the Drupal API and those from Google don't match, there are a couple of hacks to make them line up properly. Those may need some tweaking down the road if Google or Drupal adds/changes names that don't match properly.

Also, the function requires an external feed parser that I found here and I've attached that file for convenience. It should be placed into the geocoding folder alongside google.inc.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kvvnn’s picture

So, should this solve issues like a city in Thailand not geocoding, when it would geocode for Google Maps?

fred0’s picture

kvnn,

I just tested an address in Bangkok and it was geocoded properly so, I guess the answer is yes.
This patch does not, however, add map links. Those are handled by different include files. All this should do is enable geocoding.

kvvnn’s picture

What exactly are "map links"? The link to a google map (maps.google.com)?

If it geocodes, then we can display it on a Location Gmap, right?

fred0’s picture

Correct on both.

kvvnn’s picture

Thanks fred0, you're the man for posting this. Seems like a rather big step forward to me.

kvvnn’s picture

The patch did not work for me, but I copied / deleted the (+) / (-) lines and its working great.

Thanks!

kvvnn’s picture

This still does not geocode some cities that Google Maps will provide coordinates for, but its much better than the previous logic, and works with many cities throughout the world.

I'm wondering if Google Maps API Premiere is required for geocoding of every city. I'll be in contact with Google, and will report back.

kvvnn’s picture

None of Africa geocodes
None of Central America (except Mexico) geocodes
Greenland / Iceland don't geocode
None of Carribeans geocode

How do we get these to geocode? If I present a spreadsheet of every city not currently supported, can we implement it somehow?

Summit’s picture

Hi,
Non on South Afrika original, it is in this way@
EDIT: Thank you for this improvement I wanted to say!
greetings, Martijn

kvvnn’s picture

What?

nickl’s picture

Status: Active » Needs review
FileSize
6.12 KB

Patch didn't apply and had to be re-rolled.

Removed FeedParser dependency and fixed coding standards.

nickl’s picture

Title: Patch to get available countries from google's spreadsheet » auto load geocode country list
Priority: Normal » Critical

Changing title and priority, manualy updating the list leaves many countries without geocoding support.

fred0’s picture

FileSize
402 bytes
5.86 KB

Just found that the entry for "East Timor" in location.inc is incorrect per the ISO spec: http://en.wikipedia.org/wiki/ISO_3166-1_alpha-2#Officially_assigned_code...

Since it should be "Timor-Leste" we can change it in the ISO list and remove that item as a fix in google.inc.

New patches attached.

nickl’s picture

FileSize
6.64 KB

Attached patch rerolled against head.

Don't know any locations in Timor-Leste so patch not tested.

YesCT’s picture

Issue tags: +location geo-coding

what is needed to get this to work? Just the patch in #14, or "function requires an external feed parser" in the original issue description?

Is there any concern with including external code with a drupal module? things need to have the right license... see the note http://drupal.org/node/59 about GPL

is there any reason to not auto load geocode country list? Would committing this break anyone's existing site? If so, this might have to be added with a module setting "Get country list from geocoding..." What would be a good setting name for this?

What information do you think the maintainer needs to know before committing this? (it's really hard to be a maintainer, lets make it as easy on them as possible)

If someone wants to help and has not reviewed a patch before:
How to review a patch: http://drupal.org/patch/review
Can someone do like a code formatting review? http://drupal.org/coding-standards
Can someone please try this with the coder module: http://drupal.org/project/coder

And... pretend you were the location maintainer... what would you put in the log if this were committed, what would you add to the change log? http://drupal.org/project/cvs/18723

fred0’s picture

Patch 11 removed the external dependency and we've been rolling based on that since so, just the patch in 14 should be all you need.

YesCT’s picture

fred0: Thanks for clarifying! It's hard to follow these sometimes, and a summary like that makes it a lot easier to quickly see what is going on.

What about this question from #15? is there any reason to not auto load geocode country list? Would committing this break anyone's existing site? If so, this might have to be added with a module setting "Get country list from geocoding..." What would be a good setting name for this?

fred0’s picture

The only reason I can think of is if one were running Drupal on an intranet or an install with no internet connection. In that case, relying on google would fail.
Beyond that, there is probably a slight slowdown when you visit the settings page as it has to download and parse the entire rss feed every time.

hutch’s picture

Would it be possible to rejig that so that the feed is fetched by a cron, say once a month?

YesCT’s picture

Priority: Critical » Normal

I'm not clear on why this is critical. #12 needs clarification. (I'm pretty sure feature requests aren't http://drupal.org/node/45111)

nickl’s picture

Priority: Normal » Critical
Status: Needs review » Reviewed & tested by the community

Thank you for all your advice and support YesCT, they say great minds think alike...

The page Priority levels of issues says it perfectly and on the first line nogal...

renders ... a popular function unusable.

Even though it does not stipulate "feature" per se, google maps are a) popular and without this patch leaves geocoding b) unusable for many countries when implementing the location module. As per #584014-12: RTBC auto load geocode country list.
Bumping it back to critical (maintainers should read: committing this is high priority!) again.

We've just launched the beta site that implements this patch. I am sure they won't mind, as they are too eager to invest in our community, but alas it is not my place to be making client's beta site details public. The good news is that we've had not one hiccup from this particular patch and since both myself and fred0 can vouch for this (see #584014-16: RTBC auto load geocode country list) I reckon its also RTBC. *high five fred0*

Any maintainers around who would like to comment/commit?

hutch’s picture

Looking at the patch on google.inc (#13) I see there is no fallback position should the http_request fail. Either the original array of keys should be returned or any cached version retained. The latter could be achieved by just returning the keys from the $countries variable as obtained from location_get_iso3166_list()
drupal_http_request() will return success/fail code in $source->code, 200 is success.

In #18 fred0 mentions that it runs on every visit to the settings page, that IMO is a bit rough, either it should run when "Clear supported country list" is run or on a cron, once a month would be plenty, this is not fast changing data.

Other than these issues I think the patch is an excellent idea.

BTW setting this as 'critical' is unjustified, 'critical' applies to situations where the code will not run without the patch, clearly not the case here.

@YesCT I will leave this to your judgement ;-)

YesCT’s picture

Priority: Critical » Normal
Status: Reviewed & tested by the community » Needs work

I'm sticking with "feature requests are never critical". Not that this is not needed, but in terms of the code being broken... I dont think it is. A needed feature, maybe.

Hey, this *might* get committed even if it is not critical. :)
For it to get committed, make sure it is RTBC, and I think the points in #22 need to be addressed.
Also, the maintainers will need a good summary of what was wrong, what the patch does to fix it, and how life is better with the patch.

doing a quick coding style review...

+++ geocoding/google.inc	10 Mar 2010 20:55:14 -0000
@@ -9,76 +9,52 @@
+  // Loop though google data and find all valid entries and make new array

should be complete sentences. (sure the location code is not coding standards compliant, be we can at least add code that is).
http://drupal.org/coding-standards#comment

+++ geocoding/google.inc	10 Mar 2010 20:55:14 -0000
@@ -9,76 +9,52 @@
+    if(strpos($geocoding, "Yes") !== FALSE) {

http://drupal.org/coding-standards#controlstruct
Control statements should have one space between the control keyword and opening parenthesis, to distinguish them from function calls.

+++ geocoding/google.inc	10 Mar 2010 20:55:14 -0000
@@ -9,76 +9,52 @@
+  $countriesfixes = array_merge($cntryclean, array(
+                      "hk"=>"China",
+                      "mo"=>"China",

http://drupal.org/coding-standards#indenting

+++ geocoding/google.inc	10 Mar 2010 20:55:14 -0000
@@ -9,76 +9,52 @@
+  // Note: it may be neccessary to adjust/add to the fixes list in the future if google adds countries that don't match the Drupal API list for whatever reason.

bunch of comments added that are longer than 80 chars.

Powered by Dreditor.

nickl’s picture

Status: Needs work » Needs review
FileSize
6.56 KB

Patch re-rolled against latest DRUPAL-6--3 branch with fixes as pointed out by YesCT.

ankur’s picture

I like this patch and will blindly commit it if we can work around a couple of issues:

(1) What happens if, in the off chance, there is a failure in the network connection? I know someone would have to be a bit dim to see the list missing and click save, but it's a possibility and would potentially cause a loss of data. Is it possible to keep around a cached list of the previously downloaded list? An additional plus (and this may be overkill) would involve including a database update with this patch that pre-caches this list.

(2) This patch makes a correction to adjust hk and mo to be labeled as China. I'm thinking that this might be confusing if we have 'China' show up in the interface 3 times.

If I get the time, I might try to put up a modified version of the patch.

fred0’s picture

As I vaguely allude to above in #18 and hutch references in #22, we could probably use some sort of caching for the list.

As for the China issue, the array_intersect function only returns the matched value once so, duplicates are not an issue:

+  // Compare new google array values to fixed country names values, return matches with keys abbreviations as keys
+  $googlematched = array_intersect($countriesfixes, $regionclean);
cbrody’s picture

Hi, I've applied the patch in #24 and set all relevant countries to use Google Maps for geocoding (to a minimum of Country level), but no geocoding is happening when I select a country, unless I also enter a city or some other finer-grained location data. Any ideas for a fix? I need to be able to geocode on country only.

entilza72’s picture

Title: auto load geocode country list » "Suburb/City" level only results?

Hi team,

Great patch. However, for Japan at least, I appear to be getting only suburb/city level results and would really appreciate some guidance to get down to street level.

You can replicate these results and see the differences:

Correct Behaviour - maps.google.com
Go to the maps.google.com website. Enter "56 Jigatanicho, Nishikyo-ku, Kyoto City, kyoto, japan". If you are on the "map" view, you will see your marker centered on a green "park". This is correct.

Incorrect Behaviour - Location module with patch5 in #24 above
Load up a node (or other item) with "56 Jigatanicho, Nishikyo-ku, Kyoto City, kyoto, japan". View the map. You will see you are placed over 1 mile to the south-east on a building.

Replicated incorrect behaviour in maps.google.com
Go to maps.google.com. Enter "Nishikyo-ku, Kyoto City, kyoto, japan" (we have removed the street and number). This "suburb level" search in maps.google.com gives an identical result to the "incorrect" result from Location with patch.

This leads me to believe Location is only providing Google the suburb level details only. Results can get a lot worse: In one regional area test I did, sometimes the "reach" of the area is so large, the marker can be "incorrect" by the equivalent of a half hour drive.

I have tested this with 4 addresses I know of in Japan. Only 1 is obscure. All 4 fail in the same way.

What I don't understand is: how can this happen? Why can't we just expose the entire address string to google and get the correct result? (I'm presuming this is being done). What is going wrong?

I also wonder how many other countries have a similar problem?

Please feel free to suggest moving this request elsewhere. I wasn't sure where to place it - I only have the opportunity to report this problem thanks to the flexibility the patch gives us! :-D

Cheers,
Jason

entilza72’s picture

Title: auto load geocode country list » auto load geocode country list
UNarmed’s picture

Title: "Suburb/City" level only results? » auto load geocode country list

How would i go about adding South africa to the list so it also gets geocoded?

guillaumev’s picture

FileSize
7.49 KB

Hi,

Here is a patch, based on #24, that adds caching (basically the xml data is downloaded, and then cached. If for some reason the XML can't be downloaded from Google, the cached version is used).

ankur >
(1) This new patch adds the cache, but doesn't include a database update, which I thought wouldn't be necessary...
(2) After testing the patch in #24, I don't see China 3 times, but [China], [Hong Kong SAR, China], [Macao SAR, China]. So I think #24 will NOT display China 3 times

Cheers

erikwebb’s picture

subscribe

I've tested the patch at #31 and it appears to work on the admin screen fine.

erikwebb’s picture

Status: Needs review » Reviewed & tested by the community
Aron Novak’s picture

I can confirnm that the patch in #31 works fine.

nickl’s picture

Title: auto load geocode country list » RTBC auto load geocode country list

I had a quick look at the patch in #31 and it didn't cause any obvious visual parsing errors, its also addressing the google not available issue because of the cache it seems. Unfortunately I don't have the spare time to test it currently but it looks RTBC and I will go with that.
Any reason why we're not committing it, its been RTBC for almost a month already? Are we waiting for better weather or is it due to lovely sunshine that it lags?
Just wondering...

jindustry’s picture

The "Geocoding Options" admin page (/admin/settings/location/geocoding) becomes impractical with the expanded country list if you want to enable geocoding for all countries by default. I just started using the locations module, and I'm relieved to find this patch because I'd like to display maps for addresses all over the world. However, without the ability to enable geocoding by default for all supported countries, the admin must click hundreds of radio buttons (ok I didn't actually count them all) to enable. Could an "enable all" radio button at the top of the table help?

rooby’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for the patch, all seems pretty good to me.

I removed the part about East Timor/Timor-Lemke as that was handled in #670528: East Timor now Timor-Leste and also made a couple of minor coding standards fixes.

Committed to all branches.
http://drupal.org/cvs?commit=473834
http://drupal.org/cvs?commit=473832
http://drupal.org/cvs?commit=473848

Note that I accidentally added a line to location_search.module with the commit to HEAD.
I fixed that in this commit:
http://drupal.org/cvs?commit=473852

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Berliner-dupe’s picture

Status: Closed (fixed) » Active

Sorry for reopen this Issue,

i have patched google.inc with patch from #31 - the geocoding for spain works now. But not for switzerland and not for austria.

Is this a mistake from me or is the patch not for swiss or austria?

I thought the patch add the xml data from google automaticly?

Can anyone give me a feedback plz?

Thank you and regards
Matthias

gease’s picture

FileSize
493 bytes

It doesn't work on sites with primary language other than English. Here's the patch.

rooby’s picture

Status: Active » Fixed

Committed to all branches, a modified version of #40 which also translates the country fixes.
http://drupalcode.org/project/location.git/commit/a5701a9
http://drupalcode.org/project/location.git/commit/bcd9372
http://drupalcode.org/project/location.git/commit/743d2af
http://drupalcode.org/project/location.git/commit/d591b63

Aside from that I see no problem with Switzerland or Austria.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

kaidjohnson’s picture

Version: 6.x-3.x-dev » 7.x-3.x-dev

Running the 7.x-3.x-dev (as well as 7.x-3.0-alpha1-1), we are still finding that Thailand provinces are not geocoding properly. Bankok is mapping to middle of nowhere Wynne, Arkansas. We are not using other fields, just country and province. We could not reproduce this issue with any other country that we tested.