Make sure we catch all exceptions #35

Nyholm · 2016-07-31T16:30:31Z

Q	A
Bug fix?	yes
New feature?	yes
BC breaks?	~~yes~~ no yes
Deprecations?	no
Related tickets	fixes #34, related to #33
Documentation
License	MIT

What's in this PR?

I widening the type hints to catch all exceptions.

Nyholm · 2016-07-31T16:31:51Z

src/Plugin/Journal.php

     */
-    public function addFailure(RequestInterface $request, Exception $exception);
+    public function addFailure(RequestInterface $request, \Exception $exception);


Here is the BC break. Possible solution to avoid this is to introduce a new Journal interface. Could we do something better?

GrahamCampbell · 2016-08-01T08:53:54Z

👍 for merging this, then look at getting throwables to work too after?

sagikazarmark · 2016-08-01T08:57:58Z

Is there a generally accepted way for supporting both PHP 5.x and 7.x? Remove typehint or create a fallback interface, etc?

sagikazarmark · 2016-08-01T08:59:03Z

src/Plugin/Journal.php

     */
-    public function addFailure(RequestInterface $request, Exception $exception);
+    public function addFailure(RequestInterface $request, \Exception $exception);


This is a BC break, you cannot do that. All implementations of this interface will be broken.

Here is the BC break. Possible solution to avoid this is to introduce a new Journal interface. Could we do something better?

(Same as old comment): #35 (comment)

GrahamCampbell · 2016-08-01T09:18:00Z

Is there a generally accepted way for supporting both PHP 5.x and 7.x? Remove typehint or create a fallback interface, etc?

Removing the typehint, or turning the throwable into an actual exception like symfony and laravel do.

GrahamCampbell · 2016-08-01T09:44:48Z

Odd that the StyleCI PR analysis is failing.

sagikazarmark · 2016-08-01T09:45:43Z

I see strange SSH key added emails coming from StyleCI and that was certainly not me.

GrahamCampbell · 2016-08-01T09:47:03Z

Looking at the logs, that's because GitHub is giving us Permission denied (publickey). fatal: Could not read from remote repository. Please make sure you have the correct access rights and the repository exists. when we try to analyze the PR, so the system tries to repair the deploy key.

dbu · 2016-08-02T10:54:48Z

so, ready to merge? @Nyholm can you try to squash the commits and see if we get styleci to also analyze "pr" and not only "push"? tried to restart the build and it said its all good, but i guess that is the "push" and not the "pr". no idea how to make it rebuild "pr"

Nyholm · 2016-08-02T10:59:22Z

I think the PR is ready to be merged.

The PR is squashed and rebased.

dbu · 2016-08-02T11:10:50Z

src/PluginClient.php

@@ -79,7 +78,7 @@ public function sendRequest(RequestInterface $request)
        $pluginChain = $this->createPluginChain($this->plugins, function (RequestInterface $request) {
            try {
                return new FulfilledPromise($this->client->sendRequest($request));
-            } catch (HttplugException $exception) {
+            } catch (\Exception $exception) {


this is a BC break for custom plugins, right? before, their failure callback would accept only Http\Client\Exception, but now it could be called with \Exception. this will lead to a fatal error with parameter mismatch, where before it would be an exception that broke the chain of promises...

i do think its correct to do this, but maybe we need a BC layer for this. maybe an option to the PluginClient for this new behaviour, that is off by default. version 2 can then drop the option and always catch all exceptions.

i don't think this is breaking because the rejected promise class always said that it could take things that weren't http plug exceptions

If one have written an function HttplugException (just like we did in HistoryPlugin) it will fail with this update. But as @GrahamCampbell says, the rejected promise class always said it would be an \Exception.

So: This is a bugfix, others may also suffer from this bug. But this is no BC break.

hm, okay. but if our own classes did not adhere to what we said, we should make sure this will go into a minor version change and that the changelog is clear about it.

So: write ducumentation that explains that only HTTPlug exceptions can be thrown in the plugin. And that's it?

What if a plugin cause a runtime exception? How is that handled? Should the next plugin see it?

Mergy mergy!

The problem is that the used RejectedPromise accepts any kind of exceptions, so if an exception is thrown, the rejected promise will call the reject callback with any exception. IMO there are two/three ways:

Create a RejectedPromise, which only passes http exceptions to the callback, rethrows everything else

When the RejectedPromise is created, make sure only HTTP exceptions are injected into the Promise (although others are possible too) and everything else is thrown immediately (this would require to modify adapter promise implementations too)

Handle the scenario in the plugins: accept any exception and only process http ones, throw the rest (how about no?)

Handle the scenario in the plugins: accept any exception and only process http ones, throw the rest (how about no?)

No, or i do not maintain this xD, it will just be a nightmare to handle (also accepting this, say that we expect people not respecting httplug async standard... )

Create a RejectedPromise, which only passes http exceptions to the callback, rethrows everything else
When the RejectedPromise is created, make sure only HTTP exceptions are injected into the Promise (although others are possible too) and everything else is thrown immediately (this would require to modify adapter promise implementations too)

👍 and we must do the same for the FulfilledPromise, as actually it's accept anything, where we should only allow ResponseInterface

The PR is here php-http/httplug#116

GrahamCampbell · 2016-08-02T11:15:20Z

👍 for merging

cursedcoder · 2016-08-02T11:38:04Z

👍

joelwurtz · 2016-09-01T13:57:18Z

Should we close this as well since #42 has been merged ?

Nyholm · 2016-09-01T14:00:38Z

Correct. Thank you

Nyholm reviewed Jul 31, 2016
View reviewed changes

sagikazarmark reviewed Aug 1, 2016
View reviewed changes

Nyholm mentioned this pull request Aug 1, 2016

Exception handling still broken KnpLabs/php-github-api#420

Closed

sagikazarmark mentioned this pull request Aug 1, 2016

Add specific http promise php-http/httplug#116

Merged

Nyholm force-pushed the issue-34 branch from 56264b1 to 2366128 Compare August 2, 2016 10:57

Make sure we catch all exceptions

705c7c7

Nyholm force-pushed the issue-34 branch from 2366128 to 705c7c7 Compare August 2, 2016 10:58

dbu reviewed Aug 2, 2016
View reviewed changes

joelwurtz mentioned this pull request Aug 31, 2016

Use http based promise in emulated trait and specs #42

Merged

Nyholm closed this Sep 1, 2016

Nyholm deleted the issue-34 branch September 1, 2016 14:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure we catch all exceptions #35

Make sure we catch all exceptions #35

Nyholm commented Jul 31, 2016 •

edited

Loading

Nyholm Jul 31, 2016

GrahamCampbell commented Aug 1, 2016

sagikazarmark commented Aug 1, 2016

sagikazarmark Aug 1, 2016

Nyholm Aug 1, 2016 •

edited

Loading

GrahamCampbell commented Aug 1, 2016

GrahamCampbell commented Aug 1, 2016

sagikazarmark commented Aug 1, 2016

GrahamCampbell commented Aug 1, 2016

dbu commented Aug 2, 2016

Nyholm commented Aug 2, 2016

dbu Aug 2, 2016

dbu Aug 2, 2016

GrahamCampbell Aug 2, 2016

Nyholm Aug 2, 2016

dbu Aug 2, 2016

Nyholm Aug 3, 2016

GrahamCampbell Aug 3, 2016

sagikazarmark Aug 3, 2016

joelwurtz Aug 3, 2016 •

edited

Loading

joelwurtz Aug 7, 2016

GrahamCampbell commented Aug 2, 2016

cursedcoder commented Aug 2, 2016

joelwurtz commented Sep 1, 2016

Nyholm commented Sep 1, 2016

Make sure we catch all exceptions #35

Make sure we catch all exceptions #35

Conversation

Nyholm commented Jul 31, 2016 • edited Loading

What's in this PR?

Choose a reason for hiding this comment

GrahamCampbell commented Aug 1, 2016

sagikazarmark commented Aug 1, 2016

Choose a reason for hiding this comment

Nyholm Aug 1, 2016 • edited Loading

Choose a reason for hiding this comment

GrahamCampbell commented Aug 1, 2016

GrahamCampbell commented Aug 1, 2016

sagikazarmark commented Aug 1, 2016

GrahamCampbell commented Aug 1, 2016

dbu commented Aug 2, 2016

Nyholm commented Aug 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelwurtz Aug 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GrahamCampbell commented Aug 2, 2016

cursedcoder commented Aug 2, 2016

joelwurtz commented Sep 1, 2016

Nyholm commented Sep 1, 2016

Nyholm commented Jul 31, 2016 •

edited

Loading

Nyholm Aug 1, 2016 •

edited

Loading

joelwurtz Aug 3, 2016 •

edited

Loading