Add code-first applied directives support for schema printing - #1857
Add code-first applied directives support for schema printing#1857spawnia wants to merge 17 commits into
Conversation
| * subscription?: MaybeLazyObjectType, | ||
| * types?: Types|null, | ||
| * directives?: array<Directive>|null, | ||
| * schemaDirectives?: array<DirectiveNode>|null, |
There was a problem hiding this comment.
Not sure if I like the naming of that one, but we can not simply call it directives like in other schema elements - as that is already taken.
| * @phpstan-type AppliedDirectiveDefinition Schema|Argument|FieldDefinition|InputObjectField|EnumValueDefinition|ScalarType|ObjectType|InterfaceType|UnionType|EnumType|InputObjectType | ||
| * @phpstan-type AppliedDirectiveAstNode SchemaDefinitionNode|SchemaExtensionNode|InputValueDefinitionNode|FieldDefinitionNode|EnumValueDefinitionNode|ScalarTypeDefinitionNode|ScalarTypeExtensionNode|ObjectTypeDefinitionNode|ObjectTypeExtensionNode|InterfaceTypeDefinitionNode|InterfaceTypeExtensionNode|UnionTypeDefinitionNode|UnionTypeExtensionNode|EnumTypeDefinitionNode|EnumTypeExtensionNode|InputObjectTypeDefinitionNode|InputObjectTypeExtensionNode |
There was a problem hiding this comment.
Those types could probably be moved to somewhere else and imported here.
| */ | ||
| private static function collectAppliedDirectives(object $definition, array $excludedDirectiveNames = []): array | ||
| { | ||
| $directives = []; |
There was a problem hiding this comment.
I don't love the pass-by-reference API of filling this, might redesign this to be functional.
| if (! in_array($directive->name->value, $excludedDirectiveNames, true)) { | ||
| $directives[] = $directive; | ||
| } |
There was a problem hiding this comment.
Duplicated with above, might extract.
There was a problem hiding this comment.
Needs tests/specification for conflicts between built-in directives (e.g. deprecationReason + manual @deprecated addition).
| * sortFields?: bool, | ||
| * sortInputFields?: bool, | ||
| * sortTypes?: bool, | ||
| * includeAppliedDirectives?: bool, |
There was a problem hiding this comment.
This option might not be fine-grained enough. Printing extra directives is not necessarily all-or-nothing - depending on the use case (e.g. federation), we might only need to print selected directives. I think that selection is a function of the schema printer - otherwise, we require different schema constructions.
|
@ruudk @simPod @LastDragon-ru First draft, what do you think? I have added some concrete discussion points. Feel free to comment and/or suggest anything else. |
I'm not sure about suggested If I understand correctly, now we have only |
|
Responding to #1857 (comment) by @LastDragon-ru:
Direct instantiation is possible, albeit a bit cumbersome: use GraphQL\Language\AST\ArgumentNode;
use GraphQL\Language\AST\DirectiveNode;
use GraphQL\Language\AST\IntValueNode;
use GraphQL\Language\AST\NameNode;
use GraphQL\Language\AST\NodeList;
$node = new DirectiveNode([
'name' => new NameNode([
'value' => 'directive',
]),
'arguments' => new NodeList([
new ArgumentNode([
'name' => new NameNode([
'value' => 'arg',
]),
'value' => new IntValueNode([
'value' => '123', // IntValueNode stores numeric literal as string
]),
]),
]),
]);I guess this could be simplified through a dedicated type, but that makes it harder to handle directive applications in the printer, given we want to support printing from SDL-first and code-first schemas. A simplified version might look something like this: $applied = new AppliedDirective([
'name' => 'directive',
'arguments' => [
'arg' => 123,
],
]);However, that runs into an issue of distinguishing between string and enum literals. |
Good point. Another point -
Yep. But the current approach also makes it harder. If we have
One more question - will it ( |
# Conflicts: # docs/class-reference.md # src/Type/Definition/CustomScalarType.php # src/Type/Definition/ScalarType.php # src/Utils/SchemaPrinter.php
There was a problem hiding this comment.
Pull request overview
Adds code-first support for applied directives (as DirectiveNode instances) on schema/type/field/arg definitions, and enables optionally printing those applied directives in SDL via SchemaPrinter.
Changes:
- Add
SchemaConfig::$schemaDirectives(+ getters/setter) to support schema-level applied directives in code-first schemas. - Add
directivesconfig/property support (asDirectiveNode[]) to several definition classes (types, fields, args, enum values, input fields). - Add
SchemaPrinteroptionincludeAppliedDirectivesto print applied directives collected from config + AST nodes + extension AST nodes; add tests/docs.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Utils/SchemaPrinterTest.php | Adds coverage for applied-directive printing (off by default, on when enabled, includes AST + extensions). |
| tests/Type/SchemaConfigTest.php | Adds coverage for SchemaConfig::schemaDirectives getter/setter behavior. |
| src/Utils/SchemaPrinter.php | Implements applied-directive collection/printing behind includeAppliedDirectives. |
| src/Type/SchemaConfig.php | Introduces schemaDirectives storage and API on schema config. |
| src/Type/Definition/UnionType.php | Adds directives config/property for unions. |
| src/Type/Definition/ScalarType.php | Adds directives config/property for scalars. |
| src/Type/Definition/ObjectType.php | Adds directives config/property for objects. |
| src/Type/Definition/InterfaceType.php | Adds directives config/property for interfaces. |
| src/Type/Definition/InputObjectType.php | Adds directives config/property for input objects (and removes duplicated @throws). |
| src/Type/Definition/InputObjectField.php | Adds directives config/property for input fields. |
| src/Type/Definition/FieldDefinition.php | Adds directives config/property for field definitions. |
| src/Type/Definition/EnumValueDefinition.php | Adds directives config/property for enum values. |
| src/Type/Definition/EnumType.php | Adds directives config/property for enum types. |
| src/Type/Definition/CustomScalarType.php | Updates config phpdoc to mention directives. |
| src/Type/Definition/Argument.php | Adds directives config/property for arguments. |
| docs/class-reference.md | Documents schemaDirectives and includeAppliedDirectives options/type aliases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return static::printDescription($options, $type) | ||
| . "scalar {$type->name}" | ||
| . static::printSpecifiedBy($type); | ||
| . static::printSpecifiedBy($type) | ||
| . self::printAppliedDirectivesIfEnabled($options, $type); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/Utils/SchemaPrinter.php:410
- When
includeAppliedDirectivesis enabled, scalars that havespecifiedByURLset (often derived from an@specifiedBydirective in the AST) will print@specifiedBy(...)twice: once viaprintSpecifiedBy()and again via applied directives collection. ExcludespecifiedByfrom applied directives whenspecifiedByURLis non-null to avoid duplicate directives in SDL output.
return static::printDescription($options, $type)
. "scalar {$type->name}"
. static::printSpecifiedBy($type)
. self::printAppliedDirectivesIfEnabled($options, $type);
docs/class-reference.md:3106
- This fenced block is marked as PHP (```php) but the
@phpstan-type` ...` lines are PHPDoc annotations, not valid PHP code. With the newly added@phpstan-type` AppliedDirective*` lines the snippet becomes harder to copy/paste. Consider commenting these lines (or switching the code fence language to `text`) so the block content is syntactically valid.
includeAppliedDirectives?: bool,
}
@phpstan-type AppliedDirectiveDefinition Schema|Argument|FieldDefinition|InputObjectField|EnumValueDefinition|ScalarType|ObjectType|InterfaceType|UnionType|EnumType|InputObjectType
@phpstan-type AppliedDirectiveAstNode SchemaDefinitionNode|SchemaExtensionNode|InputValueDefinitionNode|FieldDefinitionNode|EnumValueDefinitionNode|ScalarTypeDefinitionNode|ScalarTypeExtensionNode|ObjectTypeDefinitionNode|ObjectTypeExtensionNode|InterfaceTypeDefinitionNode|InterfaceTypeExtensionNode|UnionTypeDefinitionNode|UnionTypeExtensionNode|EnumTypeDefinitionNode|EnumTypeExtensionNode|InputObjectTypeDefinitionNode|InputObjectTypeExtensionNode
Adds support for printing applied directives from code-first schema definitions in the SchemaPrinter.