Torann / laravel-geoip

Determine the geographical location of website visitors based on their IP addresses.
http://lyften.com/projects/laravel-geoip
BSD 2-Clause "Simplified" License
2.09k stars 372 forks source link

Possible bug #142

Open zgelici opened 5 years ago

zgelici commented 5 years ago

Hi,

I get an issue with a part of code. i did some checks.

vendor\torann\geoip\src\Services\IPApi.php

Line 74 gives notice:

Notice: Trying to get property 'status' of non-object

current code:

if ($json->status !== 'success') {
            throw new Exception('Request failed (' . $json->message . ')');
        }

i think this will solve it:

if ($json && $json->status !== 'success') {
            throw new Exception('Request failed (' . $json->message . ')');
        }

I get the issue when i call this:

geoip()->getLocation($ip)['country']

i dont get the notice everytime.

jadjoubran commented 5 years ago

I'm also getting that sometimes

lamalamaMark commented 5 years ago

We experience this error also frequently. @zgelici can you make a pull request containing your fix?

zgelici commented 5 years ago

We experience this error also frequently. @zgelici can you make a pull request containing your fix?

I did it

lamalamaMark commented 5 years ago

I think the solution needs to be worked out further because the code after this condition also relies on the content of the $json variable. Did you look at that part?

zgelici commented 5 years ago

@lamalamaMark

You are right, the other part need to be also inside $json check, but than its possible that the api or something gives empty results. What effect it will have.

zgelici commented 5 years ago

@lamalamaMark can you check now the new pull request

its possible that requests or something fails, so i did multi tries. if not it give empty. i didnt test it and its late here. tomorrow evening i will check my code again. but i already did push.

paulocastellano commented 5 years ago

I have same problem, sentry always alarm it.

JexPY commented 4 years ago

Same situation with sentry, but this is happening after update from Laravel 5.8 to 6.0

huesoamz commented 4 years ago

@lamalamaMark the fix of @zgelici its very useful why dont accept that PR?

gutitrombotto commented 4 years ago

Hi guys!

I have te same problem.

Did you find the solution?

wlepinski commented 4 years ago

I just had the same problem today. And I think it is related to the rate limiters defined here.

Usage limits This endpoint is limited to 45 requests per minute from an IP address.

If you go over the limit your requests will be throttled (HTTP 429) until your rate limit window is reset. If you constantly go over the limit your IP address will be banned for 1 hour.

The returned HTTP header X-Rl contains the number of requests remaining in the current rate limit window. X-Ttl contains the seconds until the limit is reset. Your implementation should always check the value of the X-Rl header, and if its is 0 you must not send any more requests for the duration of X-Ttl in seconds.

We do not allow commercial use of this endpoint. Please see our pro service for SSL access, unlimited queries and commercial support.

While the PR proposed solves the problem with the non-object error it does not solve the problem of going above the limit on a non-paid environment.

hosamalzagh commented 1 year ago

same error