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

Error "A block definition cannot be nested under non-capturing nodes" not emitted when there is no parent #3926

Open
ericmorand opened this issue Dec 3, 2023 · 27 comments

Comments

@ericmorand
Copy link
Contributor

ericmorand commented Dec 3, 2023

When rendering the following template, I get the error A block definition cannot be nested under non-capturing nodes:

{% extends "layout.twig" %}

{% if false %}
    {% block content %}FOO{% endblock %}
{% endif %}

(note that there is no need for the layout.twig template to exist for the error to be thrown)

But when rendering the following one, I don't get any error:

{% if false %}
    {% block content %}FOO{% endblock %}
{% endif %}

In both cases, there is a block definition nested under a non-capturing node (if), so why is the first template failing and why is the second one not failing?

@stof
Copy link
Member

stof commented Dec 4, 2023

A {% block %} has 2 effects:

  • it creates a block definition
  • if the block is not top-level in a child template, it creates a block reference to render that block

That's why the second case is not forbidden: block references are perfectly fine inside non-capturing nodes.

This is related to the way the extends feature works: https://twig.symfony.com/doc/3.x/tags/extends.html
If there were top-level non-capturing nodes in a child template, we would not know where this output should go in the final output.

@ericmorand
Copy link
Contributor Author

ericmorand commented Dec 4, 2023

In the first example, it is a block definition? That sounds counter intuitive to me. Isn't the block definition in the parent template - layout.twig?

I mean the first example is a template that inherits from another template and that references a block that was defined in the parent template.

In the second example however there is no parent so the block tag defines a new block. It doesn't reference anything since there is no previous definition of the block.

I have the feeling that your explanation says exactly the opposite.

Anyway, my original question was more about user land: I feel that the error message doesn't convey anything that can help understand and fix the issue.

@ericmorand
Copy link
Contributor Author

ericmorand commented Dec 5, 2023

@stof , I've been testing this capturing node rule and it is very confusing. Look at this template:

https://twigfiddle.com/j1gr3s

child

{% extends "layout.twig" %}

{% set foo %}
    {% block content %}child{% endblock %}
{% endset %}

parent

I am the

{% block content %}
parent
{% endblock %}

Even though the content block is captured in the child template - i.e. is not expected by the template designer to be displayed -, rendering the child template outputs this with Twig 3:

I am the

child

And it is not different from...

child

{% extends "layout.twig" %}

{% if false %}
    {% block content %}child{% endblock %}
{% endif %}

parent

I am the

{% block content %}
parent
{% endblock %}

...where the template designer also expect the child content to not be displayed, but also outputs the exact same thing with Twig 2 (Twig 3 forbids the syntax):

I am the

child

So, what is the difference?

The result is the same in both situations and in both cases this is not the expected result.

So, why is one syntax accepted and the other not accepted if they both yield the exact same incorrect output?

Why are they not both forbidden? Apparently the langage doesn't support nested blocks on inheriting templates at all - and yes considering how the extends tag work internally, it clearly can't support this pattern - why not forbid it entirely instead of forbidding one case that does not work (non-capturing) and allowing another that doesn't work either (capturing)?

@stof
Copy link
Member

stof commented Dec 15, 2023

You are not capturing the block definition. You are capturing the output of rendering that block reference in a foo variable. But the block is still always defined. Block definitions are part of the template structure at compile time.

Think about block definitions as protected methods in a PHP class and block references as calls to those methods.

Putting a block inside a capturing variable can work (except that in your case, you never use that variable after the fact). It is indeed weird to reuse a name of a parent block in such variable. But we cannot forbid defining blocks there as that would forbid some valid usages.

@stof
Copy link
Member

stof commented Dec 15, 2023

In the first example, it is a block definition? That sounds counter intuitive to me. Isn't the block definition in the parent template - layout.twig?

Both templates are defining blocks.
This is the same than in PHP classes with protected methods: the child class still defines a method to be able to override the parent method.

@ericmorand
Copy link
Contributor Author

I think the main issue here is conceptual: the block tag serves to declare and to reference - which is what you explain:

You are not capturing the block definition. You are capturing the output of rendering that block reference in a foo variable.

