geocoder-php / Geocoder

The most featured Geocoder library written in PHP.
https://geocoder-php.org
MIT License
3.94k stars 517 forks source link

Here provider needs an update: apiKey #1087

Closed verybigelephants closed 3 years ago

verybigelephants commented 3 years ago

now, the created apps use apiKey instead of app_id and app_code, however, the library only accepts app_id/app_code. these two cannot be generated in the Here panel for anymore (only for ios and android apps), and official developers even discourage using them https://stackoverflow.com/questions/60438951/does-app-key-means-the-same-as-app-code-for-here-geocoding-credentials-on-py

jbelien commented 3 years ago

Hello @verybigelephants ,

Thanks for the notice! Would you have time to create a PR to extend the library to use apiKey ?

verybigelephants commented 3 years ago

i would like to contribute i'm not sure how does the process actually work with the composer libraries. that is, how to actually test the code that i'll write :D

could you point me in the right direction?

jbelien commented 3 years ago

All are dedicated repository for each provider are read-only ; you'll have to submit your PR to this main repository [geocoder-php/Geocoder](https://github.com/geocoder-php/Geocoder]. You can find the Here provider in src/Provider/Here.

For testing while your adapting the provider, you can for instance use path in your composer.json file to point the library to a specific folder.

verybigelephants commented 3 years ago

i've tried "\o_O/"

https://github.com/geocoder-php/Geocoder/pull/1088

atymic commented 3 years ago

I had a PR open for this already as well, in https://github.com/geocoder-php/Geocoder/pull/1042 I'm not sure why it didn't end up being merged, will check now.

atymic commented 3 years ago

Okay, looks like it just needs docs, I can update this arvo.

verybigelephants commented 3 years ago

i don't mean to offend or anything, but why would you keep deprecated app id/code inside the main __constructor and then make another another static method for a new constructor. this is a red flag that the library will need yet another update in a short future.

i mean, i understand that you've wanted to keep the old users using this api update without a hassle, but then why wouldn't you use something like func_get_args inside a __constructor and check the argument types then? :S

jbelien commented 3 years ago

I had a PR open for this already as well, in #1042 I'm not sure why it didn't end up being merged, will check now.

Oh riiiight, I thought we had already did something about that ... didn't think to check the existing PR 🤦‍♂️ @atymic Could you finish that one so we can merge it ?

jbelien commented 3 years ago

Duplicate of #1041

jbelien commented 3 years ago

i don't mean to offend or anything, but why would you keep deprecated app id/code inside the main __constructor and then make another another static method for a new constructor. this is a red flag that the library will need yet another update in a short future.

i mean, i understand that you've wanted to keep the old users using this api update without a hassle, but then why wouldn't you use something like func_get_args inside a __constructor and check the argument types then? :S

@verybigelephants We try to avoid as much as possible any breaking change. If appId/appCode still works (which seems to be the case, even though they're deprecated), there should not be any breaking change. In the way proposed in #1042, the current user won't have to adapt anything (which is how it should be) and the new appKey method is available.

jbelien commented 3 years ago

Version 0.5.0 is released.