eileenmcnaughton / org.wikimedia.geocoder

Geocoder for CiviCRM
Other
6 stars 17 forks source link

update Nominatim provider to use correct search URL #37

Closed sebalis closed 11 months ago

sebalis commented 11 months ago

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.

eileenmcnaughton commented 11 months ago

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

sebalis commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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

agileware-justin commented 11 months ago

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 commented 11 months ago

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

agileware-justin commented 11 months ago

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

eileenmcnaughton commented 11 months ago

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

agileware-justin commented 11 months ago

@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 commented 11 months ago

@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 commented 11 months ago

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 commented 11 months ago

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

agileware-justin commented 11 months ago

@eileenmcnaughton here's the PR, https://github.com/eileenmcnaughton/org.wikimedia.geocoder/pull/39

eileenmcnaughton commented 11 months ago

@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 commented 11 months ago

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.