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.

https://github.com/webonyx/graphql-php/blob/86d5a659445225c8cbf410ac596090e3ebb0752c/src/Utils/Utils.php#L13-L15 https://github.com/webonyx/graphql-php/blob/86d5a659445225c8cbf410ac596090e3ebb0752c/src/Language/Visitor.php#L344-L346 https://github.com/webonyx/graphql-php/blob/86d5a659445225c8cbf410ac596090e3ebb0752c/src/Language/Printer.php#L76-L77

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

But it becomes a problem when this happens: https://github.com/webonyx/graphql-php/blob/86d5a659445225c8cbf410ac596090e3ebb0752c/src/Type/Definition/Type.php#L114-L119

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: https://github.com/webonyx/graphql-php/blob/86d5a659445225c8cbf410ac596090e3ebb0752c/src/Executor/ReferenceExecutor.php#L664-L667

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: https://github.com/webonyx/graphql-php/pull/1426