Improve serialization groups filter doc

[fredericbarthelet]

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#2891
License	MIT
Doc PR	-

@dunglas @teohhanhui, I got inspired by existing getDescription function of FilterProperty to add 'enum' information to generated documentation.
https://github.com/api-platform/core/blob/master/src/Serializer/Filter/PropertyFilter.php#L73-L105

However, I was wondering if I should not use your newly created TypeFactory to enrich generated array. I would do that by :

• creating a new Schema in FilterProperty::getDescription method with one unique enum property set to the withlisted serialization groups.
• return description array with a new schema key
• update https://github.com/api-platform/core/blob/master/src/Swagger/Serializer/DocumentationNormalizer.php#L690 to pass optional schema value found in parameter data as last getType argument
• update TypeFactory::getType method to use provided schema to improve returned type (not only for BUILTIN_TYPE_OBJECT as it is the case right now)

What do you think ? This could be ported as well on #2971, which is currently relying on a new Schema property defined without the Schema::class type.

#hackdayparis

[teohhanhui]

The filter's description should not be tied to the serialization formats. As I've tried to explain before in #2971, converting the filter description to whichever appropriate documentation format is the job of the DocumentationNormalizer.

But it might be a good idea to use a Schema object instead of an array. Hmm... Maybe not. It does not actually encapsulate anything.

[dunglas]

I agree with @teohhanhui, the filter should be agnostic of the documentation format (Swagger/OpenAPI/Hydra etc). However, describing it with a JSON Schema looks OK to me. Formats relying on JSON Schema (such as OpenAPI/Swagger) can use it directly, and others (such as Hydra) will have to convert it. As JSON Schema is kind of a standard (an Internet Draft actually, but that should become a RFC at some point), using it looks better to me than creating another custom format of our own.

So the filter should only provide a schema key (instead of swagger and openapi).

[dunglas]

Using a plain array (following the JSON Schema semantic) looks better to me than using a Schema instance (easier for the end user). It's easy to transform an array in a Schema instance in DocumentationNormalizer. We could also support both (array|\ArrayObject, as Schema is just a specialized ArrayObject).

[fredericbarthelet]

@teohhanhui @dunglas understood, no Schema use in getDescription return value.

As of now we than have 2 strategy to "enrich" generated plain array description, that will then be eventually used by the DocumentationNormalizer :

• using swagger and openapi key (like in this PR or for PropertyFilter)
• using schema key (since Improve order filter generated API doc #2971 like for OrderFilter)

Which one do you prefer in order to ensure consistency in this array ?
Should I add a Description object to standardize and ensure getDescription returns are all consistent and avoid this discussion in the future ?

[fredericbarthelet]

@teohhanhui @dunglas I made a small concept PR to show you what I mean by Description object : https://github.com/fredericbarthelet/core/pull/2/files

This would ensure :

• normalization of getDescription output (thus avoiding the above conversation). PHPdoc on the Filter interface did not prevent me from introducing additional keys (which is bad i guess ?)
• cleaner fetching of additional parameters based on current documentation version (swagger or openapi)

WDYT ?

[teohhanhui]

Uhh... I didn't realize we have swagger and openapi keys in the filter's description. 😞

That's bad and should be deprecated.

[fredericbarthelet]

@teohhanhui, noted, I removed swagger and openapi keys in my Description proposal. It is OK then to normalize this output using an object like described in https://github.com/fredericbarthelet/core/pull/2/files ? If so, I will update this PR replacing all occurrences of swagger and openapi keys with new Description object.

[teohhanhui]

No, sorry. My comment is about those keys in the existing implementation.

As for your suggestion of using a class, I don't think we'll do it. (Personally, I'd like to see everything being well-defined classes, but I understand it's not what we'd like to do in this project. That's why you see array $context = [] a lot.)

[fredericbarthelet]

@teohhanhui I updated FilterInterface PHPDoc to reflect decision not to use swagger and openapi keys. I added non-existing schema, items and description keys in the description. I updated my PR to match this "interface" and remove remaining swagger and openapi usage on PropertyFilter.

For my knowledge, what is the advantage of using plain arrays instead of object to describe "options" arguments ? Is something like src/JsonSchema/Schema.php a sort of compromise, allowing for requirement in construction of certain arguments, but leaving free the possibility to add properties not initially defined on the object ?

[fredericbarthelet]

Hi @teohhanhui, I updated my PR with latest changes from master. Did you see my previous message :) ? Is there any remaining modification to be made on this PR ?

[fredericbarthelet]

Hello @teohhanhui, sorry to insist, but could you let me know if this PR is fine for you now :) ?

[teohhanhui]

Sorry, I've somehow missed this one. 🙇‍♂️

I'm going to take a look now.

[teohhanhui]

Suggested change

	* - swagger (optional/deprecated): additional parameters for the path operation
	* - openapi (optional/deprecated): additional parameters for the path operation in the version 3
	* - swagger (optional) (deprecated): additional parameters for the path operation
	* - openapi (optional) (deprecated): additional parameters for the path operation in the version 3

[teohhanhui]

Suggested change

	$description['schema'] = ['type' => 'array', 'items' => ['type' => 'string', 'enum' => $this->whitelist]];
	$description['schema'] = [
	'type' => 'array',
	'items' => [
	'type' => 'string',
	'enum' => $this->whitelist,
	],
	];

[teohhanhui]

We need to trigger deprecation errors instead. This phpdoc is unnecessary.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.