Module Review: Multiple Issues

Rather than issue individual bug reports (yes I am blatantly abusing the issue queue), I report the following as something that may be turned into actionable issues - but if nothing else as a means of providing feedback about the module in its current state. This is an exercise in both concrete bug reporting and constructive criticism.

1) Country Fields:
The entire country configuration options with regard to what is displayed on a location form is beyond confusing and can only be deciphered by tracing through the code to see the logic of when country fields will or will not be displayed. The fact that there are two possible country fields is an indication that countries needs a re-think - and/or detailed documentation. I would lean towards re-think.

2) Geocoding issue:
There are two ways to include a country in a location form:
The first is the default country (that can optionally be hidden)
The second is the option to display a country field and allow users to enter one.
The second is required for geocoding - if it is not entered geocoding ALWAYS fails.

3) Latitude and Longitude fields:
Once a location has been geocoded - the values of these fields are never filled in and displayed to the user.
There is no feedback provided to the user to indicate that the system has successfully geocoded the location.

4) Gmap lat/long picking
If gmaps is enabled as a lat long assistant - the map does not remain centered on the selected point between edits.
When coupled with points 2) and 3) above - geocoding appears to be completely broken to the user. (when in fact it may be working, but only if configured in a specific way, and even so the user would never know it was working).

5) Location Data Storage:
Every time a location is re-saved with a minor change - the old location information is not updated /deleted - rather a new entry is made in the location table and the old location remains.
Consider the followed entries I found in my table after testing and tracing geolocation bugs:

10  	Cirrus Gallery  542 S. Alameda Street  	   	Los Angeles  	   	90013-1708  	us  	34.039905  	-118.238328  	3  	0
11 	Cirrus Gallery 	542 S. Alameda Street 	  	Los Angeles 	  	90013-1707 	us 	34.039905 	-118.238328 	3 	0
12 	Cirrus Gallery 	542 S. Alameda Street 	  	Los Angeles 	  	90013-1708 	us 	34.039905 	-118.238328 	3 	0
13 	Cirrus Gallery 	542 S. Alameda Street 	  	Los Angeles 	  	90013-1707 	us 	34.039905 	-118.238328 	3 	0
14 	Cirrus Gallery 	542 S. Alameda Street 	  	Los Angeles 	  	90013-1708 	us 	34.039905 	-118.238328 	3 	0

I could see the location table becoming very polluted due to this saving logic.

6) Too many options:
Generally speaking I like having configuration options. But in the case of this module there are far too many - and the interactions between the configurations have not been thoroughly tested. (e.g. disabling a country field breaks geocoding).
As I understand it, location strives to be an API module, yet it is still provides dozens of UI components.

7) Debug Code in the Module:
I noticed open issues for this, but its worth mentioning again. There are calls to devel.module functions in the code.

8) Dozens of @todo items in the code
While reading through the code there are quite a few comments indicating that there is some work todo and that core functions (core to location.module) are in a state of flux. In Szeged it was mentioned that this module was nearing RC status, but there clearly seems to be many items that aren't stable. Perhaps consider updating the module's todo.txt file so that reviewers know what is stable - and what just may be leftover cruft in the code.

I mention this, because any one of the todo items could be picked up by a contributor as a way of helping the project along. But its tough to tell what is real and what direction the module is being taken.

I hope this review is helpful.

andre