I need to allow for partial geocoding.
For example, if the street address is not found, geocode to the city and state.

So "123 Totally Wrong Address, Valid City, Valid State, 12345" would geocode to "Valid City, Valid State, 12345".

I have created a patch that does this.
I had to play around with the way the failure message is displayed, so it needs a good look-over, and it has no tests.

Additionally, this might not even be something that the maintainers want.

CommentFileSizeAuthor
#4 Screenshot 2020-01-13 09.44.25.png17.39 KBarnaldop
#4 Screenshot 2020-01-13 09.43.25.png19.85 KBarnaldop
#4 allow-for-approximate-geocoding-3105954-4.patch3.58 KBarnaldop
#3 Screen Shot 2020-01-12 at 11.17.25 PM.jpg361.46 KBitamair
allow-for-approximate-geocoding.patch4.11 KBarnaldop

Comments

arnaldop created an issue. See original summary.

itamair’s picture

itamair commented
Status: Needs work » Closed (won't fix)

Sorry to say @arnaldop but after a quick view and first in-deep review all this seem to me both a bizzarre feature request and a very messy implementation attempt, also totally incomplete.

1 - You are asking for the implementation in the Geocoder module of a functional logics that should be addressed in the Geocode providers themselves eventually: the output/response of an Address Geocode request should be (and is) on behalf of the specific Geocoding Provider service.
The Drupal Geocoder module should/would just wrap on that and output the response, with Dumping (@GeocoderDumper plugin) the output or Re-Formatting (@GeocoderFormatter plugin) the Formatted Address output;

2 - You patch is totally incomplete (what is the geocode1 case?), doesn't comply to the Drupal coding standards at all (@sse the attached screenshot) and is also very badly written in its functional logics.
The following code snippet really doesn't makes sense to me:

(omissis) 

while (!isset($geo_collection) || is_null($geo_collection)) {
            $geo_collection = isset($candidate_value) ? \Drupal::service('geocoder')->geocode($candidate_value, $providers) : NULL;

            if (!isset($geo_collection) || is_null($geo_collection)) { // geocoding failed
              $index_of_comma = strpos($candidate_value, ",");

(omissis)

 } else { //geocoding succeeded
              if (strcasecmp($candidate_value, $original_candidate_value) === 0) {
                unset($failure_status_message);
              } else {

(omissis)

How might the "else" statement be reached with that initial while that replicates the exact sequent "if" condition ???

I even didn't try to better understand what the whole new case "geocode": code tries to check.

Sorry to say (again) but this way of contributing sounds really un-constructive to me, and just time consuming ...
Such an incomplete patch should not be posted in this way, although tagged as "needs work" because it only makes a lot of confusion.
At least it should work somewhere.
In this case I do not see absolutely what minimum task it can perform and what specific use case it can solve, without risking to break the more general use cases functional logic ...

itamair’s picture

itamair commented
StatusFileSize
new Screen Shot 2020-01-12 at 11.17.25 PM.jpg361.46 KB
arnaldop’s picture

arnaldop
Location St Cloud, FL
commented
StatusFileSize
new allow-for-approximate-geocoding-3105954-4.patch3.58 KB
new Screenshot 2020-01-13 09.43.25.png19.85 KB
new Screenshot 2020-01-13 09.44.25.png17.39 KB

@itamair, my thoughts:

  1. "Bizarre feature request" - The client cannot enforce that users enter addresses that always geocode correctly. Therefore, the client asked that we geocode a partial address, so that the content will at least appear somewhere on the map, instead of not showing up at all. So the original request itself is totally reasonable.
  2. "Very messy implementation" and "totally incomplete" - Perhaps, perhaps not. It all depends on whether you understood what the patch does.
  3. Your point #1 is totally valid, and it was an option that did not occur to me. Your view, that the burden should fall on the provider, is valid. However, it could be argued that I might want the field to handle this if I want the same behavior (partial fallback geocoding) even if I am using multiple providers (for whatever reason I might have). In this case, I would have to implement the fallback in a custom version of each provider. Perhaps this is the right solution, but I wouldn't say it's the only way to solve the problem.
  4. Regarding the coding standards, I admit that there were quite a few things I missed, such as spaces at the end of a line. However, some of the other things I left in the code were left there on purpose, such as the comments, so they could aid in our discussion as we worked on it to improve it.
  5. As far as I am aware, there is no mechanism for people like me, non-maintainers, to share code with maintainers, other than to submit a patch like this one. So unless I missed something, this is the only way I would have to share my ideas with you. So while I admit that this patch needed some additional work, I had no other means to even bring this to your attention in-process (meaning, connected to a feature request that is tracked via the module's Issue Tracker).
  6. See below for a very simple pseudocode flow describing the code changes. As you'll see, it's all very simple.
  7. The code will absolutely reach the else, even though the conditions are the same.
  8. You have many lines that exceed 80 characters. Actually, 11% (40 out of 359 lines) of your file exceeds 80 characters. So I'm not sure if that coding standard is a rule or a suggestion.
  9. "I even didn't try to better understand..." - That is exactly the problem, unfortunately. You didn't try.
  10. "In this case I do not see absolutely what minimum task it can perform and what specific use case it can solve..." - This was described in the original request. The code matches the feature request perfectly.
  11. The code could be optimized a bit, and after dismissing your rant, I did see a few things that could improve, and I made those changes locally.
  12. To compound to the absurdity of your response, the code works perfectly! It does exactly what the original description requested.

@itamair, we had a Slack chat last year and you were incredibly encouraging and inspirational. But now, I find your response to my request to be the definition of "un-constructive". You made certain assumptions, failed to try to understand the code, and then took action based on your flawed understanding.


The patch works well. Very well, actually.
It took me several hours to put it together.
Your point that perhaps this functionality should be in the providers is noted.
I feel that your response is destructive to the Drupal community as a whole, as it discourages people from trying to contribute, regardless of skill level.
In a corporate environment, your tactless response would be grounds for administrative action.

I attached an update that takes into account your comments about standards, etc., and screenshots showing this patch working as designed.

P.S.: For everyone's edification, here is the pseudocode for the case 'geocode' segment so everyone can follow:

  1. save original address field value and original failure message
  2. while true (loop until we call break in one of the internal if statements)
  3. - attempt geocode
  4. - if geocode failed (geocoding is attempted inside the while, so it could go into the while but fail the if - this is basic code flow, which works in any language)
  5. -- if there is a comma in the address
  6. --- remove initial section of the the address, such as the street info
  7. -- else (there are no commas left)
  8. --- set failure message
  9. --- break from while loop
  10. - else (geocoding succeeded)
  11. -- if the geocoded address is the original address, unset the failure message
  12. -- else, use failure message to notify the user that an approximate address was geocoded
  13. -- break from while loop
itamair’s picture

itamair commented
Status: Closed (won't fix) » Needs work

Ok. So you amended your patch, in a meaningful way ... so it looks more logic (in its functional workflow) at least to me:

- the "case 'geocode1' is not mentioned anymore ... (that couldn't be intercepted in any way);
- now the "else" statement into the "while (true)" might be thrown, and break the while itself (in the previous "allow-for-approximate-geocoding.patch" patch version it couldn't, relying on the "while" way of working);

BUT still my (personal) opinion is that your patch is trying to override a very singular specific use case of the module (when the address fails to geocode and contains a comma at the beginning, so it is supposed to might be stripped out. What about if the Address doesn't contain any comma?), and in a very specific workflow of it (it means just in the "geocoder_field_entity_presave" function and not in the more general "Drupal\geocoder\Geocoder" service ... so to behave the same in every Geocoder api implementation).
and still I think that it not his behalf ... but the single Geocoder Providers one.

It still wouldn't pass my review and I still wouldn't commit this patch into the module.

BUT ... ok. May be my point of view and feedback are too rigid and personal (although still very constructive and collaborative so as all my activities related the Drupal8 Geofield Map Stack ... so far).
So let's tag this back to "Needs Work" for a while and let's see if any other authoritative developer in Drupal 8 steps in and believes that this feature might be implemented, in your own way or in some better and more general one ... and gives a further feedback to this.

itamair’s picture

itamair commented
Status: Needs work » Closed (outdated)

No news oaths issue. Nobody stepped in to follow up omg this further. Closing this ...