twigphp
Add the render function, deprecate the include one
Maybe it would be good to also deprecate the include tag in 3.15.
My only "concern" is about the embed tag that is kind of a glorified include node behind the scene.
@fabpot wanted to give some feedback. I really like the new include decision without context by default. Even I'm not a fan of renaming it render, but I understand from BC reasons for better transparency. I'm working the last two years where we only use with_context = false to make things more transparent what variables are accessed by components. Only case which was confusing is the embed tag which we use for things like overlays, fieldsets, ...
Typically looks like this:
{# overlay.html.twig #}
<dialog>
<h3>
{% block title %}{% endblock %}
</h3>
<div>
{% block content %}{% endblock %}
</div>
</dialog>
{# using.html.twig #}
{% set title = 'Title' %}
{% set content = 'Content' %}
{% embed "overlay.html.twig" only %}
{% block title %}
{{ title }} {# not available #}
{% endblock %}
{% block content %}
<div>
{{ content }} {# not available #}
</div>
{% endblock %}
{% endembed %}
To get this work I need give the variables also to overlay.html.twig which may have similar variables defined.
{# using.html.twig #}
{% set title = 'Title' %}
{% set content = 'Content' %}
{% embed "overlay.html.twig" {title: title, content: content} only %}
{% block title %}
{{ title }}
{% endblock %}
{% block content %}
<div>
{{ content }}
</div>
{% endblock %}
{% endembed %}
So it is a little bit strange. That I need to give the variables to the overlay.html.twig to access them in my own twig file. Better would be only have to give variables in it which are really used by the overlay.html.twig and block of the embed still has access to the one by its own file:
{# using.html.twig #}
{% set title = 'Title' %}
{% set content = 'Content' %}
{% embed "overlay.html.twig" only %}
{% block title %}
{{ title }} {# still have access to this one #}
{% endblock %}
{% block content %}
<div>
{{ content }} {# still have access to this one #}
</div>
{% endblock %}
{% endembed %}
Another solution would be maybe make it transparency on the embed block e.g.:
{# using.html.twig #}
{% set title = 'Title' %}
{% set content = 'Content' %}
{% embed "overlay.html.twig" only %}
{% embedblock title with { title: title} %}
{{ title }}
{% endembedblock %}
{% embedblock content with { content: content} %}
<div>
{{ content }}
</div>
{% endembedblock %}
{% endembed %}
I hope I did make it understandable where the issues with the embed tag is.
render will conflict with the Symfony function rendering the response of a sub-request, which exists since 10 years in the Symfony ecosystem ? https://github.com/symfony/symfony/blob/v7.1.6/src/Symfony/Bridge/Twig/Extension/HttpKernelExtension.php#L28
@alexander-schranz the {% embed %} tag actually defines an anonymous template extending the provided template name, with the body of the tag being the body of that anonymous template. The blocks inside it are normal blocks, but they are blocks defined by the anonymous template.
Your proposed syntax will not work, because blocks are rendered with the context from the caller (i.e. the parent template). they have no way to access the context of the place including the template.
@stof I understand that it is a technical challange to change that behaviour. Just wanted to give feedback that from end user perspective it just felled for me strange how it behaves currently.
@fabpot is the new terminology variables as opposed to context or does context have a different meaning?
renderwill conflict with the Symfony function
Maybe we can keep the include function, but add a flag to opt into the new behavior and deprecate not doing so.
@derrabus technically, that flag already exists in include: with_context = false
@derrabus technically, that flag already exists in
include:with_context = false
Perfect. So let's deprecate not setting that flag to false explicitly?
I thought @derrabus meant a flag in the configuration, like how you can ignore missing variables.
@willrowe setting a flag in the Environment is bad, as it expects that the migration is done all at once, including all third-party templates. Switching the behavior of a function through a global state basically makes it unreliable for third-party code.
@lyrixx think twig-cs-fixer from @VincentLanglet can do that work. Upgrading include without_context = false to the new render I think should be possible without lot of effort.
Problem is more about include where the context was not false which was default at current state. Sure quickfix could be give everywhere _context in it with a fixer rule but think that is something I would not suggest todo, but is sure the "easiest" upgrade way.
Example: https://github.com/VincentLanglet/Twig-CS-Fixer/pull/327
For such rule it is sure easier if include get renamed to render. As the upgrade is then easier. Else a "fixer" is not able to skip already converted includes only if we would make still possible to set with_context = false to mark already converted includes, even true throws an error.
Please don't make this change. Neither including or excluding context is purely good or bad. Many teams use either approach successfully. Many teams run into issues with either approach.
It is typical for such cases to only hear about the teams that run into issues with the current default while not hearing anything from the other side as they are happy with how things are.
Switching the default (or worse, deprecating the current default) will not actually do any good for the wider community.
Is there any dedicated issue where this was discussed in detail?
From a business perspective this change alone would be devastating for us. We have thousands of twig templates we actively maintain.
We can't rewrite all of these so that each variable is passed explicitly. There also isn't any tool that can automate this refactor.
Please do not just make such massively breaking changes. Including other templates is the core of any templating system. Changing this aspect is unthinkable imho.
I think a more prudent approach could be to add new functionality without deprecating the old behavior.
Let users pick the behavior that best suites their context/project.
Needing to add _context to each and every template include also seems horrible to me.
The entire feature of always having the context available in included templates is that you don't need to do that. :)
Why place extra burden on template authors? Why make the easy cases harder?
