Fix handling of errors in mutations #297
Conversation
src/Execution/Executor.php
Outdated
| @@ -114,8 +114,8 @@ public function execute(): ?array | |||
| $result = $operation->getOperation() === 'mutation' | |||
| ? $this->executeFieldsSerially($objectType, $rootValue, $path, $fields) | |||
| : $this->executeFields($objectType, $rootValue, $path, $fields); | |||
| } catch (\Throwable $ex) { | |||
| $this->handleError(new ExecutionException($ex->getMessage(), $fields, null, null, null, null, $ex)); | |||
| } catch (ExecutionException $ex) { | |||
There was a problem hiding this comment.
Now \Throwable and other exceptions won't ever be caught?
There was a problem hiding this comment.
This should be correct, \Throwables are already elsewhere wrapped after ExecutionExceptions.
There was a problem hiding this comment.
What we can do is add another catch clause, just to be sure though. Good point.
crisu83
force-pushed
the
fix-mutation-error-handling
branch
from
2f21b27 to
50003e9
Compare
src/Execution/Executor.php
Outdated
| @@ -217,8 +218,8 @@ public function executeFieldsSerially( | |||
|
|
|||
| $promise->then(function ($resolvedResults) use (&$finalResults) { | |||
| $finalResults = $resolvedResults ?? []; | |||
| })->otherwise(function ($ex) { | |||
| $this->context->addError($ex); | |||
| })->otherwise(function (ExecutionException $ex) { | |||
There was a problem hiding this comment.
What if $ex is not ExecutionException? Do we lose that exception?
There was a problem hiding this comment.
Sorry, clicked the wrong button. I'm also wondering if this is correct?
There was a problem hiding this comment.
You're right, I've fixed this now.
| @@ -379,7 +385,7 @@ protected function executeFields( | |||
| continue; | |||
| } | |||
|
|
|||
| $doesContainPromise = $doesContainPromise || $this->isPromise($result); | |||
| $doesContainPromise = $doesContainPromise || $result instanceof ExtendedPromiseInterface; | |||
There was a problem hiding this comment.
@crisu83 Not sure why you don't like the isPromise method, but we use it in ~3 places at least.
There was a problem hiding this comment.
Because it’s better to use instanceof instead of the @var comments.
There was a problem hiding this comment.
@crisu83 If you insist, feel free to change, but we actually don't really use it in the code.
| @@ -115,7 +114,7 @@ public function execute(): ?array | |||
| ? $this->executeFieldsSerially($objectType, $rootValue, $path, $fields) | |||
| : $this->executeFields($objectType, $rootValue, $path, $fields); | |||
| } catch (\Throwable $ex) { | |||
| $this->handleError(new ExecutionException($ex->getMessage(), $fields, null, null, null, null, $ex)); | |||
| $this->handleError(new ExecutionException($ex->getMessage(), null, null, null, null, null, $ex)); | |||
There was a problem hiding this comment.
@crisu83 not sure why you removed the $fields?
No description provided.