yawik / SimpleImport

Simple Job Import Module. Imports job openings into YAWIK
MIT License
0 stars 1 forks source link

Upgrade Geocoder library and add result cache #35

Closed TiSiE closed 5 years ago

TiSiE commented 5 years ago

The geocoder library used by this module released a new major version which changes its usage in a way that needs some adaption.
It changes the name to and splits the packages into the "geocoder-php/*" namespace, which has the most impact on this module.

The following actions need to be done:

We created the branch upgrade-geocoder for developing this enhancement.

Seting up a devenv.

cbleek@php7:~$ git clone https://github.com/yawik/SimpleImport.git
cbleek@php7:~$ cd SimpleImport/
cbleek@php7:~/SimpleImport$ git checkout upgrade-geocoder
cbleek@php7:~/SimpleImport$ composer install

The feature branch contains a YAWIK_TEST database which can be installed via:

cbleek@php7:~/SimpleImport$ composer db.init

The YAWIK_TEST contains a user with an organization an a simpleimport.crawler. The execution of vendor/bin/yawik simpleimport import leads to an exception, because of the changes in the geocoder-php changes.

cbleek@php7:~/SimpleImport$ vendor/bin/yawik simpleimport import
======================================================================
   The application has thrown an exception!
======================================================================
 Error
 Class 'Geocoder\Provider\GoogleMaps' not found
---------------------------------------------------------------------
kilip commented 5 years ago

@TiSiE , @cbleek

I want you to know that cache is provided by zendframework/zend-cache library, so we can use any adapter like filesystem, apc, etc. You can find more details in here

This cache can be configured in simpleimport module options:

// SimpleImport.local.php

return [
    'options' => [
        'SimpleImport/Options/Module' => [
            'options' => [
                'geocodeGoogleApiKey' => 'GOOGLE_MAPS_API_KEY',
                'cache' => [
                    'adapter' => [
                        'name' => 'filesystem',
                        'options' => [
                            'cacheDir' => __DIR__.'/../../var/cache/geo'
                        ]
                    ],
                    'plugins' => ['serializer']
                ]
            ]
        ]
    ]
];

Please let me now, if you think I need to add configuration options in this geocoder implementation.

TiSiE commented 5 years ago

At the moment, an exception is thrown, because Zend\Cache is "Missing [an] adapter".

There are two ways to handle cache: Either a filesystem cache is enabled by default in module.config.php or the caching is opt-in and need to manually be configured.

I prefer the former, because it will improve performance.

TiSiE commented 5 years ago

@cbleek To make the module work when the 'upgrade-geocoder' branch is checked out, the 'options' key in module.config.php must look like this:

'options' => [
        'SimpleImport/Options/Module' => [
            'class' => Options\ModuleOptions::class,
            'options' => [
                'cache' => [
                    'adapter' => [
                        'name' => 'filesystem',
                        'options' => [
                            'cacheDir' => 'var/cache/geocoder',
                        ],
                    ],
                    'plugins' => ['serializer'],
                ],
            ],
        ],
        Options\LanguageGuesserOptions::class => []
    ],

Additionally test/sandbox/config/config.php needs the first line:

chdir(dirname(__DIR__));

Do not forget to run composer install (optionally with the '--no-dev' flag) afterwards.

TiSiE commented 5 years ago

I've accidentally committed the default configuration already: b56bdd4

cbleek commented 5 years ago

@kilip the new version does not seem to set the default language anymore.

at least we noticed, that region names stored in the mongodb are in "english".

The factory sets the region but no default language.

We've to take a look at this befor we can merge this into the master.

cbleek commented 5 years ago

I noticed, that a test is failing

https://travis-ci.org/yawik/SimpleImport/jobs/548793398#L1823

kilip commented 5 years ago

@TiSiE, @cbleek

I just want to mention that in new geocoder-php structure locale needs to be defined in the query. So I have add new locale props in Job/GeocodeLocation (which using value from geocodeLocale options):

https://github.com/yawik/SimpleImport/blob/f4473e676048d2a5db70fca1c016e0d31516a951/src/Job/GeocodeLocation.php#L40-L44

This locale value will be used in geocoder query like this:

https://github.com/yawik/SimpleImport/blob/f4473e676048d2a5db70fca1c016e0d31516a951/src/Job/GeocodeLocation.php#L72-L83