Improve order filter generated API 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	-

[alanpoulain]

LGTM. Is it used in the UI?

[fredericbarthelet]

Yep

[teohhanhui]

Actually the order filter parameter could also be used without a value, e.g. GET /books?order[datePublished]

It'd use the default order configured for that property. (I don't like it, but it's something that's supported.)

See Behat test scenario here:

core/features/doctrine/order_filter.feature

Line 244 in cfa8f34

Scenario: Get collection ordered by default configured order on a string property and on which order filter has been enabled in whitelist mode with default descending order

[alanpoulain]

It can be documented like this but could be used without value too, doesn't it?

[teohhanhui]

Enum usually means you must use one of the specified values in the list. Haha...

[teohhanhui]

So, there is allowEmptyValue in OpenAPI v3 (it's also in Swagger v2). We could probably add that for other filters too, where applicable. But perhaps in another PR.

[alanpoulain]

So this PR could be merged as is.

[teohhanhui]

@alanpoulain See the other review comment above.

[dunglas]

@teohhanhui can we merge this one?

[teohhanhui]

Test is missing for the Swagger DocumentationNormalizer.

[fredericbarthelet]

@dunglas @teohhanhui test added in DocumentationNormalizerV3Test

[teohhanhui]

Thanks @fredericbarthelet! 🎉

[dunglas]

Actually, I think that we should deprecate type and required, in favor of schema (because JSON Schema already supports type and required). Using a JSON Schema to describe the filter is smart, and we should only support that. WDYT @teohhanhui?

[teohhanhui]

I think that'll just make a lot of code more complicated...

[dunglas]

Shouldn't we do this test before calling $this->jsonSchemaTypeFactory->getType, in order to not call it when it's useless (when data['schema'] exists)? Also, are the provided schema incompatible with Swagger v2?

[teohhanhui]

We already do useless work like this in many other places in this class. (At least it was the case before. Not sure after your refactoring.)

I guess it's okay in this case as it keeps the code as simple as possible.

Also, are the provided schema incompatible with Swagger v2?

It has no support for schema in parameters.

[dunglas]

We already do useless work like this in many other places in this class. (At least it was the case before. Not sure after your refactoring.)

I tried to improve it as much as possible :)

I guess it's okay in this case as it keeps the code as simple as possible.

It doesn't increase the complexity that much:

                if ($v3 && isset($data['schema'])) {
                    $parameter['schema'] = $data['schema'];
                } else {
                 $type = \in_array($data['type'], Type::$builtinTypes, true) ? $this->jsonSchemaTypeFactory->getType(new Type($data['type'], false, null, $data['is_collection'] ?? false)) : ['type' => 'string'];
                $v3 ? $parameter['schema'] = $type : $parameter += $type;
              }

(it could probably be more simplified btw)

It has no support for schema in parameters.

It is supported: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#fixed-fields-7 (If in is "body":).

[dunglas]

And because it is supported, the code can be simplified a lot:

$type = $data['schema'] ?? \in_array($data['type'], Type::$builtinTypes, true) ? $this->jsonSchemaTypeFactory->getType(new Type($data['type'], false, null, $data['is_collection'] ?? false)) : ['type' => 'string'];
$v3 ? $parameter['schema'] = $type : $parameter += $type;

[teohhanhui]

No, it's not supported. That's for the request body. We're talking about filters here.