php-nominatim icon indicating copy to clipboard operation
php-nominatim copied to clipboard

[Feature] Use client inteface or Psr Client interface

Open aedart opened this issue 3 years ago • 2 comments

Hi

In your \maxh\Nominatim\Nominatim constructor, you accept a custom Guzzle Client instance, which is really fine. But would it perhaps not be prudent to accept \GuzzleHttp\ClientInterface or \Psr\Http\Client\ClientInterface as argument? If so, the your component's flexibility will increase, by allowing developers to provide custom http client implementations.

This feature request can be considered somewhat breaking. So, if you accept it, then it should be for your next major version.

aedart avatar Sep 16 '22 07:09 aedart

Hi @aedart,

Indeed, this could be great. We can event avoid breaking something by blocking Guzzle versions below 7.0 since \Guzzlient\Client implement \Psr\Http\Client\ClientInterface.

It's been a while since I worked on this project, here is some other idea that could be done :

  • Remove this statement : https://github.com/maxhelias/php-nominatim/blob/27066844b5f3c852e1ad5fbf5df32f6387c0abe1/src/Nominatim.php#L103-L115
  • Rework the $application_url param on the \maxh\Nominatim\Nominatim class because the base_uri is enough

maxhelias avatar Sep 16 '22 08:09 maxhelias

Hi @maxhelias

That would be great.

As for you additional idea to remove $application_url juggling; I think you are right, it could simplify the constructor logic. However, that will become a breaking change, if your code adheres to semver 2. (You can always release a new major version - its all good for me).

aedart avatar Sep 18 '22 08:09 aedart