Directives from Scalar/Enum/Input/Type/Interface extensions

[LastDragon-ru]

• Added or updated tests
• Documented user facing changes
• Updated CHANGELOG.md
• Closes: extend scalar X directives are lost #2509

Changes

Directives defined in Scalar/Enum/Input/Type/Interface type extensions will be merged into target. The following schema will create type A with two directives @foo and @bar (previously only with @foo).

type A @foo {
  field: String
}

extend type A @bar

Custom scalars seems fine, but for standard (Int/String/etc) the AST node is required. And I'm not sure how it should work.

Technically, it is possible to add scalar Int, but it will not work without @scalar¹. Adding @scalar(class: "GraphQL\Type\Definition\IntType") for standard scalars looks superfluous, IMHO. Moreover despite scalar Int @scalar(class: "My\Custom\Class") is fully valid, but seems useless, because Type::getStandardTypes() has a bigger priority and thus My\Custom\Class will not be used (at least in Schema::getType()).

Another issue - to get directives we need AST node, standard types don't have it, so we probably need to assign it somewhere.

My proposal is to extend TypeRegistry::resolveScalarType() to

• Bind AST node to standard type if defined (required to add directives to standard types via extend Int @mydirective)².
• Allow standard types without @scalar

What do you think?

Also, would be nice if someone can clarify usage of scalar Int @scalar(class: "My\Custom\Class") (allowed/should work or not).

Breaking changes

• Signature of ASTBuilder::missingBaseDefinition() and ASTBuilder::assertExtensionMatchesDefinition() changed to accept ScalarTypeDefinitionNode/ScalarTypeExtensionNode
• if the extend scalar X is used alone (= no scalar X defined it the schema) there will be an error that was not present before. But I think this is ok, because it works the same as for extending other types.

Footnotes

1. Failed to find class Int extends GraphQL\Type\Definition\ScalarType in namespaces [App\GraphQL\Scalars] for the scalar Int. ↩

2. I'm not sure that is will work in all cases, will try to test it tomorrow ↩

[LastDragon-ru]

I'm not sure that is will work in all cases, will try to test it tomorrow

Seems it will not work. So, I don't know how to extend directives for standard types and thus they will be processing like custom scalars. This is means that the following graphql will do nothing (the same as before the PR).

scalar Int @scalar(class: "xxx")
extend scalar Int @directive(location: "extend")

Also, another possible BC: if the extend scalar X is used alone (= no scalar X defined it the schema) there will be an error that was not present before. But I think this is ok, because it works the same as for extending other types.

[LastDragon-ru]

Error: Child process error (exit code 1): ErrorException thrown in /home/runner/work/lighthouse/lighthouse/vendor/laravel/framework/src/Illuminate/Filesystem/Filesystem.php on line 335 while loading bootstrap file /home/runner/work/lighthouse/lighthouse/vendor/larastan/larastan/bootstrap.php: symlink(): File exists

Looks like failed tests are not releated to the PR

[k0ka]

Yep, these tests are randomly failing. Just re-run them.

[spawnia]

Do you have a use cases for changing the standard types? If not, I would rather not spend effort on it.

[LastDragon-ru]

Do you have a use cases for changing the standard types?

Yep, for @searchBy may be useful to extend scalar Int @appComparisonOperator (will add a new operator to Int). But I've added a workaround for it 🤗 (it just move operators into custom scalar).

I would rather not spend effort on it.

Me too. Maybe will be easy to implement it after webonyx/graphql-php#1426 (but I didn't dig into pr)