Add support for arguments description

[downace]

Fixes #622

[oojacoboo]

The only thing with this that somewhat concerns me is the potential to expose internals over your graph schema without knowing, and the inability to override any internal param comments through an attribute.

We'd have to modify the Field attribute and add some nested array mess to overcome this, and that API wouldn't be pretty. I'm inclined to ignore this and assume that present docblock comments aren't going to contain sensitive information. But being that this will immediately expose additional comments over the schema, I think we need to consider this a BC break.

That's not a huge issue for the next release, as we already have some somewhat significant changes in master.

[downace]

the inability to override any internal param comments through an attribute

Currently there is similar issue with the Field annotation: only non-empty description on Field overrides docblock description, so #[Field(description: null)] or #[Field(description: '')] will not work.

So maybe all this behaviour of taking description from docblocks should be improved in some future release all at once

[downace]

I think we need to consider this a BC break

Agree. Should I mention it somewhere for future changelog?

[andrew-demb]

But being that this will immediately expose additional comments over the schema, I think we need to consider this a BC break.

I think we can introduce configuration parameter aka "provide argument description" with default to "false", to provide a smooth upgrade path?

[oojacoboo]

Maybe going with a more explicit useDocblockDescriptions makes more sense, where if this is true, it's understood that they're used globally, including for types and other fields. It's always been a bit vague in what you're going to get for comments/descriptions with this lib. Enabling or disabling all of them seems to make more sense, than a setting that toggles some specific piece of that functionality.

By disabling a global setting like this, you can also enforce descriptions being added to attributes, ensuring better schema documentation, as it'll always pertain to the API, and not to any internals. It's all a bit wonky, TBH. I don't like that we have output coming from so many places where it's not always inherently clear.

[oojacoboo]

@downace @andrew-demb did you guys have any thoughts on a global setting for using docblock descriptions? I'd like to see a PR for this and updated docs so we can include this in a new release.

[andrew-demb]

@oojacoboo
I agree with your approach to using useDocblockDescriptions as a global setting.
It allows the use of consistent ways to describe documentation for specific projects with a strict review for public API descriptions.

But I can't provide a PR with docs in the next two weeks, sorry.

[downace]

@oojacoboo
I agree with adding global setting, it should clarify that DocBlock descriptions are actually used by the lib and where they will go in the schema.

But I have some thoughts about using DocBlocks in general

Using the DocBlocks is the most confusing thing for me in this lib.
I think that it should be possible to avoid them entirely. The GraphQL types and API docs should be clearly separated from internal code documentation.
Currently there are some issues with that (e.g. "generic" types, such as Pagination).

[oojacoboo]

@downace I don't disagree. It would be great to have complete separation. Being that this lib was originally written with annotation support, prior to PHP attributes, this wasn't originally possible. Obviously that's changed now and it should be entirely possible. I'm not exactly sure what you mean by Pagination here. But I assume you mean GraphQL list types (array, iterables, etc.). I don't see any reason why the definition for these types couldn't be added to the Field attribute. Are there any other cases where docblocks are actually needed?

The only downside to full separation is doc/meta duplication. Of course, it would be entirely optional. I think it'd be great to include this global setting in conjunction with full attribute support, allowing you to entirely ignore docblocks. A combined PR for this would be awesome.

[downace]

@oojacoboo

I'm not exactly sure what you mean by Pagination here

I mean actual Pagination, using Porpaginas or Laravel Pagination. In our project we also implement the Connections spec.

I don't see any reason why the definition for these types couldn't be added to the Field attribute. Are there any other cases where docblocks are actually needed?

Actually you're right, almost anything can be defined using attributes:

// Simple list type
#[Query(outputType: '[Foo!]!')]
// Pagination
#[Query(outputType: 'PorpaginasResult_Foo')]

But using type names instead of class names looks like an issue for me (not crutial, but still notable).
First, it brings additional overhead to keep type names in sync with corresponding class names. For "generic" types we also have to deal with naming strategy (prefix in the pagination example above) which is not documented.
Second, type definitions are not unified. Somewhere we rely on PHP type hints, somewhere we have to use GraphQL type names for the same object.

It could be solved by allowing to use class names everywhere. Here's pretty rough example (hope the idea is clear):

// Class name can be detected and converted into type name Foo the same way as for PHP type hints
#[Query(outputType: '[' . Foo::class . '!]!')]
// Same here, "generic" types and their arguments can be detected and converted into type names
#[Query(outputType: PorpaginasResult::class . '<' . Foo::class . '>')]

The only downside to full separation is doc/meta duplication

Global setting will give us two options:

• Disable it and duplicate docs
• Enable it and accept the fact that every description from DocBlock will be exposed to the API

Maybe it should be somehow overridable per attribute, like this:

// Global useDocblockDescriptions = true, "blacklist mode"
/**
 * Internal docs, should not be exposed
 */
#[Query(useDocblockDescription: false)]
// or
#[Query(description: 'API docs')]

// Global useDocblockDescriptions = false, "whitelist mode"
/**
 * Internal docs that matches the API docs, exposed explicitly
 */
#[Query(useDocblockDescription: true)]

---

Anyway it's just some thoughts and ideas for future (if maintainers give it a greenlight), I'm not sure I could implement some of these myself soon.