FriendsOfSymfony / FOSRestBundle

This Bundle provides various tools to rapidly develop RESTful API's with Symfony
http://symfony.com/doc/master/bundles/FOSRestBundle/index.html
MIT License
2.79k stars 703 forks source link

fix: Sensio extra bundle TemplateListener invocation before view response listener and serialization #2410

Closed flohw closed 5 months ago

flohw commented 5 months ago

Hi,

As discussed in #2400 and pointed in #2409 (thanks for the bundles.php, not sure I would have found it without a lot of struggles :sweat_smile: ) here is the fix and test.

I relied on the existing redirect test as this existing one and the one you provided trigger the same error. A test idea which could cover more cases (and may be more common) would be that the controller returns an object to be serialized but it would require to add symfony serializer or jms into the loaded bundles and provide the necessary configuration I think.

Let me know if I really should add the new test you provide or a modified one which could return an \stdClass which should be serialized via the view response listener.

Thanks @mbabker

flohw commented 5 months ago

Failing test may be because of deprecation :thinking: Not sure how to handle them. I doubt can-fail: true is the correct answer and we can't remove-sensio-bundle: yes as it's what we want to test ^^

mbabker commented 5 months ago

Just update the deprecation helper config. At some point the test app/bundle configs need some cleanup so the baseline isn't so high, but a lot of it's unavoidable (especially with the Symfony 6.4 builds and the annotations deprecations).

<env name="SYMFONY_DEPRECATIONS_HELPER" value="max[self]=110&amp;max[indirect]=230"/>

That looks to be the right numbers based on the failing builds.

mbabker commented 5 months ago

A test idea which could cover more cases (and may be more common) would be that the controller returns an object to be serialized but it would require to add symfony serializer or jms into the loaded bundles and provide the necessary configuration I think.

IMO it doesn't need to go that in-depth. The functional tests IMO really just need to be in-depth enough to cover the "make sure it doesn't break with SensioFrameworkExtraBundle installed" case and at least one end-to-end scenario with the framework handling a request and ensuring a valid response comes back, the unit tests cover a lot of the internal behaviors much more thoroughly and probably don't need to be re-tested with the functional test cases.

flohw commented 5 months ago

Hi Michael,

Thanks for the env hint, I forgot about it. I am more used to gitlab and our configuration don't fail when the deprecation are raised. I also forget to look at what is tested on unit tests. :facepalm: I agree with you that my idea is not necessary.

I added the functional test and have to raise the max self deprecation due to it. I let the indirect deprecation to its original value as it was higher than your recommendation.

Let me know if I have to set them exactly on the max number I found on the actions (112 and 230)

Thanks for your help.