Replace static variables and statically referenced types with instance based TypeRegistry

[ruudk]

First of all, we ❤️ this amazing library. We've been using it since early 2017 and have 2 large schema's consisting over 1000+ types.

The library sometimes uses static variables to store objects that it creates. This way, the object is instantiated once, and then returned from cache.

graphql-php/src/Utils/Utils.php

Lines 13 to 15 in 86d5a65

	static $undefined;
	return $undefined ??= new \stdClass();

graphql-php/src/Language/Visitor.php

Lines 344 to 346 in 86d5a65

	static $skipNode;
	return $skipNode ??= new VisitorSkipNode();

graphql-php/src/Language/Printer.php

Lines 76 to 77 in 86d5a65

	static $instance;
	$instance ??= new static();

The above examples are fine, because these objects don't hold state that can ever change.

But it becomes a problem when this happens:

graphql-php/src/Type/Definition/Type.php

Lines 114 to 119 in 86d5a65

	static $builtInTypes;
	return $builtInTypes ??= \array_merge(
	Introspection::getTypes(),
	self::getStandardTypes()
	);

Here, the Introspection types (several objects, enums, and referenced built in types) and the standard types are stored statically.

Same here:
https://github.com/webonyx/graphql-php/blob/86d5a659445225c8cbf410ac596090e3ebb0752c/src/Type/Introspection.php#L51C1-L52

And here:

graphql-php/src/Executor/ReferenceExecutor.php

Lines 664 to 667 in 86d5a65

	static $schemaMetaFieldDef, $typeMetaFieldDef, $typeNameMetaFieldDef;
	$schemaMetaFieldDef ??= Introspection::schemaMetaFieldDef();
	$typeMetaFieldDef ??= Introspection::typeMetaFieldDef();
	$typeNameMetaFieldDef ??= Introspection::typeNameMetaFieldDef();

Let's say you have multiple independent schema's. One of the schema's uses a Type::overrideStandardTypes to replace the standard String
type with a custom one. This now becomes a problem depending when the second schema loads. Because it uses statically cached types, they reference to a different String type, resulting in:

Found duplicate type in schema at Viewer.__typename: String. Ensure the type loader returns the same instance. See https://webonyx.github.io/graphql-php/type-definitions/#type-registry. in /Volumes/CS/www/cosmos/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php:878
 
 Caused by
 AssertionError: Found duplicate type in schema at Viewer.__typename: String. Ensure the type loader returns the same instance. See https://webonyx.github.io/graphql-php/type-definitions/#type-registry.
 

This problem pops up when running our test suite. When I run a single test, everything is fine. But when I run multiple tests together, every time the Schema loads, it gets a new instance of our standard String type. Even if I would also statically cache this default type, the problem with also happen when I run 2 tests that both operate on a different schema. Therefore I think the static references are not the way to go.

I tried to create a PR to fix this, but it's a bit hard for me. Some code directly references methods on Introspection that return the cached definitions.

What I would like to see for a next version of this library, is that instead of statically referenced types we switch to an instance based type registry.

For example:

interface TypeRegistry {
  public function (protected callable $typeLoader);
  public function get($typeName) : Type&NamedType;
  public function nonNull($wrapped) : NonNullType;
  public function listOf($wrapped) : ListOfType;
  public function string() : ScalarType;
  public function int() : ScalarType;
  // ... etc
}

class DefaultTypeRegistry implements TypeRegistry {
  protected $resolvedTypes = [];  
  // ...
}

$typeRegistry = new DefaultTypeRegistry(
    typeLoader: function(string $typeName): Type&NamedType {
        return match ($typeName) {
            'Boolean' => new BooleanType(),
            'Float' => new FloatType(),
            'ID' => new IDType(),
            'Int' => new IntType(),
            'String' => new StringType(),
            'Query' => new QueryType(),
            'HelloWorld' => new HelloWorldType(),
        }
    }
);

$schema = new Schema([
    'typeRegistry' => $typeRegistry
]);

Instead of referencing Type::string() to get a StringType, you use $typeRegistry->get('String') that returns that instance.

If needed, we can create specific methods for standard types like $typeRegistry->string().
The Type::listOf and Type::nonNull can also move to this registry: $typeRegistry->listOf($typeRegistry->nonNull($typeRegistry->string())).

The Introspection class becomes an instance within the Schema. The Schema can instantiate it like $this->introspection = new Introspection($this->typeRegistry);.

The statically called Directive also switched to an instance created inside the Schema.

Since every Schema has their own registry, there are no collisions when multiple schema's coexist.

The Type::overrideStandardTypes can be removed, as this is now controlled in your type loader. It will first go to the type loader to load String. It can fallback to a default StringType later.

I also think that Type::getStandardTypes can be removed, as it doesn't really matter if something is standard or not. Just always go through the type loader and cache it inside the instance of the type registry.

[ruudk]

@spawnia if you have time I'd like to hear your thoughts on this. 😊

[shmax]

I tried to create a PR to fix this, but it's a bit hard for me. Some code directly references methods on Introspection that return the cached definitions.

Why not post your PR as a draft? Then you can point out the bits that don't work, and maybe someone will be able to help you work out the kinks...

[ruudk]

@shmax I first want to know if this is a direction to be considered. Then I'd be happy to spend time on it and come up with a PR.

[ruudk]

@shmax @spawnia Created a PR to resolve this issue: #1426