AST::fromArray converts DirectiveDefinitionNode->locations from array into NodeList

[zimzat]

After running AST::fromArray on the output of AST::toArray, the DirectiveDefinitionNode->locations goes from an array to a NodeList. The KnownDirectives usage of DirectiveDefinitionNode->locations expects to work with an array so an error is generated instead.

Warning: array_map(): Expected parameter 2 to be an array, object given

graphql-php/src/Language/AST/DirectiveDefinitionNode.php

Lines 24 to 25 in 4f34309

	/** @var NameNode[] */
	public $locations;

graphql-php/src/Utils/AST.php

Lines 104 to 116 in 4f34309

	foreach ($node as $key => $value) {
	if ($key === 'loc' || $key === 'kind') {
	continue;
	}
	if (is_array($value)) {
	if (isset($value[0]) || count($value) === 0) {
	$value = new NodeList($value);
	} else {
	$value = self::fromArray($value);
	}
	}
	$instance->{$key} = $value;
	}

graphql-php/src/Validator/Rules/KnownDirectives.php

Lines 74 to 84 in 4f34309

	foreach ($astDefinition as $def) {
	if (! ($def instanceof DirectiveDefinitionNode)) {
	continue;
	}
	$locationsMap[$def->name->value] = array_map(
	static function ($name) : string {
	return $name->value;
	},
	$def->locations
	);

I'm not sure if the correct fix is to change KnownDirective to use Utils::map, adjust AST::fromArray to ensure it stays as an array, and/or to adjust the creation of DirectiveDefinitionNode to use a NodeList initially.

diffs for exposing the problem and a naive "fix"

The easiest way to expose this problem:

diff --git a/tests/Validator/KnownDirectivesTest.php b/tests/Validator/KnownDirectivesTest.php
index 38c190e..f35e94b 100644
--- a/tests/Validator/KnownDirectivesTest.php
+++ b/tests/Validator/KnownDirectivesTest.php
@@ -5,8 +5,10 @@ declare(strict_types=1);
 namespace GraphQL\Tests\Validator;
 
 use GraphQL\Error\FormattedError;
+use GraphQL\Language\Parser;
 use GraphQL\Language\SourceLocation;
 use GraphQL\Type\Schema;
+use GraphQL\Utils\AST;
 use GraphQL\Utils\BuildSchema;
 use GraphQL\Validator\Rules\KnownDirectives;
 
@@ -17,7 +19,7 @@ class KnownDirectivesTest extends ValidatorTestCase
 
     public function setUp() : void
     {
-        $this->schemaWithSDLDirectives = BuildSchema::build('
+        $this->schemaWithSDLDirectives = BuildSchema::build(AST::fromArray(AST::toArray(Parser::parse('
           directive @onSchema on SCHEMA
           directive @onScalar on SCALAR
           directive @onObject on OBJECT
@@ -29,7 +31,7 @@ class KnownDirectivesTest extends ValidatorTestCase
           directive @onEnumValue on ENUM_VALUE
           directive @onInputObject on INPUT_OBJECT
           directive @onInputFieldDefinition on INPUT_FIELD_DEFINITION
-        ');
+        '))));
     }
 
     private function expectSDLErrors($sdlString, $schema = null, $errors = [])

The easiest immediate "fix" (though having the type change doesn't seem correct?):

diff --git a/src/Validator/Rules/KnownDirectives.php b/src/Validator/Rules/KnownDirectives.php
index 6dbf849..758e861 100644
--- a/src/Validator/Rules/KnownDirectives.php
+++ b/src/Validator/Rules/KnownDirectives.php
@@ -36,6 +36,7 @@ use GraphQL\Language\AST\UnionTypeExtensionNode;
 use GraphQL\Language\AST\VariableDefinitionNode;
 use GraphQL\Language\DirectiveLocation;
 use GraphQL\Type\Definition\Directive;
+use GraphQL\Utils\Utils;
 use GraphQL\Validator\ASTValidationContext;
 use GraphQL\Validator\SDLValidationContext;
 use GraphQL\Validator\ValidationContext;
@@ -76,11 +77,11 @@ class KnownDirectives extends ValidationRule
                 continue;
             }
 
-            $locationsMap[$def->name->value] = array_map(
+            $locationsMap[$def->name->value] = Utils::map(
+                $def->locations,
                 static function ($name) : string {
                     return $name->value;
-                },
-                $def->locations
+                }
             );
         }
 

[spawnia]

I think it should always use a NodeList.

[zimzat]

In looking at creating a pull request for this, I noticed that Parser::implementsInterfaces would have the same scenario occur and the definition for Parser::unionMemberTypes is actually returning NodeList<NamedTypeNode> even though it specified NamedTypeNode[].

• Will this be a problematic backwards compatibility break if I updated each of these?
• Should all of the @var comments on the objects be updated to match the correct types (e.g. NodeList<DirectiveNode> instead of DirectiveNode[]), or would that be too much churn?
  • If it's not, I'd be happy to make those changes [whether in the same or a separate PR].
  • Some of the Node properties are defined as @var DirectiveNode[]|null but, as far as I can tell, the only way it could be null is if the AST::fromArray value doesn't include it when rehydrating the class.
    • Should every @var type include null as a possibility to account for this, be defaulted to [] (some have = [] as their default value) or null, or ...? [if this is too much to change in one go, I can limit these changes to just match what already existed]
• For creating a test, would adding scenarios to the existing ParserTest->testPartiallyParsesSource be sufficient?

Thanks!

[spawnia]

Will this be a problematic backwards compatibility break if I updated each of these?

In practice, few users of the library use those structures at all and much fewer depend on an implementation detail. I myself have always assumed that nodes can be any iterable.

Should all of the @var comments on the objects be updated to match the correct types

That is almost entirely the point of them, so yes.

Some of the Node properties are defined as @var DirectiveNode[]|null

A unified and principled approach would be great, it does not have to be in the same PR as changing from array to NodeList.

For creating a test, would adding scenarios to the existing ParserTest->testPartiallyParsesSource be sufficient?

Sure.