Fix exceptionToStatus in ApiResource and Operation

[ArnoudThibaut]

Q	A
Tickets	#4848
License	MIT

I open this PR to discuss about the possible solution to fix the exceptionToStatus feature inside ApiResource attribute and operation

Because of Symfony ErrorListener the request is duplicated without all the attributes needed to fetch the exception to status mapping.

A possible solution (the one I choose) could be to extend this listener to override the duplicateRequest method to also duplicate the attributes needed to get the exception to status mapping but I'm not realy satisfied with this solution so I'm really open to suggestions

[alanpoulain]

I don't see a better solution. Was it working in 2.6?

[ArnoudThibaut]

Was it working in 2.6?

I did not find the feature in 2.6

[alanpoulain]

Right, it was introduced by this PR: https://github.com/api-platform/core/pull/3519. Could you target 2.7 then, since it's a bugfix?

[ArnoudThibaut]

Could you target 2.7 then, since it's a bugfix?

It's done

[rvanlaak]

I think we have no other choice than extending the class+service via inheritance here. This could probably be revisited on 3.x, in conjunction with making the ErrorListener more cleanly extensible on Symfony's side.

This approach actually is the right way of doing it. The class gets registered as additional error listener, and basically adds some more data to the response for further processing. As long as we do not set the exception response, the follow-up listener (with a higher priority) will get triggered as well.

So, maybe we can add an integration test to make sure that the listener thereafter still gets triggered as well?

[chalasr]

What I meant is that, as a rule of thumb, decoration should be preferred over inheritance. Especially when it comes to third party code

[rvanlaak]

Ah @chalasr fair point, I did not see that it uses extension. My comment solely focussed on the service registration and related event listener priorities.

Is there a reason why the request needs to get duplicated? Is that because the _api_operation has to be available at a later moment? What about then just lowering the priority of the error listener to before the other listener that makes use of it? That could then also possibly allow removing the extend, as the duplication then isn't needed anymore.

When searching the repo for _api_operation this comment looks promising with relation to priorities ;-) https://github.com/api-platform/core/blob/6ab093e902a3a7b72f023458cd0f9e5f06eb4b50/src/Util/OperationRequestInitiatorTrait.php#L28

[soyuka]

Please check my last commit:

• fixes a typo in the behat tests that used a wrong resource path (dummy_exceptions instead of dummy_exceptions_to_statuses)
• uses service declaration

Let me know what you think of that.

[soyuka]

This is quite ready to be merged but we'll focus on the #4881 first.

[soyuka]

Thanks @ArnoudThibaut

[dannyvw]

@soyuka This feature is not available in 3.0

[soyuka]

You're right, I'll check how to fix this today. It looks like https://github.com/api-platform/core/commit/2d705258978c8c5167356633503d379038d1b602 is on main though but I can't file the files, bad merge on our end?

[dannyvw]

@soyuka Maybe this merge https://github.com/api-platform/core/pull/4910

[rvanlaak]

Looking to https://github.com/api-platform/core/commits/main/src/Symfony/EventListener, it seems that there isn't any commit on main that reverts this change?

[rvanlaak]

For reference, feature seems (re)introduced for branch 3.0 via PR https://github.com/api-platform/core/pull/4981

[jackbentley]

This broke error handling in v2.7 + Symfony 4.4

The fix implemented by #3285 no longer works because the error_listener passed is now always present and never null.

https://github.com/api-platform/core/blob/6b483bc3a2696a908ea4cb68cda0344cfa224146/src/Symfony/Bundle/Resources/config/legacy/api.xml#L95

https://github.com/api-platform/core/blob/6b483bc3a2696a908ea4cb68cda0344cfa224146/src/Symfony/EventListener/ExceptionListener.php#L40