[FrameworkBundle] Introduce `AbstractController::renderForm()` instead of `handleForm()` #41178

lyrixx · 2021-05-11T19:28:30Z

Q	A
Branch?	5.4
Bug fix?	no
New feature?	yes
Deprecations?	yes
Tickets
License	MIT
Doc PR	-

I'm know I'm a bit late on this once, but I don't really like the handleForm() method:

It uses callable and PHP does not support type hint on callable so it's error prone. While trying the feature I forgot to return a response and I got a fatal error "cannot call getStatusCode() on null". Not really user friendly;
callables receive mixed $data: it's too generic. Static analysis could not work properly and so autocompletion does not work;
This is a new syntax to learn;
All documentation, blog post, etc should be updated, and it's not fixable with sed or similar tool;
This is not really flexible. We are going to lock people with this flow, and they will hesitate to use the "old" syntax when they need more flexibility;

That's why I propose this alternative, which is more simple I guess and addresses issues I leveraged.

I read somewhere that calling isValid() trigger twice the validation logic: This is wrong. The validation occurs during form submitting via an event listener. calling isValid() only check if there is some errors attached to the form.

Usage:

     #[Route('/new', name: 'thing_new', methods: ['GET', 'POST'])]
     public function new(Request $request): Response
     {
         $thing = new Thing();
         $form = $this->createForm(ThingType::class, $thing);
 
         $form->handleRequest($request);
         if ($form->isSubmitted() && $form->isValid()) {
             $entityManager = $this->getDoctrine()->getManager();
             $entityManager->persist($thing);
             $entityManager->flush();
 
             return $this->redirectToRoute('thing_index');
         }
 
-        return $this->render('thing/new.html.twig', [
+        return $this->renderForm('thing/new.html.twig', $form, [
             'thing' => $thing,
             'form' => $form->createView(),
         ]);
     }

UPGRADE-5.4.md

nicolas-grekas · 2021-05-11T21:36:28Z

While trying the feature I forgot to return a response and I got a fatal error "cannot call getStatusCode() on null".

This can be improved. Would you mind sending a PR?

callables receive mixed $data: it's too generic. Static analysis could not work properly;

Use the entity-class as type hint and all works seamslessly.

This is a new syntax to learn;
All documentation, blog post, etc should be updated, and it's not fixable with sed or similar tool;

As everything, this very PR is no exception :)

this is not really flexible. We are going to lock people with this flow

How is that not flexible? Which flow is not covered? Can you give some examples please?

About the proposal in this PR itself, it tights twig and forms together, which means it's relies on having only one form per controller. That is less flexible to me.

handleForm() more flexible to me, and requires less logic. That's why I think it's the correct approach.

nicolas-grekas · 2021-05-12T05:30:17Z

I submitted #41181 to improve handleForm()!

OskarStark · 2021-05-12T06:12:50Z

@lyrixx do we need the $form twice?

-        return $this->render('thing/new.html.twig', [
+        return $this->renderForm('thing/new.html.twig', $form, [
             'thing' => $thing,
             'form' => $form->createView(),
         ]);
     }

I mean 'form' => $form->createView(), .... 🤔

lyrixx · 2021-05-12T06:28:32Z

@nicolas-grekas

This is a new syntax to learn;
All documentation, blog post, etc should be updated, and it's not fixable with sed or similar tool;

As everything, this very PR is no exception :)

Not really, handleForm() is a new syntax (or a flow) to learn: IE the flow is different, and updating application will be painful! (I hope to see a rector for that if it's kept). renderForm() is instead a new method that is easy to update to. Updating application could be done in minutes with a simple sed command (or similar). It's the same for documentation.

this is not really flexible. We are going to lock people with this flow

How is that not flexible? Which flow is not covered? Can you give some examples please?

Sure. I searched for handleRequest() in some application I have on my computer where the flow is special. So there is client code that I can not comment about, obviously.

some examples

The data is not validated (1) (I don't know why):

$form = $this->formFactory->create(WhitePaperAccessType::class, $whitePaperAccess);
$form->handleRequest($request);

try {
    $whitePaperAccess->setToken($this->tokenGenerator->generateToken());
    $this->entityManager->persist($whitePaperAccess);
    $this->entityManager->flush();
} catch (ORMException $exception) {
    // Ignore and hide.
}

// [...]
return new RedirectResponse(
    $this->router->generate($route, $routeParams)
);

There is a difference between 1/ new form 2/ form submitted 3/ form invalid (looks like XHR handling)

$form = $this->createForm(ContactType::class, $contact)->handleRequest($request);

if (!$form->isSubmitted()) {
    return $this->render('account/_contact_agent.html.twig', [
        'form' => $form->createView(),
        'agent' => $agent,
    ]);
}

if (!$form->isValid()) {
    return $this->json([
        'success' => false,
        'data' => $this->renderView('account/_contact_agent.html.twig', [
            'form' => $form->createView(),
            'agent' => $agent,
        ]),
    ]);
}

//[...]

return $this->json([
    'success' => true,
    'data' => $this->renderView('account/_contact_agent.html.twig', [
        'form' => false,
        'agent' => $agent,
    ]),
]);

There is many form (the proposed solution won't work either but it is easier to adapt)

$monthlyPaymentsForm = $this->createForm(SimulatorMonthlyPaymentsType::class, $monthlyPaymentDto);
$capacityForm = $this->createForm(SimulatorCapacityType::class, $capacityDto);

$monthlyPaymentsForm->handleRequest($request);
$capacityForm->handleRequest($request);

if ($monthlyPaymentsForm->isSubmitted() && $monthlyPaymentsForm->isValid()) {
    $monthlyPayments = $calculator->computeMonthlyPayments($monthlyPaymentDto);
    $amortizationTable = $calculator->computeAmortizationTable($monthlyPaymentDto);
}

if ($capacityForm->isSubmitted() && $capacityForm->isValid()) {
    $capacity = $calculator->computeCapacity($capacityDto);
    $amortizationTable = $calculator->computeAmortizationTable($capacityDto);
}

return $this->render('tools/investment_simulator.html.twig', [
    'monthlyPaymentsForm' => $monthlyPaymentsForm->createView(),
    'capacityForm' => $capacityForm->createView(),

The data is not validated because it's a search:

$originDestinationSearch = new OriginDestinationSearch();
$form = $this->createForm(OriginDestinationType::class, $originDestinationSearch, [
    'validation_groups' => 'None',
    'entry_type' => CitySelectorType::class,
]);

$form->handleRequest($request);

$parameters = $request->query->all();
$parameters['locale'] = $request->getLocale();

if ($originDestinationSearch->isFulfilled() && !$request->isXmlHttpRequest()) {
    return $this->deeplinkAction($request);
} elseif ($originDestinationSearch->isEmpty()) {
    // $data = [...]
} else {
    // $data = [...]
}

$viewData = [
    'form' => $form->createView(),

use submit() instead of handleRequest()

$formRequest = $this->createForm(ReferrerRequestType::class, $referralProgramRequest);

// We don't use $form->handleRequest because we want to always submit the value, whatever the method is
if (!$formRequest->submit($request->query->all())->isValid()) {
    return $this->redirectToRoute('referral_landing');
}

About the proposal in this PR itself, it tights twig and forms together, which means it's relies on having only one form per controller. That is less flexible to me.

I already talk about that in the examples ☝ but how could you use handleForm() with 2 forms? I fail to see :/. Could you show me please? Because according to the doc and the code, handleForm() must return a response, so which response are you supposed to return? How could you do something only if one form is valid and not the other one ? Or both of same? You can say it's the same for renderForm(), and indeed renderForm() could not work with 2 forms, but it's super easy to mimic what is done in the framework to the application and keep a full control of the flow.

handleForm() more flexible to me, and requires less logic. That's why I think it's the correct approach.

I agree it requires less logic but this logic is well know but I did think we can do better but not this way.

I highly disagree handleForm() it's more flexible. Since you force the flow, I fail to see how it could be flexible. Anyway I posted many example where handleForm() could not work whereas renderForm() could. (I could post much more, but I think it's useless :) )

Finally, @dunglas created the initial PR in order to get a "good" status code when the form is submitted and not valid. And we end up with a totally new syntax for handling a form. We are kinda breaking things here. I don't want to be conservative, but (experience from the field) people don't really like when we change things. And it the case: it solves nothing more than setting the right status code.

I'm sorry if I'm pushing hard against handleForm(). If we decide to close this PR I'll be fine, don't worry. But I think it would be a shame.

@OskarStark

@lyrixx do we need the $form twice?

I thought about that too. I saw quite often in application that the form variable passed to twig is not named form.
We could indeed create the view for the user, and give it to the view but if people want to change the var name ?

maybe something like this :

return $this->renderForm('thing/new.html.twig', $form); // a "form" var is passed to the view
return $this->renderForm('thing/new.html.twig', $form, "another_variable_name"); // a "another_variable_name" var is passed to the view

WDYT?

…() (nicolas-grekas) This PR was merged into the 5.3-dev branch. Discussion ---------- [FrameworkBundle] improve AbstractController::handleForm() | Q | A | ------------- | --- | Branch? | 5.x | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | ( | License | MIT | Doc PR | - Related to #41178 Commits ------- 7c69682 [FrameworkBundle] improve AbstractController::handleForm()

nicolas-grekas · 2021-05-12T07:14:47Z

how could you use handleForm() with 2 forms [...] handleForm() must return a response

🤔 you're right! #41184 might help :)

@dunglas created the initial PR in order to get a "good" status code when the form is submitted and not valid

I agree that renderForm() solves that and reduces the diff.

Thanks for the examples. While not covering 100% of use cases, handleForm() has more use cases than renderForm() to me, especially after #41184! (this PR already triggered some changes, so thanks for it,whatever the outcome).

But I get your arguments. I'd be happy to see what others think about this proposal.

nicolas-grekas · 2021-05-12T07:21:53Z

(my progress of thoughts: if handleForm() has value but renderForm() provides a smoother transition, we might replace handleForm() by renderForm() in 5.3, and reconsider handleForm() or a variant of it for 5.4)

lyrixx · 2021-05-12T07:31:18Z

I have removed handleForm() for now. CF previous comment. (thanks @nicolas-grekas)

nicolas-grekas

This PR basically reverts #40799, which had some arguments that you answered I think.
The patch basically restores #39843, which we could borrow some more code from?

src/Symfony/Bundle/FrameworkBundle/Controller/AbstractController.php

wouterj · 2021-05-12T08:49:29Z

But I get your arguments. I'd be happy to see what others think about this proposal.

Disclaimer: I have very limited knowledge about Symfony forms, from both personal experience and general user experience.

To me, the limited diff of this proposal between "using forms in controllers that don't extend AbstractController" vs "using the AbstractController form helper functions" is a big win over #40799 (users will have to learn less to be able to write complex or default form handling).

wouterj · 2021-05-12T08:53:02Z

What we should think about is the number of alternatives in 5.3. Especially in documentation, you don't want to have choices without clear pros/cons. So if we keep both renderForm(), handleForm() and the pre-5.3 way, we should have very clear pros/cons on when to use each of them imho (otherwise, it's probably wise to remove/deprecate on of them).

edit: I just saw that this PR removes handleForm(). The previously merged changes to that method made me believe we kept both. All OK :)

fabpot

Thank you for the discussion @lyrixx. I agree with most of the things you say and I think you're right about having a simpler helper method. Even if it's late in the process, I'm happy to merge it now and give us more time to revisit this issue for 5.4.

fabpot · 2021-05-12T09:10:02Z

Thank you @lyrixx.

nicolas-grekas · 2021-05-12T09:12:05Z

The blogpost needs an update :)
https://symfony.com/blog/new-in-symfony-5-3-form-handler-helper
/cc @javiereguiluz

Warxcell · 2021-05-12T09:51:17Z

What if have 2 forms? Isn't better like that:

return $this->renderForm('conference/edit.html.twig', ['form' => $form]);
That will simplify the signature because string $formVar = 'form' will be removed, and it will make more obvious the var name of form?

nicolas-grekas · 2021-05-12T09:53:55Z

@Warxcell but the code needs to get access to the $form object in the body of the method. How would it cherry-pick it in the array of $parameters?

Warxcell · 2021-05-12T09:56:07Z

@Warxcell but the code needs to get access to the $form object in the body of the method. How would it cherry-pick it in the array of $parameters?

Parameters is the 3rd parameter. 2nd parameter will be array of forms only.

lyrixx · 2021-05-12T09:57:03Z

IMHO, if you have N form (not so commun), you should fallback to the previous method and write the code by hand.

Warxcell · 2021-05-12T09:58:42Z

Typical use-case for multiple form is "Profile" section, where you have 1 form for name and other simple fields and 1 form just for password changing, 1 form for phone changing, etc.
Even Symfony website uses that use-case:

nicolas-grekas · 2021-05-12T10:26:12Z

WDYT of #41190?

lyrixx · 2021-05-12T10:46:22Z

@Warxcell haha, it's funny you quote SymfonyConnect because I this feature! And as far as I remember, I did not use many forms in the same action, instead I used many render(controller()) (or ESI) 👼🏼

…() (nicolas-grekas) This PR was merged into the 5.3-dev branch. Discussion ---------- [FrameworkBundle] improve AbstractController::renderForm() | Q | A | ------------- | --- | Branch? | 5.x | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Even better than #41178, this requires a simple change on apps, and is compatible with multiple forms. Usage: ```diff - return $this->render('thing/new.html.twig', [ + return $this->renderForm('thing/new.html.twig', [ 'thing' => $thing, - 'form' => $form->createView(), + 'form' => $form, ]); ``` In 5.4, we could even deprecate passing a FormView to render() so that we can always set the 422. Commits ------- e244d31 [FrameworkBundle] improve AbstractController::renderForm()

zmitic · 2021-05-12T16:20:58Z

Wouldn't this trigger validation twice?
Once in AbstractController::renderForm and once in controller itself:

$form->handleRequest($request);
if ($form->isSubmitted() && $form->isValid()) {  // first validation

    // do something but DO NOT redirect      
}

// always render the form; triggers validation again
return $this->renderForm('thing/new.html.twig', $form, [
]);

~~The above is from real use-case I have in turbo+stimulus app. Users can edit some form, but when valid, they get Bootstrap alert and stay on the same page.~~

Updated

My mistake; I do redirect but to same page. Sorry.

jvasseur · 2021-05-12T16:25:55Z

@zmitic it's in the PR description:

I read somewhere that calling isValid() trigger twice the validation logic: This is wrong. The validation occurs during form submitting via an event listener. calling isValid() only check if there is some errors attached to the form.

The validation is donne in handleRequest, not in the isValid method.

lyrixx · 2021-05-12T17:10:12Z

@jvasseur That's what I said, yes 👍🏼 (don't forget that when you comment on a PR, everyone that participate to it receive a notification - And yes, I spamming everyone too :( sorry)

TomasVotruba · 2021-05-16T22:55:18Z

Oh, I was about to use handleForm() in my projects... now I'm bit confused it got promoted as done feature and now removed. What other promised features might be gone in 5.3?

Nette is using similar callable approach since ~2010 and it saves so much repeatead code that we have to obey in Symfony. I hoped Symfony can finally get rid of that boiler plate and lower risk for bugs while improving DX.

lyrixx · 2021-05-17T08:50:26Z

@TomasVotruba I'm sorry about this very late change. The renderForm() method will save some bit of code, and we might find something event better for Symfony 5.4.

What other promised features might be gone in 5.3?

This change was exceptional, and not other promised feature will be gone in 5.3.

I hope you understand

TomasVotruba · 2021-05-17T10:24:25Z

I see.

I wish this would happen on internal level before communicating to the public.
Now it seems there are 2 core Symfony devs who remove mutual features, both good and useful.

nicolas-grekas · 2021-05-17T10:46:43Z

But nothing is internal in the dev process of Symfony.
That's because we get feedback from the community that things improve. Even if the feedback comes as a comment on a blog post.

TomasVotruba · 2021-05-17T13:00:54Z

Could you link the feedback? I wish I was there too to avoid this :D

wouterj · 2021-05-17T13:11:38Z

@TomasVotruba part of the feedback is this PR (note that this PR, like all non-CVE PRs, is not created by "the core team" but by @lyrixx as "Symfony contributor").

Some other feedback can be found in the comments on this article: https://symfony.com/blog/new-in-symfony-5-3-form-handler-helper

TomasVotruba · 2021-05-17T13:22:43Z

@wouterj Thanks 👍
I've read this post and remember positive reactions. Now I can see only "pushing this forward" and "right direction" feedback. Not removing the method completely, that's why I'm surprised it get removed completely.

I'll try to write positive feedback on posts I like, maybe it helps next time :)

lyrixx added Form FrameworkBundle labels May 11, 2021

lyrixx requested a review from dunglas May 11, 2021 19:28

carsonbot changed the title ~~[FrameworkBundle] Introduce AbstractController::renderForm()~~ [Form][FrameworkBundle] Introduce AbstractController::renderForm() May 11, 2021

carsonbot added Status: Needs Review Deprecation Feature labels May 11, 2021

lyrixx changed the title ~~[Form][FrameworkBundle] Introduce AbstractController::renderForm()~~ [Form][FrameworkBundle] Introduce AbstractController::renderForm() May 11, 2021

derrabus reviewed May 11, 2021

View reviewed changes

UPGRADE-5.4.md Outdated Show resolved Hide resolved

lyrixx force-pushed the handle-form branch from 7852349 to 71955fd Compare May 11, 2021 20:31

nicolas-grekas mentioned this pull request May 12, 2021

[FrameworkBundle] improve AbstractController::handleForm() #41181

Merged

nicolas-grekas added this to the 5.3 milestone May 12, 2021

nicolas-grekas removed the Deprecation label May 12, 2021

lyrixx force-pushed the handle-form branch from 71955fd to fd4044a Compare May 12, 2021 07:29

lyrixx force-pushed the handle-form branch from fd4044a to 513ad95 Compare May 12, 2021 07:31

nicolas-grekas reviewed May 12, 2021

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Controller/AbstractController.php Outdated Show resolved Hide resolved

nicolas-grekas changed the title ~~[Form][FrameworkBundle] Introduce AbstractController::renderForm()~~ [Form][FrameworkBundle] Introduce AbstractController::renderForm() instead of handleForm() May 12, 2021

nicolas-grekas removed the Form label May 12, 2021

carsonbot changed the title ~~[Form][FrameworkBundle] Introduce AbstractController::renderForm() instead of handleForm()~~ [FrameworkBundle] Introduce AbstractController::renderForm() instead of handleForm() May 12, 2021

lyrixx force-pushed the handle-form branch from 513ad95 to b003dd8 Compare May 12, 2021 08:12

[FrameworkBundle] Introduce AbstractController::renderForm()

27f8f50

lyrixx force-pushed the handle-form branch from b003dd8 to 27f8f50 Compare May 12, 2021 08:26

chalasr approved these changes May 12, 2021

View reviewed changes

fabpot approved these changes May 12, 2021

View reviewed changes

fabpot mentioned this pull request May 12, 2021

[FrameworkBundle] Introduce AbstractController::renderForm() instead … symfony/symfony-docs#15334

Closed

fabpot merged commit 854226b into symfony:5.x May 12, 2021

nicolas-grekas mentioned this pull request May 12, 2021

[Form][FrameworkBundle] Replace render() with new renderForm() in form documentation symfony/symfony-docs#15217

Merged

lyrixx deleted the handle-form branch May 12, 2021 09:18

nicolas-grekas mentioned this pull request May 12, 2021

[FrameworkBundle] improve AbstractController::renderForm() #41190

Merged

fabpot mentioned this pull request May 12, 2021

Release v5.3.0-BETA4 #41196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FrameworkBundle] Introduce `AbstractController::renderForm()` instead of `handleForm()` #41178

[FrameworkBundle] Introduce `AbstractController::renderForm()` instead of `handleForm()` #41178

lyrixx commented May 11, 2021 •

edited

Loading

nicolas-grekas commented May 11, 2021

nicolas-grekas commented May 12, 2021

OskarStark commented May 12, 2021

lyrixx commented May 12, 2021 •

edited

Loading

nicolas-grekas commented May 12, 2021

nicolas-grekas commented May 12, 2021

lyrixx commented May 12, 2021

nicolas-grekas left a comment

wouterj commented May 12, 2021 •

edited

Loading

wouterj commented May 12, 2021 •

edited

Loading

fabpot left a comment

fabpot commented May 12, 2021

nicolas-grekas commented May 12, 2021

Warxcell commented May 12, 2021 •

edited

Loading

nicolas-grekas commented May 12, 2021 •

edited

Loading

Warxcell commented May 12, 2021

lyrixx commented May 12, 2021

Warxcell commented May 12, 2021 •

edited

Loading

nicolas-grekas commented May 12, 2021

lyrixx commented May 12, 2021 •

edited

Loading

zmitic commented May 12, 2021 •

edited

Loading

jvasseur commented May 12, 2021 •

edited

Loading

lyrixx commented May 12, 2021

TomasVotruba commented May 16, 2021

lyrixx commented May 17, 2021

TomasVotruba commented May 17, 2021

nicolas-grekas commented May 17, 2021

TomasVotruba commented May 17, 2021 •

edited

Loading

wouterj commented May 17, 2021

TomasVotruba commented May 17, 2021

[FrameworkBundle] Introduce AbstractController::renderForm() instead of handleForm() #41178

[FrameworkBundle] Introduce AbstractController::renderForm() instead of handleForm() #41178

Conversation

lyrixx commented May 11, 2021 • edited Loading

nicolas-grekas commented May 11, 2021

nicolas-grekas commented May 12, 2021

OskarStark commented May 12, 2021

lyrixx commented May 12, 2021 • edited Loading

nicolas-grekas commented May 12, 2021

nicolas-grekas commented May 12, 2021

lyrixx commented May 12, 2021

nicolas-grekas left a comment

Choose a reason for hiding this comment

wouterj commented May 12, 2021 • edited Loading

wouterj commented May 12, 2021 • edited Loading

fabpot left a comment

Choose a reason for hiding this comment

fabpot commented May 12, 2021

nicolas-grekas commented May 12, 2021

Warxcell commented May 12, 2021 • edited Loading

nicolas-grekas commented May 12, 2021 • edited Loading

Warxcell commented May 12, 2021

lyrixx commented May 12, 2021

Warxcell commented May 12, 2021 • edited Loading

nicolas-grekas commented May 12, 2021

lyrixx commented May 12, 2021 • edited Loading

zmitic commented May 12, 2021 • edited Loading

Updated

jvasseur commented May 12, 2021 • edited Loading

lyrixx commented May 12, 2021

TomasVotruba commented May 16, 2021

lyrixx commented May 17, 2021

TomasVotruba commented May 17, 2021

nicolas-grekas commented May 17, 2021

TomasVotruba commented May 17, 2021 • edited Loading

wouterj commented May 17, 2021

TomasVotruba commented May 17, 2021

[FrameworkBundle] Introduce `AbstractController::renderForm()` instead of `handleForm()` #41178

[FrameworkBundle] Introduce `AbstractController::renderForm()` instead of `handleForm()` #41178

lyrixx commented May 11, 2021 •

edited

Loading

lyrixx commented May 12, 2021 •

edited

Loading

wouterj commented May 12, 2021 •

edited

Loading

wouterj commented May 12, 2021 •

edited

Loading

Warxcell commented May 12, 2021 •

edited

Loading

nicolas-grekas commented May 12, 2021 •

edited

Loading

Warxcell commented May 12, 2021 •

edited

Loading

lyrixx commented May 12, 2021 •

edited

Loading

zmitic commented May 12, 2021 •

edited

Loading

jvasseur commented May 12, 2021 •

edited

Loading

TomasVotruba commented May 17, 2021 •

edited

Loading