API Add Directive types

[marwan38]

This PR is working as intended for my use case and can be merged in. However, I have a note for the maintainers here to make the decision on. I'm currently using this as a patch in our codebase on version 4.13 of this module. I'm hoping that it can be merged in by the time I'm done with our codebase migration and can then bump up to v5 of this module.

SchemaTransciber does not introspect the directives. Therefore users relying on the schema transcriber won't be able to get the full schema including the directives. I can't make the decision on behalf of the framework on how much you want added within the introspection query. I personally don't use it, we generate the schema directly using @graphql-codegen and that introspects what we need. An example of a more comprehensive introspection query can be found here and what the directives field may contain here. That introspect query also adds the complexity of configuration, so I'm not really sure what you guys would want here. As I said, this feature as-is is enough for me at the moment.

Description

Directives are part of the GraphQL specification and this PR adds the Directive type for developers to create custom ones. Ideally, this PR would be followed up (or done alongside) with more integration of directives. One use case i would like to see is adding deprecationReason to to types.

types:
  myType:
    fields: {}
    deprecationReason: Use goodType instead

or if there is any reason to add directives to a field (not sure if this is a valid use case)

types:
  myType:
    fields: {}
    directives:
      deprecated:
        deprecationReason: Use goodType instead

Manual testing steps

A new type is added directives in the graphql schema yaml file.

---
directives:
  myDirective:
    description: 'Optional description'
    args:
      if:
        type: Boolean
        defaultValue: true
    locations: [QUERY]

Viewing the the graphql schema (e.g. using /dev/graphql/ide) should now contain an entry like so:

# Optional description
directive @myDirective(if: Boolean = true) on QUERY

The configuration of the Directive type can be found here https://webonyx.github.io/graphql-php/type-definitions/directives/#custom-directives.

The directive can then be resolved within the resolver like so:

/** @var ResolveInfo $info */
$allDirectives = $info->operation?->directives?->getIterator() ?? [];
foreach ($allDirectives as $directive) {
  var_dump($directive->name->value);
}

Issues

• GraphQL Directives #578

Pull request checklist

• The target branch is correct
  • See picking the right version
• All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
  • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
• The commit messages follow our commit message guidelines
• The PR follows our contribution guidelines
• Code changes follow our coding conventions
• This change is covered with tests (or tests aren't necessary for this change)
• Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
• CI is green

[marwan38]

This probably breaks some PSR specs, I'm not sure if it matters here?

[GuySartorelli]

You say "this" on a comment on an empty line... but I assume you are actually referring to having multiple classes defined in the same file?

That does break PSR1 (see the PSR1 spec).
Ideally each of these templates should be explicitly for a single class.

One approach would be to reuse the same template file (with only one clsas definition) for both of these since they're practically identical, and just add appropriate conditional statements. e.g. type.inc.php uses a lot of conditional logic to build its class.

[GuySartorelli]

Please add a PR to https://github.com/silverstripe/developer-docs as well, adding documentation for this new feature.

The documentation will be necessary for other developers to take advantage of this - but it's also a way I can make sure I'm testing it correctly. If I follow the documentation and things don't work as expected, it's clear something (either the documentation or the implementation) isn't working as expected.

I'm also still not sure what these directives do or what they're for... You've mentioned they're "part of the GraphQL specification" and that they're "a fundamental part of GraphQL" but not much more. Can you please briefly explain what this feature actually is, and why it is useful?

[GuySartorelli]

Why was this change made? It seems unrelated

[GuySartorelli]

You say "this" on a comment on an empty line... but I assume you are actually referring to having multiple classes defined in the same file?

That does break PSR1 (see the PSR1 spec).
Ideally each of these templates should be explicitly for a single class.

One approach would be to reuse the same template file (with only one clsas definition) for both of these since they're practically identical, and just add appropriate conditional statements. e.g. type.inc.php uses a lot of conditional logic to build its class.

[GuySartorelli]

Please add an assertion for the result as well - presumably the directive should add something to the result which we can test for.

[GuySartorelli]

@marwan38 This PR hasn't been updated for a while. Are you still keen for this feature to be included?
If so please address the comments above. If not, I will close this PR.

[marwan38]

@GuySartorelli Yeah, still want it included. I've just not dedicated any time to it just yet. If you prefer to close this PR and bring up a new one when I'm ready; that's fine.

There's a few bad commits in here for various different reasons (my own formatters, debug code, etc..).

[GuySartorelli]

Sweet. Yes, please feel free to open a new PR when you have a chance to work on this - but I'll close it for now.