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

update Nominatim provider to use correct search URL #37

Merged
merged 1 commit into from
Aug 6, 2023

Conversation

sebalis
Copy link
Contributor

@sebalis sebalis commented Aug 4, 2023

Hi Eileen,

Nominatim have recently taken a stricter approach to the search URLs they process (see Github issue). This leads to failures in this extension which reveal that the extension currently issues requests to URLs of the form https://nominatim.openstreetmap.org/search/search?format=jsonv2&q=[…] – note the duplicate “/search” in the path. This pull request repairs this problem, it has been tested on one affected instance. If you accept this and publish a new release soon it would be great, but maybe you would want to also include a cleaned up version of PR #36 which I will submit next.

@civibot civibot bot added the master label Aug 4, 2023
@eileenmcnaughton
Copy link
Owner

Ok - both merged - so we are ready to increment version & tag a release?

@eileenmcnaughton eileenmcnaughton merged commit 393cd52 into eileenmcnaughton:master Aug 6, 2023
@sebalis
Copy link
Contributor Author

sebalis commented Aug 6, 2023

I would think so. Thanks! One thing to keep in mind, though, is that updating will not be enough to change the Nominatim URL (according to our tests). It had to be changed in the civicrm_geocoder table. I didn’t know where to communicate this, but should have mentioned it here in any case.

@eileenmcnaughton
Copy link
Owner

Oh well I've tagged it so it gets out there - it would be better to add an upgrade script but that can always follow

@agileware-justin
Copy link
Contributor

@sebalis and @eileenmcnaughton thanks for the fix - was just tracking down what had caused geocoding to stop working. Much appreciated!

@agileware-justin
Copy link
Contributor

it would be better to add an upgrade script but that can always follow

Just noting that the upgrade script is essential and 1.9 will not work without it.

@eileenmcnaughton
Copy link
Owner

argh - @agileware-justin as in 1.9 is more broken that the previous version? If so I'll delete the release

@agileware-justin
Copy link
Contributor

1.9 update requires db to be updated. I just did it manually.

@eileenmcnaughton
Copy link
Owner

@agileware-justin ok - but is it more broken with 1.9 without the update? Or is it broken either way?

@agileware-justin
Copy link
Contributor

agileware-justin commented Aug 7, 2023

@eileenmcnaughton

org.wikimedia.geocoder 1.9 with database update = works
org.wikimedia.geocoder 1.9 without database update = does not work

The database update required is something like this:

UPDATE `civicrm_geocoder` SET `url` = 'https://nominatim.openstreetmap.org' WHERE `url` LIKE '%nominatim.openstreetmap.org%';

@eileenmcnaughton
Copy link
Owner

@agileware-justin ok - & geocoder 1.8 - does that work?

If 1.8 works & 1.9 without the update doesn't work then I will delete 1.9 release to get us back to something stable. Otherwise if it's equally broken then 1.9 can stay put.

Obviously the goal is to get 1.10 out with the fix - and I would merge a PR that does that - but I'm really just focussed on 'is it worse with 1.9 tagged rather than not tagged' at the moment.

@sebalis
Copy link
Contributor Author

sebalis commented Aug 8, 2023

I think that @agileware-justin said in #issuecomment-1667168729 that 1.8 doesn’t work, which also matches my observations that prompted me to submit this PR. Which is not to say that you should take my word over theirs. :-) In any case, I really regret not having researched how to write an update script before submitting this PR. I don’t know how to integrate it as an update script, but here is the SQL:

UPDATE civicrm_geocoder SET url="https://nominatim.openstreetmap.org" WHERE class="Nominatim\\Nominatim" AND url LIKE "https://nominatim.openstreetmap.org%";

I might be able to do find out how to inlcude an update script in the next day or so.

[Now away from Github for about 7 hours or more]

@agileware-justin
Copy link
Contributor

@eileenmcnaughton I'll submit a PR with the DB update.

@agileware-justin
Copy link
Contributor

@eileenmcnaughton here's the PR, #39

@eileenmcnaughton
Copy link
Owner

@sebalis - @agileware-justin just put up an upgrade script - which I merged. If that looks all good to you then we can tag 1.10 & hopefully the next person to hit the issue will not even know they did cos they already upgraded....

@sebalis
Copy link
Contributor Author

sebalis commented Aug 8, 2023

I have added two comments that I hope @agileware-justin will incorporate. Otherwise it looks good to me, thank you! I haven’t tested the update functionality before giving this thumbs-up though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants