magefan / module-geoip Goto Github PK

Hello magefan team!

Today I stumbled upon this strange error, when entering the admin: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'magento.adminnotification_inbox' doesn't exist, query was: SELECT adminnotification_inbox.* FROM adminnotification_inbox WHERE (title = 'NEW Magento 2 Product Tabs Extension!') AND (url = 'https://magefan.com/magento-2/extensions/product-tabs?utm_source=website&utm_medium=admin-ntf&utm_campaign=product-tabs ')

Our project has the Magento_AdminNotification module disabled, so the adminnotification_inbox was never created in the database. Is it possible that you depend on this table existing in the system without checking? This is quite bad, because is completely disables the admin area by throwing these ugly exceptions:

Would be cool if you fixed this :-) Perhaps checking for the existance of the adminnotification_inbox would be enough.

Hi,

If php-geoip is not installed, there's a fallback to the database tables but the code breaks in this case.

Here's the error:

Fatal error: Uncaught Error: Call to undefined method Magefan\GeoIp\Model\ResourceModel\IpToCountry\Collection::addFielToFilter() in /var/www/vendor/magefan/module-geoip/Model/IpToCountryRepository.php:56 Stack trace: #0 /var/www/vendor/magefan/module-geoip/Model/IpToCountryRepository.php(74): Magefan\GeoIp\Model\IpToCountryRepository->getCountryCode('127.0.0.1') #1 /var/www/app/code/Clrz/GeoIp/Helper/Popin.php(193): Magefan\GeoIp\Model\IpToCountryRepository->getVisitorCountryCode() #2 /var/www/app/code/Clrz/GeoIp/view/frontend/templates/switch/languages.phtml(35): Clrz\GeoIp\Helper\Popin->getUserCountry() #3 /var/www/vendor/magento/framework/View/TemplateEngine/Php.php(59): include('/var/www/projec...') #4 /var/www/vendor/magento/framework/View/Element/Template.php(270): Magento\Framework\View\TemplateEngine\Php->render(Object(Clrz\GeoIP\Block\Store\Switcher\Interceptor), '/var/www/projec...', Array) #5 /var/www/generated/code/Clrz/GeoIP/Block/Store/Switcher/Interce in /var/www/vendor/magefan/module-geoip/Model/IpToCountryRepository.php on line 56

The module downloads it's IP database here https://github.com/magefan/module-geoip/blob/master/Model/GeoIpDatabase/MaxMind.php#L18

As you can see with curl -sSL -D - https://magefan.com/media/geoip/GeoLite2-Country.mmdb -o /dev/null, that file was last-modified: Fri, 29 Oct 2021 10:27:55 GMT

It's about one year old, and you can tell: an IP such as 85.204.70.110 is from France, and https://www.maxmind.com/en/geoip2-precision-demo?ip_address=85.204.70.110 reports this correctly. But the module database is outdated, and so the module reports it as "Great Britain".

This is a by-the-book example of why is a terrible practice to copy and host live data. You should at very least update the database once per month or, even better, allow the user to insert an apikey and get it from maxmind.com directly.

Thanks

Hi,

Can this module used for Maxmind GeoIP2 WebService?

https://dev.maxmind.com/geoip/geoip2/web-services/

Thanks

After installing module by composer or zip archive when run command setup:di:compile getting error for Allowed memory size.

Here is attached screenshot.

There is an error here: https://github.com/magefan/module-geoip/blob/master/Model/IpToCountryRepository.php#L128

Since CWD is <magento>/vendor/magefan/module-geoip/Model, if you go up '/../../../../../ you end up in the directory ABOVE Magento, thus var/magefan/geoip/GeoLite2-Country.mmdb is ALWAYS non-existant.

Also, please use the recommended function to get the var directory.

Also, please assign the path to a variable, so you don't have to write the same code twice.

This is my quick and dirty patch:

....
                    $geoDbFile =
                        \Magento\Framework\App\ObjectManager::getInstance()
                            ->get('\Magento\Framework\Filesystem\DirectoryList')
                            ->getPath('var') . DIRECTORY_SEPARATOR . 'magefan/geoip/GeoLite2-Country.mmdb';

                    if ( file_exists($geoDbFile) ) {
                        $datFile = $geoDbFile;

...

                    } else {
....

Even better: use DI instead of ObjectManager::getInstance.

Also: that function is too long. Please put a guard clause at the beginning and split the code in multiple function: were to split is obvious just looking at the code layout.

Thanks.

Does this module work with Varnish caching enabled?