Synchronize construct with nominatim

[eileenmcnaughton]

Per https://github.com/geocoder-php/nominatim-provider/commit/3068e7f692e0d799679adcfd81df6046cfcab8fb Nominatim accepts 2 additional parameters - one being 'required' because of Nominatim Terms of service. Really the constructor should be standard between all providers so if this is accepted for Nominim then it makes sense to put it in the parent. The point of a factory like this is for one syntax to work with many providers

Note I don't think it needs to be 'required'

[eileenmcnaughton]

@atymic yes - it seems so. I have used similar packages that aggregate & standardise a number of providers for other tasks (e.g Omnipay) and the goal is always that the calling code can cope with a number of providers without being adjusted per package.

Ideally I would

$provider = new $class($client);
$provider->setReferrer();
$provider->setUserAgent();

& the parent would provide null variants of those params

OR I would call

$provider->setParameters(['user_agent' => ...);

& the parent would have a class like

$method = 'set' . $key;
if (methodExists($method , $this) {
  $this->$method($value);
}

The ideal is that I can implement many providers & provide 'enough' information in my code to cover what they might need without having to know which ones do or don't need a specific piece of information or what their signature is.

The alternative is I wind up with a big switch statement

if ($providerName === x) {
  $provider = new $providerName(.... specific variables)
}

if ($providerName === y) {
  $provider = new $providerName(.... specific variables)
}

I just updated from an older version from the library & the constructor change on Nomatim broke stuff - because it went from one required argument to now needing to map multiple arguments that may or may not be required . When it was just one argument from a limited range I was able to store the mappings in the DB & a data administrator could manage that. But once we are into 'non-provided data' (ie not just an api key or similar) - that approach won't work & I will have to develop something new - probably a shim that provides standardised wrapper constructors.

For now I'm going to see if I can get away with just passing the arguments needed for Nominatim to all of the providers we use.

[atymic]

I just updated from an older version from the library & the constructor change on Nomatim broke stuff

I agree, those parameters should have been added as optional rather than required. We shouldn't introduce BC break unless absolutely necessary

For configuration, you could have a file that maps the provider classes to their correct arguments, similar to the way the laravel package does.

Some kind of per-provider configuration will usually be required to provide API keys, etc.

[eileenmcnaughton]

Yes - per provider configuration is required - but that can be in a database when it is api keys & such like - which is what we did & which allows non-devs to add new ones.

Obviously I can hack past this for now (because I'm OK with breaking other integrations to fix Nominatim as that is the one we have unit tests on) & if I come back to it I'll create a wrapper that normalises the constructors so we can use it and other people can too.

[jbelien]

The nominatim provider is the only to require User-Agent and Referer, so I really don't think it makes sense to add those parameters to AbstractHttpProvider.php.

That being said, it's true that some coherence between constructors could be good idea but would create a huge breaking change !

If we go that way, I would rather do something like this:

public static function withOpenStreetMapServer(HttpClient $client, string $userAgent, string $referer = ''): self
{
    $provider = parent::__construct($client);

    $provider->setRootUrl('https://nominatim.openstreetmap.org')
    $provider->setUserAgent($userAgent);
    $provider->setReferer($referer);

    return $provider;
}

Same idea with Nominatim constructor!

[jbelien]

I'll close this PR because it won't be merged in its current state but the discussion about this is of course more than welcome!

@Nyholm @willdurand If you have any input on this topic, that would be great :)

[eileenmcnaughton]

Thanks @jbelien - hopefully something will come out of it

[Nyholm]

I agree with @jbelien There is no reason to update the AbstractHttpProvider with a feature only used with 2 providers.

[jbelien]

@Nyholm Should we make all the constructor more coherent with one another ?

[Nyholm]

No, there is no reason. Every provider is different and have different needs and requirements. That is one of the reasons we put them in separate packages. We want them to live independently and by themselves.

[jbelien]

Fine by me ! 👌