twigphp / Twig

Twig, the flexible, fast, and secure template language for PHP
https://twig.symfony.com/
BSD 3-Clause "New" or "Revised" License
8.17k stars 1.25k forks source link

Twig 2.11 is a breaking change - and still it is a minor release #3090

Closed ericmorand closed 1 month ago

ericmorand commented 5 years ago

The documentation explictly says so:

Before Twig 2.11, it was possible to use macros imported in a block in a "sub-block". When upgrading to 2.11, you need to either move the import in the global scope or reimport the macros explicitly in the "sub-blocks".

https://twig.symfony.com/doc/2.x/tags/macro.html#macros-scoping

A template written pre-2.11 is not guaranteed to render to the same result - or even compile successfully - using 2.11. This is a breaking change.

stof commented 5 years ago

Can you share a reproducer (using https://twigfiddle.com/ for instance) ?

ericmorand commented 5 years ago

Here is a fiddle that works with Twig 2.10.0 but fails with Twig 2.11.0:

https://twigfiddle.com/ea4eqj

It implements exactly what is in the doc:

Before Twig 2.11, it was possible to use macros imported in a block in a "sub-block". When upgrading to 2.11, you need to either move the import in the global scope or reimport the macros explicitly in the "sub-blocks".

simshaun commented 5 years ago

Just ran into this. My use case that broke on 2.11 (dumbed down): https://twigfiddle.com/1oy6q4

{% block body_html -%}
  {% import 'macros.twig' as macros %}

  {% block header_wrap %}
    {{ macros.spacer(16) }}
    Header
    {{ macros.spacer(32) }}
  {% endblock %}

  {% block footer_wrap %}
    {{ macros.spacer(32) }}
    Footer
    {{ macros.spacer(16) }}
  {% endblock %}
{%- endblock %}
althaus commented 5 years ago

I've just had an issue with this, too:

{# macros.html.twig #}
{% macro foo %} ... {% endmacro %}
{# base.html.twig #}
{% import 'macros.html.twig' as m %}
{# page.html.twig #}
{% extends 'base.html.twig' %}
{% import _self as m %}

{{ m.foo }}

fails with Twig 2.11.3 complainig about Macro "foo" is not defined in template "page.html.twig".

stof commented 5 years ago

@althaus your own case was never meant to work, as page.html.twig does not contain a foo macro that you could import.

ericmorand commented 5 years ago

@stof, his example (reworked to add a block that he probably forgot to paste here) works with Twig 2.10:

https://twigfiddle.com/asu8dr

Not with Twig 2.11.

That it was never meant to work is debatable. page.html.twig extends from a template that imports some macros. Is there something in the doc that says that macros are not available to child templates? I can't find anything about this in the extends documentation.

althaus commented 5 years ago

@ericmorand Yes, I tried to reduce our acutal code to show the problen. Thanks for creating the working fiddle!

@stof: Don't know if this was meant to work, but it worked for roughly the last 5 years.

stof commented 5 years ago

@ericmorand the doc for macros are saying:

The macros can then be called at will in the current template: https://twig.symfony.com/doc/2.x/tags/macro.html#importing-macros

And there is another note explaining that a bit later in the doc (and this note was already there at least in Twig 2.9, so it is not something added to describe the changes done in 2.11):

Importing macros using import or from is local to the current file. The imported macros are not available in included templates or child templates; you need to explicitely re-import macros in each file. https://twig.symfony.com/doc/2.x/tags/macro.html#macros-scoping

stof commented 5 years ago

I understood the issue reported by @althaus. He relied on a side-effect of the internal implementation of macros, where the m import in the parent would stay in the context in the child, and the fact that there is a macro import with the same name in the child means the child also considers m.foo as a macro call (and so compiles it to the proper code). Note that this case is highly confusing, as any macro defined in the child (and so imported by the {% import _self as m %}) would not work (as m gets replaced entirely by the parent import). This was never considered as supported, and the only way to keep that working would be to never change the internal implementation of macros (and to never fix any of the macro issues, as the fact that _self macros would not be usable after being imported would rightfully be considered as a bug by others for instance).

ericmorand commented 5 years ago

@stof, thanks for the clarification with the doc.

About the issue itself, it raises the same kind of debate that #3091 raised: if the implementation is faulty - and pre 2.11 macro support was faulty, shouldn't the existing project relying on this faulty implementation be considered as rightful?

This is what you wrote for #3091:

Existing projects care about the implementation, not about what the doc says or not, regarding what breaks them.

That's exactly what is happening there with @althaus - and many others - project: it cares about the implementation, not the doc. What makes this situation different from #3091 ?

althaus commented 5 years ago

@stof We started with Twig 1.15.0... so dunno how clear the doc has been at that time. ;-)

So I've basically have to check all our templates and verify that they are properly importing any macros from any parent on their own? So a template is a black box regarding the resolving of macros?

That sounds like fun... :(

adampippin-cmg commented 4 years ago

Wanted to throw another use case in here that this change breaks, since it's a little different: Apparently macros can no longer be passed as arguments to functions/extensions.

Simplified example of what I was doing previously:

$twig_environment->addFunction(\Twig\TwigFunction('my_function', function($macros) { /* do stuff with the macros */ }));
{% import ('my/macros.twig') as plugin %}
{{ my_function(plugin) }}

This now results in null being passed to the custom function. Adding needs_context and examining the context it seems like the macros are no longer present anywhere.

I understand this may or may not have not been a supported use case (though that's not really clear to me -- this is access within the same block), but it was certainly unexpected from a minor, supposedly backwards-compatible, update.

Unfortunately I can't simply roll back as others have been doing because I need to support PHP 7.4.