Resolve ambiguous null interpretation with regards to defaultValues
#262
Conversation
|
I think we should wait for a fix in the reference implementation. |
|
The reference implementation merged the referenced PR in April. |
|
Needs rebase tho |
|
Also, i think spec and reference implementation changed since this PR. Explicit "null" don't sets value to default anymore. |
| /** | ||
| * @it when null value provided directly | ||
| */ | ||
| public function testWhenNullValueProvidedDirectly() |
There was a problem hiding this comment.
| public function testWhenNullValueProvidedDirectly() | |
| public function testWhenNullValueProvidedDirectly() : void |
| /** | ||
| * @it when null value variable provided | ||
| */ | ||
| public function testWhenNullValueVariableProvided() |
There was a problem hiding this comment.
| public function testWhenNullValueVariableProvided() | |
| public function testWhenNullValueVariableProvided() : void |
| fieldWithDefaultArgumentValue(input: $optional) | ||
| }'); | ||
|
|
||
| $this->assertEquals( |
There was a problem hiding this comment.
| $this->assertEquals( | |
| self::assertEquals( |
| fieldWithDefaultArgumentValue(input: null) | ||
| }'); | ||
|
|
||
| $this->assertEquals( |
There was a problem hiding this comment.
| $this->assertEquals( | |
| self::assertEquals( |
|
This looks like an important bugfix, especially considering that the reference implementation is now also fixed: graphql/graphql-js#1274 The spec definitely says that explicit null is not the same as omitting the argument: https://graphql.github.io/graphql-spec/draft/#sec-Null-Value @vladar anything i can do to move this along? |
|
I no longer have interest in fixing this specific issue to ensure it matches the current reference implementation. |
|
@vladar i would be willing to champion this and create a new PR, ok? |
|
@spawnia Yeah, as long as it matches reference implementation and spec, sure. |
|
@spawnia thanks %) |
There is some ambiguity in both the GraphQL spec and various implementations with regards to when
defaultValues are applied to arguments.A spec change to resolve this is being considered and there is a proposed PR for the JS implementation as well.
I have run into this during use of graphql-php where I wanted
defaultValueto be applied for an optional argument (which can't benulleither), but the client used sendsnullas the value for the argument. I expected thedefaultValueto be substituted, butnullwas the coerced value instead.This PR tries to apply a similar fix for the ambiguity in the above PR for the JS implementation, always using the
defaultValuefor a node if the value is explicitly sent asnull(either directly or via variables).