Skip to content

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

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

Merged
merged 1 commit into from
May 12, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@ CHANGELOG
* Deprecate the `session.storage` alias and `session.storage.*` services, use the `session.storage.factory` alias and `session.storage.factory.*` services instead
* Deprecate the `framework.session.storage_id` configuration option, use the `framework.session.storage_factory_id` configuration option instead
* Deprecate the `session` service and the `SessionInterface` alias, use the `Request::getSession()` or the new `RequestStack::getSession()` methods instead
* Added `AbstractController::renderForm()` to render a form and set the appropriate HTTP status code
* Added support for configuring PHP error level to log levels
* Added the `dispatcher` option to `debug:event-dispatcher`
* Added the `event_dispatcher.dispatcher` tag
* Added `assertResponseFormatSame()` in `BrowserKitAssertionsTrait`
* Add `AbstractController::renderForm()` to render a form and set the appropriate HTTP status code
* Add support for configuring PHP error level to log levels
* Add the `dispatcher` option to `debug:event-dispatcher`
* Add the `event_dispatcher.dispatcher` tag
* Add `assertResponseFormatSame()` in `BrowserKitAssertionsTrait`
* Add support for configuring UUID factory services
* Add tag `assets.package` to register asset packages
* Add support to use a PSR-6 compatible cache for Doctrine annotations
Expand Down
31 changes: 22 additions & 9 deletions src/Symfony/Bundle/FrameworkBundle/Controller/AbstractController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\Form\FormFactoryInterface;
use Symfony\Component\Form\FormInterface;
use Symfony\Component\Form\FormView;
use Symfony\Component\HttpFoundation\BinaryFileResponse;
use Symfony\Component\HttpFoundation\Exception\SessionNotFoundException;
use Symfony\Component\HttpFoundation\JsonResponse;
Expand Down Expand Up @@ -267,21 +268,33 @@ protected function render(string $view, array $parameters = [], Response $respon
}

/**
* Renders a view for a form.
* Renders a view and sets the appropriate status code when a form is listed in parameters.
*
* The FormView instance is passed to the template in a variable named
* "form" (can be changed via $formVar argument).
* If the form is invalid, a 422 status code is returned.
* If an invalid form is found in the list of parameters, a 422 status code is returned.
*/
protected function renderForm(string $view, FormInterface $form, array $parameters = [], Response $response = null, string $formVar = 'form'): Response
protected function renderForm(string $view, array $parameters = [], Response $response = null): Response
{
$response = $this->render($view, [$formVar => $form->createView()] + $parameters, $response);
if (null === $response) {
$response = new Response();
}

foreach ($parameters as $k => $v) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it could be better to pass explicitly the form object since it can save us from cycling all the parameters.

<?php
    protected function renderForm(string $view, FormInterface $form, array $parameters = [], Response $response = null): Response

Moreover, it improves simplest cases: instead of calling $this->renderForm('view.html.twig', ['form' => $form) you can use this->renderForm('view.html.twig', $form)

Copy link
Contributor

Choose a reason for hiding this comment

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

@garak this was discussed on other PR.
There are 2 problems with this approach:

  1. It makes less obvious what is variable name of form in view.
  2. Doesn't work when have more than 1 form. (Typical use-case: Profile editing: 1 form for simple fields (name), second for for phone change, 3rd form for email change.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's exactly what this PR iterates over, see patch. Iterating makes the helper compatible with controllers that have multiple forms as parameters, and saves from creating any convention to name that $form parameter inside the list of $parameters (see removed $formVar)

if ($v instanceof FormView) {
throw new \LogicException(sprintf('Passing a FormView to "%s::renderForm()" is not supported, pass directly the form instead for parameter "%s".', get_debug_type($this), $k));
}

if ($form->isSubmitted() && !$form->isValid()) {
$response->setStatusCode(422);
if (!$v instanceof FormInterface) {
continue;
}

$parameters[$k] = $v->createView();

if (200 === $response->getStatusCode() && $v->isSubmitted() && !$v->isValid()) {
$response->setStatusCode(422);
}
}

return $response;
return $this->render($view, $parameters, $response);
}

/**
Expand Down
8 changes: 4 additions & 4 deletions src/Symfony/Bundle/FrameworkBundle/Tests/Controller/AbstractControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,15 +420,15 @@ public function testRenderFormNew()
$form->expects($this->once())->method('createView')->willReturn($formView);

$twig = $this->getMockBuilder(Environment::class)->disableOriginalConstructor()->getMock();
$twig->expects($this->once())->method('render')->with('foo', ['form' => $formView, 'bar' => 'bar'])->willReturn('bar');
$twig->expects($this->once())->method('render')->with('foo', ['bar' => $formView])->willReturn('bar');

$container = new Container();
$container->set('twig', $twig);

$controller = $this->createController();
$controller->setContainer($container);

$response = $controller->renderForm('foo', $form, ['bar' => 'bar']);
$response = $controller->renderForm('foo', ['bar' => $form]);

$this->assertTrue($response->isSuccessful());
$this->assertSame('bar', $response->getContent());
Expand All @@ -444,15 +444,15 @@ public function testRenderFormSubmittedAndInvalid()
$form->expects($this->once())->method('isValid')->willReturn(false);

$twig = $this->getMockBuilder(Environment::class)->disableOriginalConstructor()->getMock();
$twig->expects($this->once())->method('render')->with('foo', ['myForm' => $formView, 'bar' => 'bar'])->willReturn('bar');
$twig->expects($this->once())->method('render')->with('foo', ['bar' => $formView])->willReturn('bar');

$container = new Container();
$container->set('twig', $twig);

$controller = $this->createController();
$controller->setContainer($container);

$response = $controller->renderForm('foo', $form, ['bar' => 'bar'], null, 'myForm');
$response = $controller->renderForm('foo', ['bar' => $form]);

$this->assertSame(422, $response->getStatusCode());
$this->assertSame('bar', $response->getContent());
Expand Down