Skip to content

Type hinting incoherence for exceptions #22

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

Closed
soullivaneuh opened this issue Nov 8, 2017 · 4 comments · Fixed by #35
Closed

Type hinting incoherence for exceptions #22

soullivaneuh opened this issue Nov 8, 2017 · 4 comments · Fixed by #35

Comments

@soullivaneuh
Copy link
Contributor

Q A
Bug? no
New Feature? no
Version v1.0.1

On the exceptions property:

/**
* @var Exception[]
*/
private $exceptions = [];

The type hinting corresponds to Http\Client\Exception

But on the adder:

/**
* Adds an exception that will be thrown.
*
* @param \Exception $exception
*/
public function addException(\Exception $exception)
{
$this->exceptions[] = $exception;
}

It corresponds to \Exception.

AFAIK, it makes no sense. Which one should be choose?

I may add a PR after getting #21 merged.

@Nyholm
Copy link
Member

Nyholm commented Nov 25, 2017

Using Http\Client\Exception is the correct one since clients MUST NOT throw exceptions that does not implement this interface.

@dbu
Copy link
Contributor

dbu commented Jan 8, 2018

lets change the typehint on the private field, to not lie. but lets also add a comment that according to the spec this must be a http exception. changing the add method would be a bc break, and if people use custom plugins they might deliberately violate the contract, so it seems to me there are use cases for it. want to do a PR @soullivaneuh ?

@soullivaneuh
Copy link
Contributor Author

@dbu In this case, why not trigger a deprecation error if the passed parameter is something else than a Http\Client\Exception instance and move the typing on a next major?

@dbu
Copy link
Contributor

dbu commented Feb 5, 2018

deprecation warning is a good idea, yes.

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.

3 participants