This is where you comparison with PHP fails in my opinion. In PHP you declare protected methods but declaring them doesn't mean executing them.

In Twig, declaring a block also means outputting it. And it should definitely not be the case. The block tag should be strictly dedicated to declare blocks - and override those that exists in a parent - and the block function should be used to execute a block.

If Twig worked this way, this template would be analogous to method override in php:

{% block foo %}
{{ block("foo") }} plus something else
{% endblock %}

{{ block("foo") }} outputs the parent block followed by " plus something else"

Here we have to first declare a block - overriding the parent and executing it - and then to execute the block.

Saying that Twig's behaviour today is comparable to php protected method is not true and this is the reason why blocks are confusing and why there are some arbitrary constraints like not allowing block inside non capturing blocks but allowing them inside capturing ones.

Block declaration should be allowed only at the root level of templates and should not output their content. And then it would be analogous to php behaviour.

@stof
Copy link
Member

stof commented Dec 15, 2023

The block tag always defines. And sometimes, it also does a reference, because that would require a totally different syntax otherwise, to separate the definition of blocks from their usage in templates that don't extend another one (where the whole template is rendered content.

In Twig, declaring a block also means outputting it. And it should definitely not be the case. The block tag should be strictly dedicated to declare blocks - and override those that exists in a parent - and the block function should be used to execute a block.

This would be a huge BC break in Twig, and a very bad DX if we were forcing to write code like that all the time:

