Make it possible to automatically set up geoip2 in installation #15939
Conversation
sgiehl
added
Pull Request WIP
|
what's the issue w/ making the controller requests it to the installation UI? |
That won't work easily with the hooks currently used to add the checkbox I guess. |
|
I see, I guess the original code probably uses angular or a lot of JS that needs to be loaded? Or would need to be modified to be pre-loaded during installation? |
|
Wouldn't do it directly during installation but only once fully installed. Could we set eg during the installation a flag that we need to download the DB... then we try to complete this download eg as part of an hourly task which would simply be executed eg on the first tracking request or so. Additionally, if Not sure it's somewhat clear what I mean? |
|
@tsteur change that. it will now be done using a scheduled task and if async is possible, the scheduled tasks will be triggered in the last step of installation |
plugins/GeoIp2/GeoIp2.php
Outdated
|
|
||
| if ($cliMulti->supportsAsync()) { | ||
| $phpCli = new CliMulti\CliPhp(); | ||
| $command = sprintf('%s %s/console core:run-scheduled-tasks > %s/tmp/out.txt 2>&1 &', |
There was a problem hiding this comment.
@sgiehl might be good to run eg console core:run-scheduled-tasks --force "Piwik\Plugins\GeoIp2\GeoIP2AutoUpdater.update" directly? Also is the redirection needed to out.txt needed? Eg on nginx by default the file would maybe be directly accessible. Not a big issue but might just not be needed?
There was a problem hiding this comment.
the output shouldn't be needed, that was for debugging purpose 馃檲
will change that to directly execute the task.
plugins/GeoIp2/lang/en.json
Outdated
| @@ -1,6 +1,8 @@ | |||
| { | |||
| "GeoIp2": { | |||
| "AssumingNonApache": "Cannot find apache_get_modules function, assuming non-Apache webserver.", | |||
| "AutomaticSetup": "Automatically configure geolocation using a dbip database", | |||
| "AutomaticSetupDescription": "For a proper geolocation Matomo requires an external database. Using this option, Matomo will automatically be configured to download an use the latest %1$sdbip%2$s city level database.", | |||
There was a problem hiding this comment.
could maybe say ... city level database which is licensed under the [Creative Commons Attribution 4.0 International License](https://db-ip.com/db/lite.php?refid=mtm)". or just say ... city level database. [View licensing terms](https://db-ip.com/db/lite.php?refid=mtm). Not sure we do this in the GeoIP downloader already. If not, then it might not be too important. Generally be good though if we do it in the admin as well. fyi @Findus23
There was a problem hiding this comment.
sorry just seeing this is actually already done just not as clear.
|
@tsteur updated the code |
|
Looks good to me 馃憤 |
As discussed in #12811 I've added a new checkbox in the last step of the installation.
When checked, Matomo will automatically add the GeoIP2Updater setting, so the dbip database will be donwloaded/updated monthly. In addition it will set the used location provider to geoip2php.
But I'm currently unsure how to handle the automatic download of the database. Doing that within the installation process is a bit difficult to implement and it also might take a while.
With this implementation the updater will be set and thus the automatic task should be scheduled for "tomorrow". That means until then the geolocation would be reported as broken in admin, as geoip2 is configured, but no database is available. (geolocation itself would still work and fallback to the default provider)
I've changed the geolocation admin a bit, so it will automatic start the download process if autoupdate is configured but no database is available.Not sure if that is a suitable solution, but didn't have a better idea right now.
@mattab @tsteur @diosmosis Does anyone of you have a better idea how to do/trigger the download. We could maybe try to run it async, but that might not work in many cases 馃
fixes #12811