Problem/Motivation

The geocoder will send an API request even if the $address_string is an empty string. I think this is an obvious case that could be excluded, to save an API request that we know won't return anything.

Steps to reproduce

$address_string is an empty string

Proposed resolution

Simply log a warning, cache an empty result, and return early. And you might not even need to log a warning.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#10 geocoder-3574866-bab98e47.diff616 bytessolideogloria

Comments

solideogloria created an issue. See original summary.

solideogloria’s picture

solideogloria commented
Issue summary: View changes
itamair’s picture

itamair commented
Status: Active » Postponed (maintainer needs more info)

This is exactly what it is supposed to happen and it is happening actually ...
Could you provide evidence that is not?
What is your specific setup, where an empty string is trying to be Geocoded, arriving at this point in process workflow:
https://git.drupalcode.org/project/geocoder/-/blob/8.x-4.x/src/Geocoder....

solideogloria’s picture

solideogloria commented
Status: Postponed (maintainer needs more info) » Active 
        if (is_string($address)) {
          $result = $provider->getPlugin()->geocode($address);
        }
        elseif ($provider->getPlugin() instanceof ProviderGeocoderPhpInterface) {
          $result = $provider->getPlugin()->geocodeQuery($address);
        }

Empty string is a string, so it will call ->geocode($address), even though we already know that it won't be successful if $address is an empty string.

The call could instead be skipped completely for empty string, before the for loop. Personally, I think it shouldn't even throw an exception in that case. Just return NULL. This is because the only way to decide to skip geocoding during the alter hook is to set the address string to empty string. However, this causes an exception to be thrown and logged every time that happens.

  • itamair committed bab98e47 on 8.x-4.x 
    feat: #3574866 Skip geocoding an empty string (in Geocoder service)
By:...
itamair’s picture

itamair commented
Status: Active » Fixed

Ok, thanks @solideogloria
this have sense to me also.

I verified that using the Geocode service throughout the geocoding of a field entity value (thus with the geocoder_field module) that code (from the Geocoder service) is never reached in case the string being geocoded is empty, because of this:
https://git.drupalcode.org/project/geocoder/-/blob/8.x-4.x/modules/geoco...

But it could still be the case of geocoding operations directly/programmatically triggered via the APIs:

$addressCollection = \Drupal::service('geocoder')->geocode($address, $providers);

Thus it could be appropriate not to perform it in case the $address is an empty string, after all its possible alters.
And it still makes sense to me not to silently fail it, but log a warning message regarding the attempt to Geocode as empty source …

Committed into origin/8.x-4.x dev branch, will be part of the next incoming module release.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

solideogloria’s picture

solideogloria commented

Alright, thank you.

Status: Fixed » Closed (fixed)

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

solideogloria’s picture

solideogloria commented
StatusFileSize
new geocoder-3574866-bab98e47.diff616 bytes

Patch file until a new release is created with the changes.

solideogloria’s picture

solideogloria commented

@itamair I think this causes an error:

TypeError: Drupal\geocoder\ProviderUsingHandlerBase::geocodeQuery(): Argument #1 ($query) must be of type Geocoder\Query\GeocodeQuery, string given, called in /var/www/html/web/modules/contrib/geocoder/src/Geocoder.php on line 89 in Drupal\geocoder\ProviderUsingHandlerBase->geocodeQuery() (line 52 of /var/www/html/web/modules/contrib/geocoder/src/ProviderUsingHandlerBase.php).

Does something need to be added to the following elseif to verify that $address instanceof GeocodeQuery?