Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nominatim geocoder cannot handle IP addresses #859

Merged
merged 3 commits into from Jul 6, 2018

Conversation

mtmail
Copy link
Contributor

@mtmail mtmail commented Jun 21, 2018

Nominatim has no such feature.

The tests sent 127.0.0.1 and expected localhost in the response but that response is hardcoded in the AbstractProvider and never came from Nominatim.

Adjusted the tests. phpunit passes for me.

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct. Thank you..

One question though... is this a BC break?

I could previously lookup "127.0.0.1" and now I get an exception.
I could also lookup any other IP address and get an empty response, now I get an exception.

Either case, we need a change log. If it is a BC break, prepare a major version of this provider.

@mtmail mtmail force-pushed the nominatim-has-no-ip4-support branch from 13a2b17 to 5247454 Compare June 28, 2018 17:34
@mtmail
Copy link
Contributor Author

mtmail commented Jun 28, 2018

Makes sense. I added a changelog entry now and upgrade from 4.1=>5.0.

I assume PHP packagist takes the version number directly from the changelog? At least I couldn't find any other mention of 4.1 in other files.

@Nyholm
Copy link
Member

Nyholm commented Jun 28, 2018

I (or some other maintainer) have to make a release. Packagist looks at all the releases.

I think this looks good. I’ll to a more detailed review later. Or maybe @jbelien wants to take a look?

@jbelien
Copy link
Member

jbelien commented Jun 29, 2018

@Nyholm Let me just fix the issue with the headers (User-Agent and Referer) - see #853 - before creating a new release, I'll take care of that in the next few days 😃

@jbelien
Copy link
Member

jbelien commented Jul 2, 2018

PR #873 is ready to be reviewed.
Once both PR #859 and #873 are merged I think we can release version 5.0 of Nominatim provider !👍

@jbelien
Copy link
Member

jbelien commented Jul 6, 2018

I think we can merge this PR and release version 5.0 ! 🎉

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great thanks!

@Nyholm Nyholm merged commit d8cb651 into geocoder-php:master Jul 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants