maxmind / GeoIP2-php

PHP API for GeoIP2 webservice client and database reader
https://maxmind.github.io/GeoIP2-php/
Apache License 2.0
2.3k stars 274 forks source link

Package Issue #185

Closed gokulmhegde closed 2 years ago

gokulmhegde commented 2 years ago

Hi,

We updated the latest version of GeoIP2 package, we are using PHP 8 and we got this error from package

Deprecated: Return type of GeoIp2\Model\AbstractModel::jsonSerialize() should either be compatible with JsonSerializable::jsonSerialize(): mixed, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /vendor/geoip2/geoip2/src/Model/AbstractModel.php on line 63

for the older version if we pass the IP address as local IPs like 192.168.1.2 the return was blank but for the latest version it is giving fatal error

Fatal error: Uncaught GeoIp2\Exception\AddressNotFoundException: The address 192.168.1.8 is not in the database

Can you please check with this?

oschwald commented 2 years ago

Thanks for the report of the deprecation notice. We will fix that.

The AddressNotFoundException is expected for IPs that are not in the database. Please see the documentation.

reznikartem commented 2 years ago

Can you add before jsonSerialize function line with #[\ReturnTypeWillChange] ? It suppress deprecation warnings for 8.1

oschwald commented 2 years ago

Will the deprecation warning be an issue with the above change to remove the explicit return type? For some reason, I was not able to reproduce the warning on 8.1.

reznikartem commented 2 years ago

Yes, in 8.1 we receive deprecation warning like this

Return type of GeoIp2\Model\AbstractModel::jsonSerialize() should either be compatible with JsonSerializable::jsonSerialize(): mixed, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice Model/AbstractModel.php on line 63

If #[\ReturnTypeWillChange] will be prepended before method declaration, this warning gone.

I dont know - is it good idea to add this annotation, or, maybe, will be better make another branch for 8.* php, with mixed return type

oschwald commented 2 years ago

To confirm, you are using main rather than the most recent release? We haven't released this fix yet.

reznikartem commented 2 years ago

We are using 2.9.0 now, in this release jsonSerialize in AbstractModel.php looks like

public function jsonSerialize()
{
     return $this->raw;
}

It's same as in main branch in repo.

I`d recommend to change this code to

#[\ReturnTypeWillChange]
public function jsonSerialize()
{
     return $this->raw;
}

For 7.* php it's just a comment, but for PHP 8.1 it's suppress deprecation warning in logs

oschwald commented 2 years ago

Do you get the error on v2.12.2 as well? Perhaps the change in #187 was in error. The docs state that any covariant type should work.

reznikartem commented 2 years ago

Yep, no error on 2.12.2

oschwald commented 2 years ago

Thanks! I'll revert those changes.

reznikartem commented 2 years ago

Thank you!