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

PHP Fatal error when a variable is missing from operation name and dictionary of values #238

Open
leoloso opened this issue Jan 10, 2020 · 12 comments · May be fixed by #239 or #246
Open

PHP Fatal error when a variable is missing from operation name and dictionary of values #238

leoloso opened this issue Jan 10, 2020 · 12 comments · May be fixed by #239 or #246
Labels

Comments

@leoloso
Copy link

leoloso commented Jan 10, 2020

I get the following exception:

PHP Fatal error:  Uncaught Error: Call to a member function hasDefaultValue() on null in .../vendor/youshido/graphql/src/Execution/Request.php:69

When executing a query which includes a variable, but:

  1. The variable was not defined in the operation name, AND
  2. The variable value was not passed in the dictionary

Click on this link and run the query to reproduce the error (it doesn't show the PHP exception error though, only that there was an internal server error)

@Jalle19
Copy link
Collaborator

Jalle19 commented Jan 12, 2020

Can you provide the full stacktrace?

@Jalle19 Jalle19 added the bug label Jan 12, 2020
@leoloso
Copy link
Author

leoloso commented Jan 12, 2020

[12-Jan-2020 10:00:26 UTC] PHP Warning:  array_key_exists() expects parameter 2 to be array, null given in /Users/leo/Sites/PoPAPIWP/vendor/youshido/graphql/src/Execution/Request.php on line 66
[12-Jan-2020 10:00:26 UTC] PHP Fatal error:  Uncaught Error: Call to a member function hasDefaultValue() on null in /Users/leo/Sites/PoPAPIWP/vendor/youshido/graphql/src/Execution/Request.php:69
Stack trace:
#0 /Users/leo/GitRepos/GitHub/Libraries/getpop/api-graphql-query/src/Schema/GraphQLQueryConvertor.php(255): Youshido\GraphQL\Execution\Request->__construct(Array, NULL)
#1 /Users/leo/GitRepos/GitHub/Libraries/getpop/api-graphql-query/src/Schema/GraphQLQueryConvertor.php(40): PoP\GraphQLAPIQuery\Schema\GraphQLQueryConvertor->parseAndCreateRequest('# query {\n#   _...', NULL)
#2 /Users/leo/GitRepos/GitHub/Libraries/getpop/api-graphql-request/src/Hooks/VarsHooks.php(60): PoP\GraphQLAPIQuery\Schema\GraphQLQueryConvertor->convertFromGraphQLToFieldQuery('# query {\n#   _...', NULL)
#3 /Users/leo/GitRepos/GitHub/Libraries/getpop/api-graphql-request/src/Hooks/VarsHooks.php(30): PoP\GraphQLAPIRequest\Hooks\VarsHooks->processURLParamVars(Array)
#4 /Users/leo/Sites/PoPAPIWP/wp/wp-includes/class-wp-hook.php(288): PoP\GraphQLAPIRequest\Hooks\VarsHooks->addURLPar in /Users/leo/Sites/PoPAPIWP/vendor/youshido/graphql/src/Execution/Request.php on line 69

My code, from which the error happens, is this line:

$request = new Request($parsedData, $variables);

from this code in my library (not Youshido):

protected function parseAndCreateRequest($payload, $variables = []): Request
{
    if (empty($payload)) {
        throw new InvalidArgumentException($this->translationAPI->__('Must provide an operation.'));
    }

    $parser  = new Parser();
    $parsedData = $parser->parse($payload);
    $request = new Request($parsedData, $variables);

    // If the validation fails, it will throw an exception
    (new RequestValidator())->validate($request);

    // Return the request
    return $request;
}

@Jalle19
Copy link
Collaborator

Jalle19 commented Jan 12, 2020

Try setting the default value of $variables to null instead of [].

@leoloso
Copy link
Author

leoloso commented Jan 12, 2020

I just did, it still fails, same exception

@leoloso
Copy link
Author

leoloso commented Jan 12, 2020

Please notice that if you pass a variables dictionary including the variable date it doesn't throw an exception. It only does when variable date is not present in the dictionary (and not defined in the operation name)

@Jalle19
Copy link
Collaborator

Jalle19 commented Jan 12, 2020

I have to try reproducing on my own, it should fail with a better error, like "Variable $date not submitted".

@leoloso
Copy link
Author

leoloso commented Jan 12, 2020

Currently it fails with a better error when either the variable is defined in the operation name but not in the variables, or the other way around. Only when both are missing it throws an exception.

I believe it should be straightforward to reproduce, since there's nothing special in my code.

Jalle19 pushed a commit that referenced this issue Jan 13, 2020
@Jalle19 Jalle19 linked a pull request Jan 13, 2020 that will close this issue
@Jalle19
Copy link
Collaborator

Jalle19 commented Jan 13, 2020

I fixed it in #239. It will still error out of course, but the exception will come from the request validator instead of the parser.

@leoloso
Copy link
Author

leoloso commented Apr 2, 2020

This problem can be fixed by checking in Execution/Request.php, function __construct, that the variable that produces the exception is not null.

Currently:

$variable = $ref->getVariable();
if ($variable->hasDefaultValue()) {

Fix:

$variable = $ref->getVariable();
if (!is_null($variable) && $variable->hasDefaultValue()) {

If it is null, then the ensuing error message "Variable %s hasn't been submitted" is good enough (at least for the time being). Would you agree? Should I submit a PR? (I need to fix it, since this problem is happening in my PROD)

@leoloso leoloso closed this as completed Apr 4, 2020
@leoloso
Copy link
Author

leoloso commented Apr 4, 2020

Ops, I closed by mistake.

This branch in my fork handles this solution a bit better:

https://github.com/getpop/graphql-parser/tree/fix-exception-when-no-args-declared

@leoloso leoloso reopened this Apr 4, 2020
@Jalle19
Copy link
Collaborator

Jalle19 commented Apr 8, 2020

Feel free to make a PR. I don't remember what the issue was with my previous attempt anymore, I got a feeling it's not as easy to fix as it sounds like. Maybe your solution works better though.

@leoloso
Copy link
Author

leoloso commented Apr 12, 2020

My solution is simple, because it notices that, if the variable was not defined in the operation, then this line will return null:

$variable = $ref->getVariable();

Maybe this could be handled more elegantly, but this solution, at least, avoids the RuntimeException that was breaking my server.

I'm submitting the PR now

@leoloso leoloso linked a pull request Apr 12, 2020 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants