Adds update script to remove GeoIP2 plugin from marketplace #12897
Conversation
sgiehl
added
not-in-changelog
core/Updates/3.5.1-b1.php
Outdated
| // which causes problems with GeoIp2 plugin included in core | ||
| // Skip for Windows as file system is case insensitive and names are handled equal | ||
| try { | ||
| if (!SettingsServer::isWindows() && is_dir(PIWIK_INCLUDE_PATH . '/plugins/GeoIP2')) { |
There was a problem hiding this comment.
Think this also needs to check for OS X too, the update deleted the GeoIp2 plugin when I ran it locally:
There was a problem hiding this comment.
Wondering how to detect if it's OS X. Maybe it would be better to find a solution to check if the file system is case insensitive. Maybe I could create a temp file with uppercase name and check if it exists with lower case name and run the update script only if that doesn't work?
There was a problem hiding this comment.
That could work. If readdir maintains case in the results, could check the file name in php. Can’t think of anything else...
There was a problem hiding this comment.
OSX can have case or case insensitive file systems AFAIK and just because it is windows doesn't mean it has some specific case. You may rather want to detect which plugin it is by checking eg for the existence of a specific file of the other plugin and this plugin etc?
There was a problem hiding this comment.
data/ISO3166-2_to_FIPS.json should be pretty unique.
There was a problem hiding this comment.
Just checking for existency won't work I think. The update will download the new version and overwrite all files with the new one. For a case insensitive filesystem that means the contents of the old GeoIP2 plugin will be overwritten with our GeoIp2 plugin. So checking for existency of any file of the old plugin can just be used to check if it was installed at all. Guess we need to check the content of specific files to be sure if it was overwritten (case insensitive) or exists in a separate directory (case sensitive)
There was a problem hiding this comment.
I've know updated the check. Checking if a file of our new GeoIp2 plugins is located in GeoIP2 folder should hopefully be enough to detect case insensitive file systems properly. I'm not able to test that right now, as I don't have a windows or mac setup with php. @Findus23 @tsteur @diosmosis anyone of you able to prove it doesn't remove anything on a case insensitive file system?
| // Skip if new file `isoRegionNames.php` is detected within `GeoIP2` directory, as that means filesystem is case | ||
| // insensitive and GeoIP2 plugin has been partially overwritten by GeoIp2 plugin | ||
| try { | ||
| if (is_dir(PIWIK_INCLUDE_PATH . '/plugins/GeoIP2') && !file_exists(PIWIK_INCLUDE_PATH . '/plugins/GeoIP2/data/isoRegionNames.php')) { |
There was a problem hiding this comment.
maybe we would need to add a test that makes sure this file exists? Just to be safe... if we otherwise ever remove the file, the plugin will be removed when someone updates from say an older Matomo version to a much newer. Be good to name the test clear to indicate what happens if the file does not exist.
There was a problem hiding this comment.
good point. Will add the test now
…rg#12897) * Adds update script to remove GeoIP2 plugin from marketplace * improve check for case insensitive file system * Adds simple test to prove file used in update is present
Note: The Update script tries to completely remove the GeoIP2 plugin from file system. This is required, as as soon as the plugin gets loaded it results in a fatal. Even if the plugin is deactivated.
fixes #12886