Changes in 3.1.0 breaking custom flatten exception normalizer

[alexander-schranz]

The change in https://github.com/FriendsOfSymfony/FOSRestBundle/pull/2294

by changing from:

return $data instanceof FlattenException;

to:

return $data instanceof FlattenException && ($context[Serializer::FOS_BUNDLE_SERIALIZATION_CONTEXT] ?? false);

Is breaking currently sulu installations error handling. I'm not sure why this was implemented but would prefer to revert this change. Or when not possible we need to change the behaviour on our side (https://github.com/sulu/sulu/pull/6316). But it is in some case a BC Break as the apis now not return the same errors like before and are not longer handled by the fos rest exception normalizer like before.

/cc @goetas looks for introduced #2294 did now use the normalizer only for serialization errors not for errors in the api itself, which I think should not be the case.

[goetas]

The change in #2294 uses the same strategy as symfony does in https://github.com/symfony/messenger/blob/1cc72a00521c69219ba79b7b04757d5e7fface66/Transport/Serialization/Normalizer/FlattenExceptionNormalizer.php#L98 .

Without that change is practically impossible to use fosrest in the same project where the messenger component is used (the two error handlers mess with each other)

Can you please try to send a PR where the issue you are encountering is reproduced?

[alexander-schranz]

@goetas The problem is that now the FlattenExceptionNormalizer is not called for the exception thrown in controller now if I understand the code correctly, which was the case before.

I will try to create a reproducable but for the fast way you can try it out with the sulu/sulu repository:

git clone git@github.com:sulu/sulu.git
cd sulu
composer update

DATABASE_URL="mysql://root:@127.0.0.1:3306/sulu_test?serverVersion=5.7" bin/runtests -i -t CategoryBundle -C --flags="--filter=CategoryControllerTest::testDeleteWithChildren"

This tests depends on the normalizer be called. With FosRestBundle 3.0.5 it works with 3.1.0 it will fail. As the normalizer is not called.

We are using the following fos_rest configuration: https://github.com/sulu/sulu/blob/74fed7290166a777a35569a4e7d3402854687bc1/src/Sulu/Bundle/CoreBundle/DependencyInjection/SuluCoreExtension.php#L88-L120

[alexander-schranz]

Created a reproducable (Before / After):

git clone git@github.com:FriendsOfSymfony/FOSRestBundle.git
cd FOSRestBundle
git remote add alex git@github.com:alexander-schranz/FOSRestBundle.git
git fetch alex

# this branch is 3.0.5 based so before the changes:
git checkout reproducable/custom-flattenexception-normalizer-3
composer update --prefer-stable

# works:
./phpunit --filter="SerializerErrorTest::testCustomExceptionSerialization"

# this branch is 3.1.0 based so after the changes which cause the issue:
git checkout reproducable/custom-flattenexception-normalizer-2

# fails:
./phpunit --filter="SerializerErrorTest::testCustomExceptionSerialization"

-'{"code":409,"message":"Conflict"}'
+'{"type":"https:\/\/tools.ietf.org\/html\/rfc2616#section-10","title":"An error occurred","status":500,"detail":"Internal Server Error"}'

[goetas]

Why serializer_error_renderer is false?

Setting serializer_error_renderer: true the test is green. Without that option, the default symfony error renderer is used (that does not provide the needed context information).

The backward compatible fix is to change this from

return $data instanceof FlattenException && ($context[Serializer::FOS_BUNDLE_SERIALIZATION_CONTEXT] ?? false);

to

return $data instanceof FlattenException && ($context[Serializer::FOS_BUNDLE_SERIALIZATION_CONTEXT] ?? !$serializerErrorRendererSetting);

[alexander-schranz]

I don't think we want to use the serializer error renderer from fos rest as it seems behave differently to the exist symfony serializer error render which provides more debug informations: https://github.com/symfony/symfony/blob/5.4/src/Symfony/Component/ErrorHandler/ErrorRenderer/SerializerErrorRenderer.php and set some specific headers.

I think the serializer error handler could always set the new context variable and we could go with:

return $data instanceof FlattenException && ($context[Serializer::FOS_BUNDLE_SERIALIZATION_CONTEXT] ?? true);

[goetas]

$context[Serializer::FOS_BUNDLE_SERIALIZATION_CONTEXT] is never false so such solution will not work. People using messenger component will always get the error.

Another option is to look into the MESSENGER_SERIALIZATION_CONTEXT and to opt out if is in the messenger context.

if (! ($data instanceof FlattenException)) {
   return false;
}
// we are in fos rest context
if (!empty($context[Serializer::FOS_BUNDLE_SERIALIZATION_CONTEXT])) {
   return true;
}

// we are in messenger context
if (!empty($context[Serializer::MESSENGER_SERIALIZATION_CONTEXT])) {
   return false;
}

return true;

Would this work?

[alexander-schranz]

@goetas I think that would also work.

[goetas]

@alexander-schranz are you willing to provide a pull request?

[alexander-schranz]

Updated: https://github.com/FriendsOfSymfony/FOSRestBundle/pull/2343

[goetas]

solved in #2343

[alexander-schranz]

Thank you!