Update ErrorFormatGuesser to handle custom formats and improve guess…

[natepage]

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

Wanted to implement custom error formats based on the format requested by the client, the issue was that whatever the request format the errors were always in the first error format configured. This PR has for purpose to improve the way the ErrorFormatGuesser is guessing the current format from the request and the configuration.

The logic is as following:
1- Check if request format has been set
2- Check if configuration for the format exist and return it if Yes
3- Otherwise guess MIME type from request headers (Accept, Content-Type)
4- Check if configuration for the MIME type exist and return it if Yes
5- Otherwise fallback to first error format configured

Example of configuration:

api_platform:
    mapping:
        paths: ['%kernel.project_dir%/src/Database/Models']
    formats:
        json:     ['application/json']
        xml:      ['application/xml', 'text/xml']
    error_formats:
        custom_error_json: ['application/json']
        custom_error_xml:  ['application/xml', 'text/xml']

Return table

Request format	Accept Header	Content-Type Header	Return
/	/	/	['key' => 'custom_error_xml', 'value' => ['application/xml', 'text/xml']]
custom_error_xml	/	/	['key' => 'custom_error_xml', 'value' => ['application/xml', 'text/xml']]
/	application/json	/	['key' => 'custom_error_json', 'value' => ['application/json']]
/	/	text/xml	['key' => 'custom_error_xml', 'value' => ['application/xml', 'text/xml']]
custom_error_xml	application/json	text/xml	['key' => 'custom_error_xml', 'value' => ['application/xml', 'text/xml']]

The priority order to guess the format is: Request format > Accept Header > Content-Type Header > Fallback to first error format configured.

[natepage]

Travis check failed because their service was down yesterday, can we please restart the tests because it should pass. Thank you.

[dunglas]

Why this change? (isset is faster)

[dunglas]

Because we already loop over formats, you can just store the first format in a var, line 48:

$requestMimeTypes = $request->getMimeTypes($request->getRequestFormat());
$defaultFormat = [];
foreach ($errorFormats as $format => $errorMimeTypes) {
    if ([] === $defaultFormat) {
        $defaultFormat = ['key' => $format, 'value' => $errorMimeTypes];
    }

    if (array_intersect($requestMimeTypes, $errorMimeTypes) {
        return ['key' => $format, 'value' => $errorMimeTypes];
    }
}

return $defaultFormat;

It's faster and safer in case no default format is defined.

[dunglas]

@param and @return can be removed because of the typehints.

[dunglas]

You can remove this method and just call $request->getMimeTypes($request->getRequestFormat()) because the content negotiation is already properly handled here: https://github.com/api-platform/core/blob/master/src/EventListener/AddFormatListener.php#L67

[dunglas]

Sometimes they are several mime types associated with one formats (it's why you should call $request->getMimeTypes()). They all must be checked. See my alternative proposal below.

[dunglas]

Can you also target the master? It's a new feature to me.

[natepage]

@dunglas I've made the changes, thank you for the pieces of code you gave me. I don't know how to target master instead of 2.1, do you want me to create a new PR to master?

[dunglas]

@natepage git rebase master should do the trick.

[meyerbaptiste]

Rebased and RFR @api-platform/core-team.

[soyuka]

what? this looks weird can we change to empty? or !$defaultFormat? Why can't we init above with the proper array?

[dunglas]

Because we not sure the key 0 exist, it looks ok to me to do this. We indeed may use !$defaultFormat.

[soyuka]

I'd be 👍 for !$defaultFormat. Anyway I panicked because [] === [] is false in javascript 😆

[meyerbaptiste]

Fixed.

[dunglas]

Thanks @natepage and @meyerbaptiste!