{% block foo %}
{# some content for the block #}
{% endblock %}
{# render the block so that it is not a no-op #}
{{ block('foo') }}

And it could lead to lots of WTF moments about why the block is not rendered.
Thus, I don't see any way to make this BC break in a backward compatible way (where we first deprecate the old behavior) as this suggested new usage is already valid code today (and will render the block twice).

Block declaration should be allowed only at the root level of templates and should not output their content. And then it would be analogous to php behaviour.

The issue is that the root level of a root content is what gets output. This cannot be changed without changing the Twig language entirely (and making it even more confusing to users IMO).

Saying that Twig's behaviour today is comparable to php protected method is not true and this is the reason why blocks are confusing and why there are some arbitrary constraints like not allowing block inside non capturing blocks but allowing them inside capturing ones.

I'm not saying that they are exactly the same (this cannot be the case as defining a function/class in PHP does not evaluate directly either).

{% block foo %}
{{ block("foo") }} plus something else
{% endblock %}

{{ block("foo") }} outputs the parent block followed by " plus something else"

This suggested code snippet is actually doing an infinite recursion due to the block foo rendering itself (and if you continue the analogy with PHP methods, this is also an infinite recursion in PHP)

@stof
Copy link
Member

stof commented Dec 15, 2023

Btw, another thing to remember there is that those 2 effects of the block tag happen at different time:

  • declaring a block is a compile-time effect (altering the structure of the template)
  • referencing a block to display it is a render-time effect

Capturing some output in a variable (or evaluating a {% if false %} condition) is a render-time behavior, so it won't have any impact about the compile-time effect.

The big difference between the {% if false %} and {% set foo %} examples you gave is that Twig can be 100% sure at compile-time that the first example is a mistake. For the second case, it cannot (defining a root variable that the parent template can use and putting an extension point in that logic is a valid use case). A validation rule catching mistake can still be OK to implement when it has false negatives (not catching all mistakes you do). But it must not have false positives (telling you that your code is invalid when it is not).

@ericmorand
Copy link
Contributor Author

I'm not saying that it can be done right now. I say that it should have been done this way to begin with and that the current rational "it's like php" is not true and is a rewriting of history: a mistake was made to begin with and we must live with it; but this mistake is not coming from trying to match php because in php declaration and usage are not using the same syntax and are not expressed at the same level ( root of a class for declaration; body of a method for usage).

About my example that is recursive, you are right. I wanted to use the parent function - which is analogous to super.

Anyway, clearly, we can't do much today. But when Twig 4 is specified, i think the block situation should be sorted out.

@ericmorand
Copy link
Contributor Author

Also don't forget the own rule of thumb of twig: a tag should not display anything.

This is written in the documentation:

https://twig.symfony.com/doc/1.x/tags/include.html

The include function is semantically more "correct" (including a template outputs its rendered contents in the current scope; a tag should not display anything);

So the block tag is wrong at multiple levels:

  • It is used to both declare and output a block
  • It displays something even though "tag should not display anything"

The rule that deprecated the include tag also applies to the block tag: it should not be used to display a block, the block function should be used for that.

@xabbuh
Copy link
Contributor

xabbuh commented Dec 15, 2023

This will most likely not change in the future due to the reason given by @stof in #3926 (comment).

@ericmorand
Copy link
Contributor Author

ericmorand commented Dec 15, 2023

That it is changed anytime soon or not is not the point: recognizing the issue would be a good start and discussing how this can be fixed in the next major version is another.

You agree that there can't be discrepancies in the language philosophy (should tag display anything or not?) that we are not allowed to discuss and improve on, right?

I mean: the whole rule "a tag should not display anything" was added to the documentation by @fabpot himself, so surely this is something that drives the langage conception and that we all should be aware of and honor, right?

2d90248

@stof
Copy link
Member

stof commented Dec 15, 2023

It is not that a tag must not display anything. Tags are meant to define the template structure. The include tag was replaced by a function because displaying something was its only purpose.

@stof
Copy link
Member

stof commented Dec 15, 2023

If tags were not allowed to ever render anything, we would also have to nuke the {% embed %} tag entirely (forcing you to store that template separately to include it)

@stof
Copy link
Member

stof commented Dec 15, 2023

Anyway, clearly, we can't do much today. But when Twig 4 is specified, i think the block situation should be sorted out.

Twig 4 must still have a progressive migration path (we don't want to create a situation like Python 2 vs Python 3.0 where we split the whole ecosystem). And as I explained, your proposal does not allow to provide such migration path.
And I also think that this would make the Twig language harder to use for the 90% of simple cases.

I say that it should have been done this way to begin with and that the current rational "it's like php" is not true and is a rewriting of history: a mistake was made to begin with and we must live with it; but this mistake is not coming from trying to match php because in php declaration and usage are not using the same syntax and are not expressed at the same level ( root of a class for declaration; body of a method for usage).

I would not say that a mistake was done. "it's like php" is not the way the block feature of Twig has ever been defined. I made an analogy with PHP protected methods at some point of my explanation. But I never said that they are 100% the same (and they will never be able to be 100% the same anyway: this would require having an equivalent of classes in Twig as well...)

The Twig syntax was inpired by the Jinja syntax (in the Python world) where blocks have the same semantic than in Twig.
And I strongly think that the current situation provides better DX for template authors that a syntax where {% block %} only performs a declaration

@ericmorand
Copy link
Contributor Author

ericmorand commented Dec 15, 2023

If tags were not allowed to ever render anything, we would also have to nuke the {% embed %} tag entirely (forcing you to store that template separately to include it)

Absolutely. That's what the rule says. Maybe it is not legit - I actually think that it is and that it makes sense that tags are structural and don't display anything and that expressions do display something - but as of today it exists and it is not honoured by the block tag.

We could also discuss the do tag that is neither structural nor outputting anything and that should definitely be a function, but I think I already opened an issue about these discrepancies.

@stof
Copy link
Member

stof commented Dec 15, 2023

do cannot be a function. functions are used inside expression. The only purpose of the do tag is to be able of executing an expression and ignoring its return value.

Note that expressions are not what display something. {{ }} is the display syntax. Functions are returning something.

@andriusvo
Copy link

@ericmorand Can you give an example, how in your case it should be now to get it work?

@xabbuh
Copy link
Contributor

xabbuh commented Dec 29, 2023

@ericmorand Can you please answer @andriusvo's question. Right now it's not clear how you imagine one would have to write templates in the future. Right now it seems to me that dealing with blocks would suddenly become more difficult for template authors.

@ericmorand
Copy link
Contributor Author

@xabbuh:

A few examples. Currently they either don't work because the parser arbitrarily forbid them, or they work but output the block twice (namely the last example). Still, they are in my opinion not more difficult than the current approach and more obvious - {% block %} serves as declaring blocks, block() and parent() as executing them.

Simple case

parent.twig

{% block foo %}
foo
{% endblock %}

child.twig

{% extends "parent.twig" %}

{{ block("foo") }}

Override

parent.twig

{% block foo %}
foo
{% endblock %}

child.twig

{% extends "parent.twig" %}

{% block foo %}
bar
{% endblock %}

{{ block("foo") }}

Override with call to "super"

parent.twig

{% block foo %}
foo
{% endblock %}

child.twig

{% extends "parent.twig" %}

{% block foo %}
{{ parent() }} bar
{% endblock %}

{{ block("foo") }}

Override and no output by child

parent.twig

{% block foo %}
foo
{% endblock %}

<div class="foo">
{{ block("foo") }}
</div>

child.twig

{% extends "parent.twig" %}

{% block foo %}
bar
{% endblock %}

This one would render as expected to:

<div class="foo">
bar

</div>

@xabbuh
Copy link
Contributor

xabbuh commented Dec 29, 2023

There is one big downside with your proposal: As soon as I introduce a new block in the parent template I would have to update all child templates to make them output the newly defined block. That’s something that’s not going to work as child templates can be provided by other packages that I have no control of and that I cannot change.

@ericmorand
Copy link
Contributor Author

ericmorand commented Dec 29, 2023

That's also one of the benefit actually: as a consumer of your package, I don't want an unwanted block that you did add to your template to actually be output into mine and mess with my rendering. That would actually be a breaking change.

This is exactly what happends currently with Twig 3 and this is a big issue: whenever someone change a template that another temlplate inherits from, the rendering of the child template is either totally messed up - something is output that is not expected to be there - or simply crashes - if the parent template now depends on a data that is not passed to the child template.

So, yeah, that would also solve this issue.

@stof
Copy link
Member

stof commented Jan 29, 2024

Well, a child template always depend on its parent. That's the whole point of inheritance.

The same is true for any language that has inheritance: when you have a class C that extends class P, changes in the parent class P will affect what happens when you use class C.

@ericmorand
Copy link
Contributor Author

ericmorand commented Jan 29, 2024

Yes, but if you change a class by adding a method, it does not mess with inheriting classes. With Twig, if you add a block to a template, every template that inherits from that parent template will output the added block or crash.

Don't you agree that you would find this behaviour unacceptable in any other language that supports inheritance?

And to make things clear:

@xabbuh says that separating block definition and block output would remove one of the pros of Twig - aka the fact that adding a block to a template impacts the templates that inherit from that template.

Then you explain that this is a behaviour shared by every language that supports inheritance, which is wrong: no language implements this behaviour for obvious reasons.

I understand that you don't want to change the way blocks are defined and output but I have the feeling that your arguments are fallacious and try to convince us that the current behaviour is the expected and natural one:

  • Having newly added block being implicitly output in child templates is neither expected nor natural
  • This behaviour violates the commonly agreed expectations about inheritance where added behaviour to the parent class doesn't impact the child ones

@stof
Copy link
Member

stof commented Jan 29, 2024

@ericmorand the root template is the one defining the rendered output, with slots where child can replace the content.

In your example, you haven't just added an unused block in the root template. You have added a new part of the output, with a configurable slot to replace it in children (with the child not replacing it yet).

@ericmorand
Copy link
Contributor Author

@stof yes, that's how it is. But saying that this is a pro of the language and a natural behaviour expected from any language that support inheritance is wrong. That's the point I want to make.

@stof
Copy link
Member

stof commented Jan 29, 2024

@ericmorand if you want to make an analogy with PHP inheritance, this is the equivalent of a parent class adding a new protected method (allowing child classes to hook into it) and calling that new method as part of its public render() method. So even if a child class does not override the new method, it still won't be the same than the old code.

I have the impression that your own mental model of the inheritance compares this to a parent abstract class providing only some protected methods for usages by child classes but not defining any of the public API. This is not the right analogy. Twig does not have "abstract" templates that cannot be rendered directly on their own (whether rendering the base.html.twig template directly makes sense from a business point of view based on what this template does in your project is a totally different matter. You could still decide to render that one instead of a child).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants