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

Open ReferenceExecutor for extending #801

Merged
merged 2 commits into from Mar 29, 2021
Merged

Open ReferenceExecutor for extending #801

merged 2 commits into from Mar 29, 2021

Conversation

4n70w4
Copy link
Contributor

@4n70w4 4n70w4 commented Mar 26, 2021

Case: need overwrite resolveFieldValueOrError without try catch block and handle exception in my own error handler from application. Without this patch, it is impossible to override this behavior.

class MyExecutor extends ReferenceExecutor {



    /**
     * Isolates the "ReturnOrAbrupt" behavior to not de-opt the `resolveField` function.
     * Returns the result of resolveFn or the abrupt-return Error object.
     *
     * @param mixed $rootValue
     *
     * @return Throwable|Promise|mixed
     */
    protected function resolveFieldValueOrError(
        FieldDefinition $fieldDef,
        FieldNode $fieldNode,
        callable $resolveFn,
        $rootValue,
        ResolveInfo $info
    ) {
        // Build a map of arguments from the field.arguments AST, using the
        // variables scope to fulfill any variable references.
        $args         = Values::getArgumentValues(
            $fieldDef,
            $fieldNode,
            $this->exeContext->variableValues
        );
        $contextValue = $this->exeContext->contextValue;

        return $resolveFn($rootValue, $args, $contextValue, $info);
    }



}

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.381% when pulling 8964ff4 on 4n70w4:patch-1 into 65428ce on webonyx:master.

@spawnia
Copy link
Collaborator

spawnia commented Mar 26, 2021

I highly recommend you to not change that behaviour and instead read the GraphQL specification thoroughly and try to understand why error handling in GraphQL works in this particular way: http://spec.graphql.org/draft/#sec-Execution. In short: GraphQL queries are complex and may contain many fields. An error in one field should not abort the entire query.

Regardless of that, I generally think that opening up library code for extensiblity is a good thing. We should apply that consistently throughout the class though.

@4n70w4
Copy link
Contributor Author

4n70w4 commented Mar 26, 2021

I make it not for error in fields. For example I have query:

class Status {

    /**
     * @param  null  $_
     * @param  array<string, mixed>  $args
     */
    public function __invoke($_, array $args) {
        throw new \Exception('dcdcd');
    }

With default ReferenceExecutor I get error response:

{
 "errors": [
   {
     "debugMessage": "dcdcd",
     "message": "Internal server error",
     "extensions": {
       "category": "internal"
     },
     "locations": [
       {
         "line": 2,
         "column": 3
       }
     ],
     "path": [
       "status"
     ],
     "trace": [
       {

I don't know what the format is, this is not my error handler. I want to turn it off.

But with MyExecutor i get:

{
  "errors": [
    {
      "id": "e84880e179a64be98d1f421b83252f23",
      "status": 500,
      "title": "Internal Server Error",
      "detail": "An error has occurred and this resource cannot be displayed."
    }
  ]
}

This is my application's error handler. I need it.

@spawnia
Copy link
Collaborator

spawnia commented Mar 26, 2021

Please read the spec. Your format is not compliant.

@4n70w4
Copy link
Contributor Author

4n70w4 commented Mar 26, 2021

Even Facebook and Instagram display errors in his own format without compliant the spec.
Do I need to remind that grapql was developed by facebook.

@spawnia
Copy link
Collaborator

spawnia commented Mar 27, 2021

Standard tools are not going to work as well when deviating from the specification.

Anyways, concerning this PR: every instance of private in this class should be protected, same with self to static.

@4n70w4
Copy link
Contributor Author

4n70w4 commented Mar 29, 2021

Okay, I change every instance of private to protected, and self to static.

A little about my case. I am using only a small part of the graphQL capabilities for one of the projects. First of all, the schema as a contract and documentation for developers.

All my graphQL requests are stored in the repository, they are verified, tested and approved. The server is under authorization. Requests to it only from trusted services. All queries always return only 1 or 0 entities by ID. The entity either returns completely or does not return. There are no other types of requests and are not planned.

GraphQL is no more than 5% of the code and traffic from the entire volume of the projects. It is more important to have the same error handling system for all projects and components.

During the 2 years of the project's existence, a graphQL spec error format has never been required and has not been useful.

@4n70w4
Copy link
Contributor Author

4n70w4 commented Mar 29, 2021

I suppose this line stay as private.

@spawnia spawnia changed the title Able to extends ReferenceExecutor and overwrite resolveFieldValueOrError method Open ReferenceExecutor for extending Mar 29, 2021
@spawnia spawnia merged commit c19cd7c into webonyx:master Mar 29, 2021
@4n70w4
Copy link
Contributor Author

4n70w4 commented Mar 29, 2021

@spawnia Thanks. It would be very nice to make a release.
To update the dependency through the composer.
The last release was almost 2 months ago.
Many changes have accumulated.

spawnia pushed a commit that referenced this pull request Mar 29, 2021
@spawnia
Copy link
Collaborator

spawnia commented Mar 29, 2021

Released with https://github.com/webonyx/graphql-php/releases/tag/v14.6.0

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

Successfully merging this pull request may close these issues.

None yet

3 participants