Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Class 'ipinfo\ipinfo\Exception' not found #1

Closed
dmitriy2018 opened this issue Oct 8, 2018 · 12 comments
Closed

Class 'ipinfo\ipinfo\Exception' not found #1

dmitriy2018 opened this issue Oct 8, 2018 · 12 comments
Assignees

Comments

@dmitriy2018
Copy link

Executing getDetails with fake IP produces fatal error.

$ip = '34535'; $IPInfoClient->getDetails($ip);

Add use \Exception; into IPInfo.php

@coderholic coderholic assigned coderholic and jhtimmins and unassigned coderholic Oct 8, 2018
@rvalitov
Copy link
Contributor

Add in PHPDoc that the function may rise an Exception

@scofield-ua
Copy link

@coderholic

@coderholic
Copy link
Member

@jhtimmins

@jhtimmins
Copy link
Contributor

@coderholic @scofield-ua I'll take a look later today and will fix/close this issue.

@jhtimmins
Copy link
Contributor

@scofield-ua my apologies, I ran into some issues with my computer so I'm having trouble testing this, which may delay me a bit.

In the meantime, could you clarify why it's necessary to import Exceptions from multiple locations?

@scofield-ua
Copy link

scofield-ua commented Jan 9, 2019

@jhtimmins no problem, thanks.

I think the main idea of this issue is to fix the part where you're throwing global Exception class. For example here:

php/src/IPinfo.php

Lines 98 to 100 in 5b360e3

throw new Exception('IPinfo request quota exceeded.');
} elseif ($response->getStatusCode() >= 400) {
throw new Exception('Exception: ' . json_encode([

Right now global Exception class is throwing in local namespace: ipinfo\ipinfo, which is causing fatal error.
Need to use throw new \Exception instead of throw new Exception, or just connect it at the beginning of the class.

In my project I'm currently have to catch Throwable exception.

Also, I think, instead of general global Exception class would be good to throw some custom named exception which is related only for IpInfo package, something like IpInfoException which you can use to throw any kind (or specific kind) of errors.

@rvalitov
Copy link
Contributor

@jhtimmins, the comment of @scofield-ua is absolutely correct. That's what have been done in my PR #5
Additionally I added the correct information about the exceptions in PHPDoc.

@jhtimmins
Copy link
Contributor

@scofield-ua Ah of course, yes that makes sense. I actually thought you submitted the PR with multiple exception types, which should have been directed at the PR author. Thanks for the info.

@rvalitov Could you explain this docblock, and why it includes two exception types?

     * @throws \GuzzleHttp\Exception\GuzzleException
     * @throws \Exception

It doesn't look like \GuzzleHttp\Exception\GuzzleException will ever get thrown directly. Or am I missing something?

@rvalitov
Copy link
Contributor

@jhtimmins
A function definition must include all uncatched exceptions. The exceptions may rise inside the function code or inside other functions that your original function calls. The functions that you call use Guzzle, and Guzzle functions throw \GuzzleHttp\Exception\GuzzleException. You don't catch exceptions from Guzzle therefore you pass them to the user. As a result you have to specify the \GuzzleHttp\Exception\GuzzleException in the PHPDoc.

@jhtimmins
Copy link
Contributor

This was fixed in commit 6068f65.

I've added an IPinfoException class which will get returned instead of the base Exception class or the GuzzleException class.

@Acrash79
Copy link

Acrash79 commented Oct 1, 2020

😊

@UmanShahzad
Copy link
Contributor

If 6068f65 does not resolve this issue, please let me know and we can reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants