Closed downace closed 11 months ago
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
I think we need to consider this a BC break
Agree. Should I mention it somewhere for future changelog?
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?
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.
@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.
@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.
@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.
@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.
@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:
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.
Fixes #622