ProviderDataTransformer: throw TransformationFailedException on error

[jorrit]

Subject

A user of my Sonata application tried to submit a heic image in a field that only allows jpeg and png. He received a 500 Internal Server Error caused by an UploadException thrown by ImageProvider.

I found out that ImageProvider->doTransform() is called twice. First by ProviderDataTransformer when the form is transformed to a model. Here the exception is caught and logged.

Later, this method is invoked again by BaseMediaEventSubscriber->prePersist(). This exception is not caught and this causes the 500 Internal Server Error.

ProviderDataTransformer has this comment:

// #1107 We must never throw an exception here.
// An exception here would prevent us to provide meaningful errors through the Form
// Error message inspired from Monolog\ErrorHandler

I don't understand this comment because it just ignores the error and nothing is returned to the form. Also, as mentioned above, the transform method is called again later and the exception occurs anyway.

My solution is to use the TransformationFailedException as suggested by the documentation of DataTransformerInterface:

* @throws TransformationFailedException when the transformation fails

If I replace the log with this exception, a pretty exception is displayed:

I am targeting this branch, because it's backwards compatible.

Possible considerations:

• I convert all exceptions to a TransformationFailedException. I could also only transform UploadExceptions and keep the current code to handle all other exceptions.
• The error messages are not translated. This could be improved because TransformationFailedException supports translations.

Changelog

### Changed

Throw TransformationFailedException when reverse transformation fails in ProviderDataTransformer.

`

[jorrit]

I can't find the button to request a review. Is that because some of the checks fail? The failing checks are not related to my code.

Can someone take a look at this PR? @VincentLanglet @jordisala1991 Thanks!

[VincentLanglet]

This was introduced by #1246 which seems pretty old and was presented as a bugfix by @lemoinem not sure if he can review this.

@jorrit can you confirm that:

• If you remove the try/catch you get a 500
• If you try/catch and throw a TransformationFailedException you get a 200

If so, it means that maybe this was the right fix to do seven years ago ^^'

[jorrit]

I confirm this. I upload a non-image to an image field and get a 500 Internal Server Error. I change the catch block in ProviderDataTransformer to throw new TransformationFailedException($e->getMessage(), 0, $e, $e->getMessage()); and I get a friendly message.

[VincentLanglet]

You can rebase 4.x the fix is build by #2446

[jorrit]

Done, and thanks for fixing the errors. It is always good to see a green checkmark.

[VincentLanglet]

I think this is a BC break.

If someone called setLogger it will crash.

Might be better to keep LoggerAwareInterface and LoggerAwareTrait with a NEXT_MAJOR comment "Remove them".