Skip to content

Conversation

clemens-tolboom
Copy link
Contributor

Is this a useful answer to #566

@codecov
Copy link

codecov bot commented Apr 30, 2018

Codecov Report

Merging #590 into 8.x-3.x will increase coverage by <.01%.
The diff coverage is 83.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##           8.x-3.x     #590      +/-   ##
===========================================
+ Coverage     80.4%   80.41%   +<.01%     
===========================================
  Files          185      185              
  Lines         2848     2854       +6     
===========================================
+ Hits          2290     2295       +5     
- Misses         558      559       +1
Impacted Files Coverage Δ
src/Controller/RequestController.php 96.77% <83.33%> (-3.23%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 38597d6...571dda7. Read the comment docs.

@fubhy
Copy link
Contributor

fubhy commented May 1, 2018

Yes, we should do this, but this is the wrong place.

If you want to give it another shot, this would be the right place: https://github.com/drupal-graphql/graphql/blob/8.x-3.x/src/GraphQL/Execution/QueryProcessor.php

@clemens-tolboom
Copy link
Contributor Author

Below the feedback from @pmelab #566 (comment)


Thanks @clemens-tolboom !
To merge this, we need to change the approach a little.

  • Errors should be logged on a processor level, since not all requests come from the HTTP endpoint (e.g. graphql_twig).
  • IMO errors should also be logged when not in debug mode. If they happen in production, I'm even more interested what happened 😃
  • Right now when debug mode is off, it will only log "Internal Server Error", which is not helpful.

The GraphQL library already accepts custom error handlers, we just have to use them:
http://webonyx.github.io/graphql-php/error-handling/#custom-error-handling-and-formatting

As far as I can tell they have to be added to the ServerConfig object: https://github.com/drupal-graphql/graphql/blob/8.x-3.x/src/GraphQL/Execution/QueryProcessor.php#L133

Do you have time to look into that? Else I will pick it up at some point in the future 😉

@fubhy
Copy link
Contributor

fubhy commented May 1, 2018

Yeah. The first point that Philipp mentioned is also what I mentioned. "Should be done in the Processor" => https://github.com/drupal-graphql/graphql/blob/8.x-3.x/src/GraphQL/Execution/QueryProcessor.php

And yes, probably with a custom error handler.

I can't work on this in the near future. Feel free to pick it up whenever you have time!

@clemens-tolboom
Copy link
Contributor Author

I cannot take this as I run into lot's of trouble using GraphQL which needs my attention too :-(

@fubhy
Copy link
Contributor

fubhy commented Sep 12, 2018

Continued in #662

@fubhy fubhy closed this Sep 12, 2018
@clemens-tolboom clemens-tolboom deleted the feature/log-errors branch September 12, 2018 13:05
@clemens-tolboom
Copy link
Contributor Author

👍

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 this pull request may close these issues.

2 participants