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

[FrameworkBundle] Add AbstractController::renderBlock() and renderBlockView() #51327

Merged
merged 1 commit into from Aug 18, 2023

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Aug 9, 2023

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

This would be especially useful when generating turbo-stream responses.
Right now, the doc recommends creating new partial templates, but this increases the complexity and scatters HTML fragments. Instead, we could encourage using twig blocks.

Adding this could help remove some boilerplate.

before (using blocks):

return new Response($this->container->get('twig')->load('foo.html.twig')->renderBlock('the_block', $context));

after:

return $this->renderBlock('foo.html.twig', 'the_block', $context);

@stof
Copy link
Member

stof commented Aug 9, 2023

I think it might be more readable with a separate renderBlock method instead of an optional argument, allowing to easily distinguish whether we only render a block of the template and not the full block (especially when the list of variables become long and is defined inline rather than using a $context variable).

@nicolas-grekas
Copy link
Member Author

In order to prevent code duplication, we could add renderBlock but keep the new argument on renderView. Otherwise we'll need a new renderBlockView and it should duplicate most of the code in renderView/render. I'm not keen on adding renderBlockView.

@nicolas-grekas
Copy link
Member Author

Mmm, even adding renderBlock requires duplicating from render.

Another idea might be to use the parameters array to add options, for example ['@block' => 'the_block'].
Can we reference a variable named @block from inside a twig template? If not, then this convention is safe from collisions.
But it's more difficult to discover.

With all this in mind, I think I prefer the additional argument.

@yceruto
Copy link
Member

yceruto commented Aug 9, 2023

I prefer the separate methods approach too, as it is more aligned with SRP and also results in a better argument order.

return $this->renderBlock('foo.html.twig', 'the_block', $context);
An alternative could be moving the duplicated code to new private methods

private function normalizeViewParameters(array $parameters): array
{
    foreach ($parameters as $k => $v) {
        if ($v instanceof FormInterface) {
            $parameters[$k] = $v->createView();
        }
    }

    return $parameters;
}

private function prepareResponse(Response $response, array $parameters, string $content): Response
{
    if (200 === $response->getStatusCode()) {
        foreach ($parameters as $v) {
            if ($v instanceof FormInterface && $v->isSubmitted() && !$v->isValid()) {
                $response->setStatusCode(422);
                break;
            }
        }
    }

    $response->setContent($content);

    return $response;
}

@nicolas-grekas nicolas-grekas changed the title [FrameworkBundle] Add $block argument to AbstractController:render() and renderView() [FrameworkBundle] Add AbstractController::renderBlock() and $block argument to renderView() Aug 9, 2023
@nicolas-grekas
Copy link
Member Author

OK, thanks for the feedback :)

PR updated to add method AbstractController::renderBlock() and argument $block to renderView()

@yceruto
Copy link
Member

yceruto commented Aug 9, 2023

by the way, I would do the same for renderBlockView(), and use normalizeViewParameters() (defined in my previous comment) to avoid duplication. The LogicException is not included as it's a different method, so the message changes.

@yceruto
Copy link
Member

yceruto commented Aug 9, 2023

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Aug 9, 2023

I'm not sold on renderViewBlock. There I still like the additional argument for renderView...

@nicolas-grekas
Copy link
Member Author

Friendly ping @symfony/mergers

@nicolas-grekas nicolas-grekas changed the title [FrameworkBundle] Add AbstractController::renderBlock() and $block argument to renderView() [FrameworkBundle] Add AbstractController::renderBlock() and renderBlockView() Aug 17, 2023
Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

Cool!

Copy link
Member

@lyrixx lyrixx left a comment

Choose a reason for hiding this comment

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

I like it !

@nicolas-grekas nicolas-grekas merged commit 9a0f178 into symfony:6.4 Aug 18, 2023
5 of 9 checks passed
@nicolas-grekas nicolas-grekas deleted the render-block branch August 18, 2023 08:12
This was referenced Oct 21, 2023
kbond added a commit to symfony/ux that referenced this pull request Apr 18, 2024
…reams (nicolas-grekas)

This PR was merged into the 2.x branch.

Discussion
----------

[Turbo] Use blocks instead of partials to render turbo-streams

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  |
| Tickets       | -
| License       | MIT

Relies on symfony/symfony#51327 for the call to `renderBlock()`.

Using `targets` with a CSS selector empowers users with useful knowledge IMHO.

Commits
-------

e8a68ad [Turbo] Use blocks instead of partials to render turbo-streams
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants