Add Debug::RETHROW_UNSAFE_EXCEPTIONS flag

[spawnia]

This allows for a more granular default for which Exceptions are thrown, taking
the settings made through the ClientAware interface into account.

• Add docs
• Add a test case
• Differentiate clearly from other test

[mfn]

But this changes the behaviour? What is the rational for this?

What if I may want/desire the current behaviour and not differentiating internal exceptions?

[spawnia]

The semantics of the current behaviour is off. I do have an Exception that implements `ClientAware` and `isClientSafe() === true`, thus it is seen as not internal. I expect it to not be rethrown. Currently it is. As of now, a more accurate name for the flag would be `RETHROW_EXCEPTIONS_THAT_ARE_NOT_INSTANCES_OF_GRAPHQL_ERROR_ERROR`

[vladar]

@spawnia I'd like to keep it backward compatible. Can you add RETHROW_INTERNAL_UNSAFE_EXCEPTIONS flag instead which would do what you want?

[spawnia]

I assume the point of RETHROW_INTERNAL_EXCEPTIONS is for debugging purposes, to handle exceptions that should not go in the response. This is why it does not rethrow GraphQL\Error\Error instances - it makes sense, as those Exceptions were expected to happen and are rendered to the client output.

Now, the ClientAware interface seems like it should enable Exceptions to behave the same way. Those that are marked as client-safe can be rendered to the output safely, and may be expected runtime errors. They are not "internal", as the $isInternal = ! $e instanceof ClientAware || ! $e->isClientSafe(); line implies.

This is why i think the current behaviour is faulty and what i propose is a valid fix for that bug. It is a change in behaviour, yes, but it only affects a small slice of users, and only if Debugging is enabled. Production use will not break as a result of this.

[vladar]

@spawnia Not really. Internal exception is an exception that is stored in $graphQLError->getPrevious(). I agree that $isInternal variable name was a bit misleading. It should have been named $isUnsafe instead.

To sum up:

• internal exception is any exception thrown in the userland
• unsafe exception is a subset of internal exceptions which shouldn't be displayed to users in production

I guess it makes sense to update variable names accordingly.

[spawnia]

That makes sense. I agree we should get rid of the naming mismatch.

How would i go about adding the new Debug flag RETHROW_INTERNAL_UNSAFE_EXCEPTIONS? Would it be an int = 8 in the Debug class?

[vladar]

Yeah, it should be the next power of 2 (since we use bitwise operations there). In our case it's 8 in the Debug class.

[vladar]

Nice, thank you!