Problem/Motivation

Several geocoder providers can use, or even require, additional parameters that are stored in the data part of the GeocodeQuery accessed with getData and setData methods. If you are building a site, or coding, knowing which gecoders are to used this is useful (to essential depending on gecoder and site requirements).

The use case I've run into this is for restricting results by countrycodes (which of course you know when someone has filled in an address field as it's the first thing they have to do) for Nominatim but a short grep threw up plenty of more use cases:-

TomTom: setData('countrySet')
Yandex: (reverseQuery) setData('toponym')
PickPoint: (reverseQuery) setData('zoom') required (defaults to 18)
Nominatim: setData('countrycodes') setData('viewbox') setData('bounded')
  (reverseQuery) setData('zoom') required (defaults to 18)
OpenCage: setData('countrycode') setData('proximity')
  also getBounds()
LocationIQ: (reverseQuery) setData('zoon') required defaults to 18
Mapbox: setData('location_type') setData('fuzzyMatch') defaults to false setData('country)'
GoogleMaps: setData('components') required setData('region') defaults to null (can be set on construction)
GoogleMapsPlaces: (both) setData('mode') required defaults to 'find'
AlgoliaPlaces: setData('type') and setData('countries') required no defaults
Here: 'country', 'state', 'county', 'city'; and open 'additionaldata'

Proposed resolution

The resolution, tasks, and API changes, for this this can be the same as #3406299: Content language by being able to construct the GeocodeQuery that is passed. Both get fixed with the same solution.

CommentFileSizeAuthor
#5 additional-geocoder-param-3406303-1.patch6.76 KBprogga
#4 additional-geocoder-param-3406303-0.patch6.88 KBprogga

Issue fork geocoder-3406303

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

ekes created an issue. See original summary.

itamair’s picture

itamair commented

Thanks @ekes ... indeed this is a nice insight and input. Good ideas for nice enhancements of the Geocoder module.

ekes’s picture

ekes commented
Status: Active » Needs review

Something like this?

https://git.drupalcode.org/issue/geocoder-3406303/-/compare/8.x-4.x...34...

I'd kinda like a way of telling, as a consumer of the services, which type of plugin (instanceof Provider, or instanceof GeometryProviderInterface) it is; but I couldn't quite think of a way of separating them further without breaking backward compatibility. So this is trying to integrate with what's there in the same way it's working now.

progga’s picture

progga commented
StatusFileSize
new additional-geocoder-param-3406303-0.patch6.88 KB

Thanks Ekes. I have tested the code and it works as expected. ProviderUsingHandlerBase is missing a `use` statement for the `ProviderGeocoderPhpInterface` interface. Otherwise all good.

I was thinking it would be good if the `geocoder` service could accept GeocodeQuery. The attached patch for the fork achieves that.

progga’s picture

progga commented
StatusFileSize
new additional-geocoder-param-3406303-1.patch6.76 KB

Corrected patch for the fork attached.

ekes’s picture

ekes commented

> I was thinking it would be good if the `geocoder` service could accept GeocodeQuery.

I was trying to think of a way of making the change there backward compatible and that does seem to do it.

ekes’s picture

ekes commented

Added to the branch.

progga’s picture

progga commented
Status: Needs review » Reviewed & tested by the community

Thanks. Tested with a few Geocoders and worked as expected. No errors or warnings in the Drupal log. RTBC.

ekes’s picture

ekes commented
ekes’s picture

ekes commented

I've now got code using this, and it's doing just what's wanted there too: https://git.drupalcode.org/project/geo_entity/-/blob/4bd966f02827113f4ed...

itamair’s picture

itamair commented

Thanks a lot here. I am following and will take my chance to double check and test this, before committing into dev ...

finn lewis’s picture

finn lewis
Location Oxford
commented

Hey itamair,

any chance of rolling this into a release?

We could use it in LocalGov Drupal.

Many thanks,

Finn

  • itamair committed ccb63852 on 8.x-3.x 
    Issue #3406303 by progga, itamair: Pass additional parameters to...

  • itamair committed 90265a17 on 8.x-4.x 
    Issue #3406303 by progga, itamair: Pass additional parameters to...

  • itamair committed c0378e5d on 8.x-4.x authored by progga 
    Issue #3406303 by progga: Pass additional parameters to geocoders
itamair’s picture

itamair commented
Status: Reviewed & tested by the community » Fixed

Swell ... actually I had to make some fixes to #5 patch with an additional commit,
some lack of quality in that implementation:

i.e.: the ProviderGeocoderPhpInterface not really existing anywhere ...

then I QAed positively and all this was worth to be deployed.

Both 8.x-4.19 and 8.x-3.42 new releases are now implementing this.

ekes’s picture

ekes commented

I'll review this, but I think what's been committed is missing a lot of the patch that is required. Including that interface!

progga’s picture

progga commented

Thanks for merging. And sorry for causing the confusion. I should have been clearer. The patch was for Eke's fork. So it was more like an interdiff. Please accept Eke's pull request for full functionality.

itamair’s picture

itamair commented
Status: Fixed » Needs work

Ah ok ... sorry, my fault, but also some confusion here. If there is a MR open, patches don't make much sense in the thread (but only confusion) isn't it?

itamair’s picture

itamair commented

Please @ekes adjust the MR to the actual 8.x-4.19 head ...

progga’s picture

progga commented

> patches don't make much sense in the thread (but only confusion) isn't it?

Yeah, I should have been smarter :(

  • itamair committed 1ed50fad on 8.x-4.x 
    Revert "Issue #3406303 by progga: Pass additional parameters to...

  • itamair committed b993841e on 8.x-4.x 
    Revert "Issue #3406303 by progga, itamair: Pass additional parameters to...

ekes changed the visibility of the branch 3406303-pass-additional-parameters to hidden.

itamair’s picture

itamair commented

FYI ... I rolled / reverted everything here with a new 8.x-4.20 release that brings everything before the commits mentioned in this thread

ekes changed the visibility of the branch 3406303-pass-additional-parameters to active.

ekes’s picture

ekes commented

OK so `3406303-pass-additional-parameters` was the original, now merged again with 4.x at 4.20; and `3406303-additional-parameters-update` was my reapplying (I think correctly) to the updated 4.x with the two commits that were on 4.19, but that's not needed now. However for certainty you should see that there is no diff between the two branches (just different commits) :-)

Both branches should have both my original geocodeQuery stuff, and progga's addition that makes it work from the geocoder class directly.
Branch `3406303-pass-additional-parameters` that applies to current 4.x HEAD will be the one you want now though.

  • itamair committed 8a54caa3 on 8.x-3.x 
    Revert "Issue #3406303 by progga, itamair: Pass additional parameters to...
itamair’s picture

itamair commented

Thanks @ekes got it ...
Actually I tested locally (it is working fine ... ) the `3406303-pass-additional-parameters` branch and spot some / many refinements (but all small coding standards, some typos, etc. nothing more ... ) that need to be added to it.
Thus ... could you open a MR on that? so I can commit my refinement and also proceed with my code review with some further comments
(i.e. the tests/modules/gecoder_test_provider/src/Geocoder/Provider/MockProvider.php functions are all missing documentation ... etc.).

Being open I am still a bit reluctant in merging / committing all this, because it is quite a lot of new code and I couldn't really test this new functionality and use case ...
BUT I am also inclined to trust your skills and match your needs (as a Local Gov Team).

Besides the MR and my further (little) comments on that, could you just answer to the following question of mine, to make me more confident and sure about all this:

  1. is this code NOT breaking any backward compatibility in any way? are you pretty sure about that? Again ... I just tested on my Local Geocoder playground and all looked working still fine ... BUT need a further assuring;
  2. it looks all this cannot be tested from a normal Drupal backend (from were the Geocoding will always happen on an address string), hence I assume you are going to apply this in a code basis scenario.
  3. Could you briefly post here a code Gist / example on how this is going to be used, in a similar scenario of you?
  4. (besides what is being dummy tested in tests/src/Kernel/ProviderTest.php ...)

Thanks! again ...

ekes’s picture

ekes commented
Issue tags: +ContributionWeekend2024

Here goes:

Backward compatibility

The public method Geocoder::geocode does change its signature but only adding that it also accepts a GeocodeQuery in addition to a string https://git.drupalcode.org/issue/geocoder-3406303/-/compare/8.x-4.x...34...

The reported change https://git.drupalcode.org/issue/geocoder-3406303/-/compare/8.x-4.x...34... in the ProviderBase::process is in fact one that was already the case before this patch, again it is additional, it was already also returning an Address Collection or Geometry. This is also the same for doGecode and doReverse. It's a Collection if the geocoder is from PHP-Geocoder https://github.com/geocoder-php/Geocoder/blob/9c2224fc81be843c5c8c3e8925... and Geometry if it's from geofield https://git.drupalcode.org/project/geocoder/-/blob/8.x-4.x/modules/geoco...

Added functionality

So in addition to the additional GeocodeQuery argument type for Geocoder::geocode method at the Provider level it adds explicit methods to use if you have constructed a GeocodeQuery yourself and want to run it directly with a specific geocoder provider. This makes sense because you often want to add particular specific arguments for specific geocoders in the GeocodeQuery. These are defined by the interface https://git.drupalcode.org/issue/geocoder-3406303/-/compare/8.x-4.x...34... which any provider supporting GeocodeQuery will implement. This is basically all of the PHP-Gecoder providers that return the AdressCollection, and not the GeometryProviderInterface based ones.

You see the interface and new methods in use in the Geocoder::geocode method https://git.drupalcode.org/issue/geocoder-3406303/-/compare/8.x-4.x...34... where it decides only to pass a GeocodeQuery into the provider if it implements the interface

Changes to how code is working internally

To enable reuse of the caching mechanism the methods to store have been spun out from ProviderBase::process. The internal methods ProviderBase::getCache https://git.drupalcode.org/issue/geocoder-3406303/-/blob/3406303-pass-ad... and ::setCache https://git.drupalcode.org/issue/geocoder-3406303/-/blob/3406303-pass-ad... are called in the ProviderBase::process https://git.drupalcode.org/issue/geocoder-3406303/-/blob/3406303-pass-ad... and it will operate in the same way as it did with the array containing a single string, or lon and lat https://git.drupalcode.org/project/geocoder/-/blob/8.x-4.x/src/ProviderB...

But now these methods are available for geocodeQuery https://git.drupalcode.org/issue/geocoder-3406303/-/blob/3406303-pass-ad... and reverseQuery https://git.drupalcode.org/issue/geocoder-3406303/-/blob/3406303-pass-ad... where they can pass in a GeocodeQuery. The reason to reuse the code in a method, in addition to DRY, was it would be easier to change the caching in the future as it's all in one method that everyone can use.

Testing

The test https://git.drupalcode.org/issue/geocoder-3406303/-/compare/8.x-4.x...34... call the Mock Provider https://git.drupalcode.org/issue/geocoder-3406303/-/blob/3406303-pass-ad... which if it the code described above is correct will get called and return a standard response. This is checking that the Provider's methods are actually getting called correctly if you are using the ::geocode, ::geocodeQuery etc. methods.

The additional tests https://git.drupalcode.org/issue/geocoder-3406303/-/compare/8.x-4.x...34... check that the Geocoder::geocode method does not break if a GeocodeQuery is passed when the provider doesn't support it; and it checks that it does return something when it does.

Usage

In the geo_entity module example also mentioned above https://git.drupalcode.org/project/geo_entity/-/blob/4bd966f02827113f4ed... the address and the country codes are made available as geodata, which are then used to customize the query depending on the provider.

For example for Nominatim it restricts the results to the country using its 'countrycodes' parameter https://git.drupalcode.org/project/geo_entity/-/blob/4bd966f02827113f4ed... No longer get results from the United States that include the string 'GB' when you know someone has created an address in Great Britain!

There are more optional parameters like the custodian_code for the https://github.com/localgovdrupal/localgov_os_places_geocoder_provider OS Places Geocoder that @progga has written. I listed other above in the motivation for this.

Passing these arguments in a GeocodeQuery to Geocoder::gecode where it runs multiple Providers is also safe, because the Providers ignore any arguments they don't know.

ekes opened merge request !45

itamair’s picture

itamair commented
Status: Needs work » Needs review

Woooooow ... @ekes, you are really outstanding.
Thousand thanks for your time in giving all those insights.
Definitely I will be happy to approve dan merge this asap.

I pushed my refinements / corrections to the MR. Please review my last commit and eventually tell me if I was wrong in something
(but I simply trusted my phpstorm drupal coding standards reporting, etc.).

Also I opened a Code review with one update/change request here: https://git.drupalcode.org/project/geocoder/-/merge_requests/45#note_258270

ekes’s picture

ekes commented
Status: Needs review » Reviewed & tested by the community

  • itamair committed b4b83e11 on 8.x-4.x authored by ekes 
    Issue #3406303 by itamair, progga: Pass additional parameters to...

  • itamair committed 5fa4db52 on 8.x-3.x 
    Issue #3406303 by itamair, progga: Pass additional parameters to...
itamair’s picture

itamair commented

Here we go ... thanks a lot @ekes, and everybody else here.
I just added another small commit on some tiny phpcs and merged both into 4.x and 3.x branch ...
All this will be part of the new incoming Geocoder release.

  • itamair committed b4b83e11 on 8.x-3.x authored by ekes 
    Issue #3406303 by itamair, progga: Pass additional parameters to...

  • itamair committed 1ed50fad on 8.x-3.x 
    Revert "Issue #3406303 by progga: Pass additional parameters to...

  • itamair committed b993841e on 8.x-3.x 
    Revert "Issue #3406303 by progga, itamair: Pass additional parameters to...

  • itamair committed 90265a17 on 8.x-3.x 
    Issue #3406303 by progga, itamair: Pass additional parameters to...

  • itamair committed c0378e5d on 8.x-3.x authored by progga 
    Issue #3406303 by progga: Pass additional parameters to geocoders
ekes’s picture

ekes commented

🎉 nice one. Thanks.

itamair’s picture

itamair commented
Status: Reviewed & tested by the community » Fixed
progga’s picture

progga commented

Great to see this merged :)

Status: Fixed » Closed (fixed)

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