Compilation omits non-rendering code in extends

[pago]

Explain the problem

When using template inheritance, Melody will omit anything except for top-level variable assignments (using {% set .... %}) that is not contained within a block.

Expected Behaviour

Melody should include all code that is non-rendering in its compilation.

Actual Behaviour

Melody only compiled top-level variable assignments.

Steps to reproduce

bar.html.twig

{% extends "foo.html.twig" %}

{% if bar %}
    {% set foo = "hello" %}
{% endif %}

foo.html.twig

{{ foo | default('Ciao') }}!!

Rendering bar.html.twig with bar set to true should emit hello!! but will instead emit Ciao!!.

Provide a repository that reproduces issue if possible

Provide your Environment details

• Node version:
  any
• Operating System:
  any
• melody version:
  any

[danielcaldas]

I would like to look into this. To start here is the repository that reproduces the issue https://github.com/danielcaldas/issue-non-rendering-code-in-extends

[danielcaldas]

In discussion with @jamesb3ll we tought of two possible solutions for this issue.
For reference here is the pseudo output of compiling the given example.

foo.html.twig

_template.render = function (_context) {
  text(_context.foo != null && _context.foo !== '' ? _context.foo : "Ciao");
};

bar.html.twig

_template.render = function (_context) {
  let foo;
  foo = "hello";

  _parent.render.call(_template, _context); // render parent foo.html.twig
};

Solution 1

foo is in fact expected on the _context when rendering the parent so instead of only declaring the variable in the bar.html.twig we could also write this variable in the _context.

Solution 2

Since render is executed with the _template context we could assign the foo to the _template scope and access it via this.foo on the parent render function.

Do you think these are valid approaches? Can you think of a better one for this? I would like to hear your opinion @pago

[jamesb3ll]

Problem with Solution 2, how will foo.html.twig know to access foo from this instead of _context. Writing to _context seems better to me, but there is probably a solution we haven't thought of? Or any side-effects that writing to _context could have?

[pago]

We don't use this as far as I remember so solution 1 would be more appropriate.
But please make sure that you don't actually mutate _context. Instead, you'll need to create a new child context, mutate that and pass it to the parent. For that specific case, we should already be able to do it if you adapt the inference for the "local usage only" optimization.
If you'd add a {% include "foo.twig" %} Melody would do the set correctly. So the inference should treat inheritance as if it was an include by disabling the optimization.

The original bug report is also about Melody dropping if and similar statements in inherited templates. Will you have a look at that as well?

[danielcaldas]

Ok we agree that solution 1 is more appropriate considering the things that @pago added regarding _context mutation.

Regarding the drop of if and similar statements.. So are we thinking of removing those statements and throw some error when a user tries to use them on inherited templates? I'm a bit confused because at the moment twig allows the use of those statements inside blocks, would we be removing that capability as well?

[pago]

Sorry for the confusion. Let me clarify:

Given the template above:

{% extends "foo.html.twig" %}

{% if bar %}
    {% set foo = "hello" %}
{% endif %}

Melody will currently emit something like what you provided:

_template.render = function (_context) {
  let foo;
  foo = "hello";

  _parent.render.call(_template, _context); // render parent foo.html.twig
};

Problem 1 is not passing the new value of foo to the parent, however, problem 2 is that we lost the if condition (if bar).

Fixing one without the other will only result in more issues. :)

[pago]

@danielcaldas I can't remember but I think we didn't address this yet, right?

[danielcaldas]

Hey @pago no I didn't finalize it, there is some work in progress here https://github.com/danielcaldas/melody/tree/fix/omitted-code-twig-extends, but it's been a while.