Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Apr 12, 2024

Conversation

flohw
Copy link
Contributor

@flohw flohw commented Apr 9, 2024

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 😅 ) 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
Copy link
Contributor Author

flohw commented Apr 9, 2024

Failing test may be because of deprecation 🤔
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
Copy link
Contributor

mbabker commented Apr 9, 2024

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
Copy link
Contributor

mbabker commented Apr 9, 2024

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
Copy link
Contributor Author

flohw commented Apr 10, 2024

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. 🤦 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.

Copy link

@benconda benconda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you 🙏

@goetas goetas merged commit db7d9a1 into FriendsOfSymfony:3.x Apr 12, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants