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.
Comment | File | Size | Author |
---|---|---|---|
#40 | non-english.patch | 493 bytes | gease |
#31 | geocoding_6.patch | 7.49 KB | guillaumev |
#24 | geocoding_5.patch | 6.56 KB | nickl |
#14 | geocoding_4.patch | 6.64 KB | nickl |
#13 | google.inc_.patch | 5.86 KB | fred0 |
Comments
Comment #1
kvvnn CreditAttribution: kvvnn commentedSo, should this solve issues like a city in Thailand not geocoding, when it would geocode for Google Maps?
Comment #2
fred0 CreditAttribution: fred0 commentedkvnn,
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.
Comment #3
kvvnn CreditAttribution: kvvnn commentedWhat 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?
Comment #4
fred0 CreditAttribution: fred0 commentedCorrect on both.
Comment #5
kvvnn CreditAttribution: kvvnn commentedThanks fred0, you're the man for posting this. Seems like a rather big step forward to me.
Comment #6
kvvnn CreditAttribution: kvvnn commentedThe patch did not work for me, but I copied / deleted the (+) / (-) lines and its working great.
Thanks!
Comment #7
kvvnn CreditAttribution: kvvnn commentedThis 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.
Comment #8
kvvnn CreditAttribution: kvvnn commentedNone 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?
Comment #9
Summit CreditAttribution: Summit commentedHi,
Non on South Afrika original, it is in this way@
EDIT: Thank you for this improvement I wanted to say!
greetings, Martijn
Comment #10
kvvnn CreditAttribution: kvvnn commentedWhat?
Comment #11
nickl CreditAttribution: nickl commentedPatch didn't apply and had to be re-rolled.
Removed FeedParser dependency and fixed coding standards.
Comment #12
nickl CreditAttribution: nickl commentedChanging title and priority, manualy updating the list leaves many countries without geocoding support.
Comment #13
fred0 CreditAttribution: fred0 commentedJust 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.
Comment #14
nickl CreditAttribution: nickl commentedAttached patch rerolled against head.
Don't know any locations in Timor-Leste so patch not tested.
Comment #15
YesCT CreditAttribution: YesCT commentedwhat 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
Comment #16
fred0 CreditAttribution: fred0 commentedPatch 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.
Comment #17
YesCT CreditAttribution: YesCT commentedfred0: 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?
Comment #18
fred0 CreditAttribution: fred0 commentedThe 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.
Comment #19
hutch CreditAttribution: hutch commentedWould it be possible to rejig that so that the feed is fetched by a cron, say once a month?
Comment #20
YesCT CreditAttribution: YesCT commentedI'm not clear on why this is critical. #12 needs clarification. (I'm pretty sure feature requests aren't http://drupal.org/node/45111)
Comment #21
nickl CreditAttribution: nickl commentedThank 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...
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?
Comment #22
hutch CreditAttribution: hutch commentedLooking 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 ;-)
Comment #23
YesCT CreditAttribution: YesCT commentedI'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...
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
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.
http://drupal.org/coding-standards#indenting
bunch of comments added that are longer than 80 chars.
Powered by Dreditor.
Comment #24
nickl CreditAttribution: nickl commentedPatch re-rolled against latest DRUPAL-6--3 branch with fixes as pointed out by YesCT.
Comment #25
ankur CreditAttribution: ankur commentedI 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.
Comment #26
fred0 CreditAttribution: fred0 commentedAs 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:
Comment #27
cbrody CreditAttribution: cbrody commentedHi, 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.
Comment #28
entilza72 CreditAttribution: entilza72 commentedHi 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
Comment #29
entilza72 CreditAttribution: entilza72 commentedComment #30
UNarmed CreditAttribution: UNarmed commentedHow would i go about adding South africa to the list so it also gets geocoded?
Comment #31
guillaumev CreditAttribution: guillaumev commentedHi,
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
Comment #32
erikwebb CreditAttribution: erikwebb commentedsubscribe
I've tested the patch at #31 and it appears to work on the admin screen fine.
Comment #33
erikwebb CreditAttribution: erikwebb commentedComment #34
Aron NovakI can confirnm that the patch in #31 works fine.
Comment #35
nickl CreditAttribution: nickl commentedI 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...
Comment #36
jindustry CreditAttribution: jindustry commentedThe "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?
Comment #37
rooby CreditAttribution: rooby commentedThanks 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
Comment #39
Berliner-dupe CreditAttribution: Berliner-dupe commentedSorry 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
Comment #40
geaseIt doesn't work on sites with primary language other than English. Here's the patch.
Comment #41
rooby CreditAttribution: rooby commentedCommitted 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.
Comment #43
kaidjohnson CreditAttribution: kaidjohnson commentedRunning 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.