Skip to content

[FrameworkBundle] Add BC layer and FormView handling after #46854 #47600

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[FrameworkBundle] Add BC layer and FormView handling after #46854 #47600

wants to merge 1 commit into from

Conversation

HeahDude
Copy link
Contributor

Q A
Branch? 6.2
Bug fix? yes (potential BC break)
New feature? yes
Deprecations? yes
Tickets #46854 (comment)
License MIT
Doc PR symfony/symfony-docs#16948

#46854 may have introduce a BC break by changing the type of template vars when decorators or templates may expect an instance of FormInterface.
Before the feature was opt-in by using an explicit method renderForm().

Furthermore, this PR improves the feature by handling instances of FormView to change the response status code, enabling the same logic without any required changes in user land code.

We may want to define the new config option as true by default in new projects, I can send a PR on symfony/recipes to do so if this one is accepted.

@carsonbot carsonbot added this to the 6.2 milestone Sep 16, 2022
This was referenced Sep 16, 2022
Closed
Merged
@HeahDude HeahDude force-pushed the feat/render-form-view branch from 5371f91 to 45ca5ed Compare September 17, 2022 07:49
@HeahDude HeahDude changed the title [FrameworkBundle] Add BC layer and FromView handling after #46854 [FrameworkBundle] Add BC layer and FormView handling after #46854 Sep 17, 2022
stof
stof requested changes Sep 17, 2022
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php Outdated
->always()
->then(function ($v) {
if (false === $v) {
trigger_deprecation('symfony/framework-bundle', '6.2', 'Setting the "framework.form.controllers_render_form_views" option to "false" is deprecated. It will have no effect as of Symfony 7.0.');
Copy link
Member

Choose a reason for hiding this comment

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

instead of using ->always() and then doing the special logic only when false, use ->ifFalse() instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my first attempt before doing this, but here's what I get when I try:

Error : Call to undefined method Symfony\Component\Config\Definition\Builder\ExprBuilder::ifFalse()

I could have used ifEmpty() but I preferred to be strict. WDYT, should I change this or should we add ifFalse() to the expression builder?

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php Outdated
->always()
->then(function ($v) {
if (false === $v) {
trigger_deprecation('symfony/framework-bundle', '6.2', 'Setting the "framework.form.controllers_render_form_views" option to "false" is deprecated. It will have no effect as of Symfony 7.0.');
Copy link
Member

Choose a reason for hiding this comment

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

The message should probably be adapted a bit, because it will be confusing when it is not set at all (which will be the case for all existing projects)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, that was a quick copy/past from a previous similar option. Updated, thanks!

@HeahDude HeahDude force-pushed the feat/render-form-view branch from 45ca5ed to 7a491db Compare September 18, 2022 06:38
@HeahDude
Copy link
Contributor Author

Status: needs review

@HeahDude HeahDude force-pushed the feat/render-form-view branch from 7a491db to 867ae2f Compare September 18, 2022 08:14
@chalasr
Copy link
Member

chalasr commented Sep 18, 2022

As this is about a single public method, could we add an optional parameter to that method instead of introducing an option?

@HeahDude
Copy link
Contributor Author

As this is about a single public method, could we add an optional parameter to that method instead of introducing an option?

Well, I believe #46854 was about making the API straightforward. A parameter would make it harder to use this feature seamlessly and globally across controllers in the application.
IMO it would not be worth it to deprecate renderForm() to add a parameter to render().

@chalasr
Copy link
Member

chalasr commented Sep 18, 2022

I get your point, however config is also part of the framework public API. One has to discover and understand that new option.

Ensuring that renderForm() keeps the same behavior as before is a must, but the fact that the suggested alternative behaves differently is definitely not BC break (as you've got time to make your code ready to use that alternative).

My point is that we should try to avoid complicating the framework configuration with feature-specific settings as much as possible. Not everyone uses AbstractController yet here every new project will get that option in their config + will eventually have to remove it at some point.
If the end goal is to make render() always behave the same as this option makes it do, I think it's worth challenging the global config approach. IMHO when the patched code is something that is called directly by the developer as it's the case here, the way to opt-in should be as close as possible to that code, as it makes it more clear what you're adopting.

nicolas-grekas
nicolas-grekas reviewed Sep 18, 2022
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

I think this is unnecessary precautions.
👎 on my side, until a real world issue is reported at least.

@HeahDude HeahDude closed this Sep 18, 2022
@HeahDude HeahDude deleted the feat/render-form-view branch September 18, 2022 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@stof stof stof requested changes

Assignees
No one assigned
Labels
Bug Deprecation Feature FrameworkBundle Status: Needs Review
Projects
None yet
Milestone
6.2
Development

Successfully merging this pull request may close these issues.
5 participants
@HeahDude @chalasr @nicolas-grekas @stof @carsonbot