localgovdrupal / localgov_geo

Entity for storing Geographic information, addresses, and areas; extensible types and plugable backends with defaults to start.
2 stars 1 forks source link

Ordnance Survey Places geocoder plugin #41

Closed Adnan-cds closed 2 years ago

Adnan-cds commented 3 years ago

This is a plugin for Drupal's Geocoder module. Useful for UK address lookup.

I am bringing these files from the localgov_forms module. This one is blocked by another pull request for the newly created localgovdrupal/localgov_os_places_geocoder_provider composer package.

Test steps (for now)

$ composer require localgovdrupal/localgov_geo:'dev-feature/os-places-geocoder-plugin as 1.1.1'
$ composer require --dev localgovdrupal/localgov_os_places_geocoder_provider:dev-feature/provider
$ drush -y en geocoder localgov_geo

Now add the Localgov OS Places Geocoder plugin from /admin/config/system/geocoder/geocoder-provider. This step, unfortunately, asks for an API key.

I am marking this pull request as Draft until localgovdrupal/localgov_os_places_geocoder_provider is ready.

andybroomfield commented 2 years ago

I'm just trying this, but it wont appear. There seems to be some namespacing issues with either the handler, or the geocoder plugin. \LocalgovDrupal\OsPlacesGeocoder\Provider\LocalgovOsPlacesGeocoder put the plugin installs under localgovdrupal/localgov_os_places_geocoder.

Adnan-cds commented 2 years ago

Hi Andy, Sorry, the class name for the OS Places PHP Geocoder plugin changed during code review. All fixed now and ready for review.

andybroomfield commented 2 years ago

Thanks @Adnan-cds however this is still not working becuase the geocoder gets installed in localgov_os_places_geocoder_provider not OsPlacesGeocoder.

Even with localgov_geo enabled and this branch checked out and localgovdrupal/localgov_os_places_geocoder_provider intalled via composer require, I am not seeing Localgov OS Places as a provider in the drop down menu to add a geocoder provider at /admin/config/system/geocoder/geocoder-provider

Adnan-cds commented 2 years ago

however this is still not working

That's worrying Andy. Good morning by the way.

the geocoder gets installed in localgov_os_places_geocoder_provider not OsPlacesGeocoder.

This should be taken care of by composer's autoloader declaration. In any case, try composer dump-autoload to recreate the site-wide class autoloader files.

I am not seeing Localgov OS Places as a provider in the drop down menu

Assuming you have already cleared the Drupal cache after fetching the latest code, may I ask if this is happening in a fresh LocalGov site or an existing site?

andybroomfield commented 2 years ago

@Adnan-cds So I did this on a fresh install and that worked ok. This is on the development copy nativly on my mac, the place I'm having issue is the copy that runs under Lando, so I'm trying to work out if it was the fresh install or the fact its Lando that is the problem.

andybroomfield commented 2 years ago

Update, using the example code that is included in the module I get this error The website encountered an unexpected error. Please try again later. Error: Class 'LocalgovDrupal\OsPlacesGeocoder\Provider\OsPlacesGeocoder' not found in localgov_geo_preprocess_page()

So its the class itself that is not being found.

Adnan-cds commented 2 years ago

Error: Class 'LocalgovDrupal\OsPlacesGeocoder\Provider\OsPlacesGeocoder' not found in localgov_geo_preprocess_page()

Thanks for persisting with this Andy. Could you please check the vendor/composer/autoload_psr4.php file for mentions of LocalgovDrupal. In my local development site, I can see two mentions:

'LocalgovDrupal\\OsPlacesGeocoder\\Tests\\' => array($vendorDir . '/localgovdrupal/localgov_os_places_geocoder_provider/tests'),
'LocalgovDrupal\\OsPlacesGeocoder\\' => array($vendorDir . '/localgovdrupal/localgov_os_places_geocoder_provider/src'),
andybroomfield commented 2 years ago

Thanks @Adnan-cds Yes I have both those entries.

    'LocalgovDrupal\\OsPlacesGeocoder\\Tests\\' => array($vendorDir . '/localgovdrupal/localgov_os_places_geocoder_provider/tests'),
    'LocalgovDrupal\\OsPlacesGeocoder\\' => array($vendorDir . '/localgovdrupal/localgov_os_places_geocoder_provider/src'),

I think this may have been a Lando issue. I've just done a lando rebuild to get it to recongnise and now it has started working. Maybe there is something strange with directory scanning between lando docker container and the mac directory structure?

ekes commented 2 years ago

The plugin was detected, but not found, that'd be new. But (and iirc lando does have apcu enabled):-

https://www.drupal.org/project/geocoder/issues/3153678#comment-14203727

ekes commented 2 years ago

OK so this is good for me I think. Did you solve your issues @andybroomfield then could add a review and get it merged?

andybroomfield commented 2 years ago

Yes this is solved, might need some documentation update though. I had to do a Lando rebuild to get it to apply. I suspect it was having some old code in there somewhere..?

Adnan-cds commented 2 years ago

Hi Andy, I have added a new section in the README about this new OS places geocoder plugin. Please let me know if this will do.

andybroomfield commented 2 years ago

Merging as per December 13th Merge Monday.

Adnan-cds commented 2 years ago

Hi Andy, Ekes, thanks for getting this in :)