Revert "Fixed the exceptions"

[GrahamCampbell]

Reverts #410. // cc @sagikazarmark

The other fix in the underlying http library is better. There are still issues here anyway.

Closes #420.

[GrahamCampbell]

That said, maybe we shouldn't revert this, and should roll with both fixes, otherwise the journal can never see our exceptions.

[sagikazarmark]

The Journal is for http related exceptions IMO. Domain exceptions should be handled in your code, unhandled exceptions should be logged by the framework.

[sagikazarmark]

public function addFailure(RequestInterface $request, Exception $exception);

You want to log an HTTP Request and a Domain Exception together?

[Nyholm]

But it would be nice to use the Symfony Profiler page for HTTPlug to see if I received a ApiLimitExceedException.

[GrahamCampbell]

Indeed. Some of our exceptions here aren't exactly very domain targeted.

[Nyholm]

I guess we should add this interface on GitHub exceptions that should be visible in on the profiler page. IE not RuntimeException, InvalidArgumentException or MissingArgumentException etc.

[sagikazarmark]

From a public API point of view, you shouldn't even know the HTTP transport layer is under the hood. If you want to handle limit exceed as part of your domain, then it has nothing to do with the HTTP flow (in fact you will see a client error code, but that's it). If you want to handle it as part of the HTTP flow, implement the Httplug exception on that one, but I think exceptions in general are definitely not HTTP related ones, rather domain specific.

[Nyholm]

From a public API point of view, you shouldn't even know the HTTP transport layer is under the hood.

Agreed.

If you want to handle it as part of the HTTP flow, implement the Httplug exception on that one, but I think exceptions in general are definitely not HTTP related ones, rather domain specific.

Yes. We want to handle it as part of the HTTPflow to be able to show more detailed message. I've created a custom plugin for that.
https://github.com/KnpLabs/php-github-api/blob/master/lib/Github/HttpClient/Plugin/GithubExceptionThrower.php

@GrahamCampbell: Make sure to update those exception related to this plugin with the HTTPlug exception interface.

[cursedcoder]

So waitin for httplug pr to be merged first?

[GrahamCampbell]

Yeh, that upstream library needs fixing for anything to happen.

[GrahamCampbell]

Still needed. The fix that went through was different.

[cursedcoder]

seems you have removed our fork so I can't merge it @GrahamCampbell

[Nyholm]

We should not merge this one. We should decide what exceptions are HTTP exceptions and what exceptions that should abort the flow of plugins.

Only HTTP exceptions should have the HTTPlug exception interface. We are currently blocked by some updates on php-http/client-common if I recall correctly. I'll investigate in this later this week.

[GrahamCampbell]

The correct way to handle this to get domain exceptions would be to catch it outside of the http library stuff.

[sagikazarmark]

The correct way to handle this to get domain exceptions would be to catch it outside of the http library stuff.

Exactly. Domain exceptions should be handled outside of the Plugin chain.