Skip to content

Commit

Permalink
closes #141
Browse files Browse the repository at this point in the history
  • Loading branch information
viniychuk committed Apr 24, 2017
1 parent bdab351 commit 38e7e4e
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 5 deletions.
6 changes: 3 additions & 3 deletions Tests/Library/Validator/RequestValidatorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class RequestValidatorTest extends \PHPUnit_Framework_TestCase
{

/**
* @expectedException Youshido\GraphQL\Exception\Parser\InvalidRequestException
* @expectedException \Youshido\GraphQL\Exception\Parser\InvalidRequestException
* @dataProvider invalidRequestProvider
*
* @param Request $request
Expand Down Expand Up @@ -101,7 +101,7 @@ public function invalidRequestProvider()
'variableReferences' => [
new VariableReference('test', null, new Location(1, 1))
]
])
], ['test' => 1])
],
[
new Request([
Expand All @@ -122,7 +122,7 @@ public function invalidRequestProvider()
new VariableReference('test', $variable1, new Location(1, 1)),
new VariableReference('test2', $variable2, new Location(1, 1))
]
])
], ['test' => 1, 'test2' => 2])
]
];
}
Expand Down
2 changes: 1 addition & 1 deletion Tests/StarWars/StarWarsTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public function testInvalidVariableType()
human(id: $someId) {
name
}
}'
}', ['someId' => 1]
);

$data = $processor->getResponseData();
Expand Down
11 changes: 10 additions & 1 deletion src/Execution/Request.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,16 @@
namespace Youshido\GraphQL\Execution;


use function array_key_exists;
use Youshido\GraphQL\Exception\Parser\InvalidRequestException;
use Youshido\GraphQL\Parser\Ast\ArgumentValue\Literal;
use Youshido\GraphQL\Parser\Ast\ArgumentValue\Variable;
use Youshido\GraphQL\Parser\Ast\ArgumentValue\VariableReference;
use Youshido\GraphQL\Parser\Ast\Fragment;
use Youshido\GraphQL\Parser\Ast\FragmentReference;
use Youshido\GraphQL\Parser\Ast\Mutation;
use Youshido\GraphQL\Parser\Ast\Query;
use Youshido\GraphQL\Parser\Location;

class Request
{
Expand Down Expand Up @@ -63,6 +66,12 @@ public function __construct($data = [], $variables = [])
}

if (array_key_exists('variableReferences', $data)) {
foreach ($data['variableReferences'] as $ref) {
if (!array_key_exists($ref->getName(), $variables)) {
throw new InvalidRequestException(sprintf("Variable %s hasn't been submitted", $ref->getName()), $ref->getLocation());
}
}

$this->addVariableReferences($data['variableReferences']);
}

Expand Down Expand Up @@ -208,7 +217,7 @@ public function setVariables($variables)
}

$this->variables = $variables;
foreach($this->variableReferences as $reference) {
foreach ($this->variableReferences as $reference) {
/** invalid request with no variable */
if (!$reference->getVariable()) continue;
$variableName = $reference->getVariable()->getName();
Expand Down

4 comments on commit 38e7e4e

@Jalle19
Copy link
Collaborator

@Jalle19 Jalle19 commented on 38e7e4e Aug 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks backward compatibility, our application expects that variables will default to null when not specified in the request. Is that a bad assumption?

@fritz-gerneth
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had a discussion about that in #148 #144 about variable defaults and such. But the spec is pretty clear that variables don't have a default value. With my changes on this you can make the variables optional with an default value on the query level though.

@Jalle19
Copy link
Collaborator

@Jalle19 Jalle19 commented on 38e7e4e Aug 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fritz-gerneth okay, I guess we'll update our software not to omit query variables instead then.

@fritz-gerneth
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the recent releases you can easily define optional variables in your query. This way you only have to change the query:

query ($username: String = null){
  reddit {
    user(username: $username) {
      username
      commentKarma
      createdISO
    }
  }
}

This way $username will default to null if not provided.

Please sign in to comment.