-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[FrameworkBundle] Introduce AbstractController::renderForm()
instead of handleForm()
#41178
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
Conversation
AbstractController::renderForm()
This can be improved. Would you mind sending a PR?
Use the entity-class as type hint and all works seamslessly.
As everything, this very PR is no exception :)
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.
|
I submitted #41181 to improve |
@lyrixx do we need the - return $this->render('thing/new.html.twig', [
+ return $this->renderForm('thing/new.html.twig', $form, [
'thing' => $thing,
'form' => $form->createView(),
]);
} I mean |
Not really,
Sure. I searched for some examplesThe 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 $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');
}
I already talk about that in the examples ☝ but how could you use
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 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
I thought about that too. I saw quite often in application that the maybe something like this :
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()
🤔 you're right! #41184 might help :)
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. |
(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) |
I have removed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/Symfony/Bundle/FrameworkBundle/Controller/AbstractController.php
Outdated
Show resolved
Hide resolved
AbstractController::renderForm()
AbstractController::renderForm()
instead of handleForm()
AbstractController::renderForm()
instead of handleForm()
AbstractController::renderForm()
instead of handleForm()
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 |
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 edit: I just saw that this PR removes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Thank you @lyrixx. |
The blogpost needs an update :) |
What if have 2 forms? Isn't better like that:
|
@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 is the 3rd parameter. 2nd parameter will be array of forms only. |
IMHO, if you have N form (not so commun), you should fallback to the previous method and write the code by hand. |
WDYT of #41190? |
@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 |
…() (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()
$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, [
]);
UpdatedMy mistake; I do redirect but to same page. Sorry. |
@zmitic it's in the PR description:
The validation is donne in |
@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) |
Oh, I was about to use 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. |
@TomasVotruba I'm sorry about this very late change. The
This change was exceptional, and not other promised feature will be gone in 5.3. I hope you understand |
I see. I wish this would happen on internal level before communicating to the public. |
But nothing is internal in the dev process of Symfony. |
Could you link the feedback? I wish I was there too to avoid this :D |
@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 |
@wouterj Thanks 👍 I'll try to write positive feedback on posts I like, maybe it helps next time :) |
I'm know I'm a bit late on this once, but I don't really like the
handleForm()
method:mixed $data
: it's too generic. Static analysis could not work properly and so autocompletion does not work;sed
or similar tool;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. callingisValid()
only check if there is some errors attached to the form.Usage: